* [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
@ 2019-01-02 13:46 Laurent Bigonville
2019-01-02 14:30 ` Laurent Bigonville
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Bigonville @ 2019-01-02 13:46 UTC (permalink / raw)
To: selinux
From: Laurent Bigonville <bigon@bigon.be>
The manpage explicitly states that:
The getpwent() function returns a pointer to a passwd structure, or
NULL if there are no more entries or an error occurred. If an error
occurs, errno is set appropriately. If one wants to check errno after
the call, it should be set to zero before the call.
Without this, genhomedircon can wrongly return the following:
libsemanage.get_home_dirs: Error while fetching users. Returning list so far.
https://github.com/SELinuxProject/selinux/issues/121
Signed-off-by: Laurent Bigonville <bigon@bigon.be>
---
libsemanage/src/genhomedircon.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 3e61b510..591941fb 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -361,7 +361,11 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
errno = 0;
setpwent();
- while ((pwbuf = getpwent()) != NULL) {
+ while (1) {
+ errno = 0;
+ pwbuf = getpwent();
+ if (pwbuf == NULL)
+ break;
if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
continue;
if (!semanage_list_find(shells, pwbuf->pw_shell))
@@ -403,7 +407,6 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
}
free(path);
path = NULL;
- errno = 0;
}
if (errno) {
@@ -1101,7 +1104,11 @@ static int get_group_users(genhomedircon_settings_t * s,
}
setpwent();
- while ((pw = getpwent()) != NULL) {
+ while (1) {
+ errno = 0;
+ pw = getpwent();
+ if (pw == NULL)
+ break;
// skip users who also have this group as their
// primary group
if (lfind(pw->pw_name, group->gr_mem, &nmembers,
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
2019-01-02 13:46 [PATCH] libsemanage: Always set errno to 0 before calling getpwent() Laurent Bigonville
@ 2019-01-02 14:30 ` Laurent Bigonville
2019-01-04 15:11 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Bigonville @ 2019-01-02 14:30 UTC (permalink / raw)
To: selinux
Le 2/01/19 à 14:46, Laurent Bigonville a écrit :
> From: Laurent Bigonville <bigon@bigon.be>
>
> The manpage explicitly states that:
>
> The getpwent() function returns a pointer to a passwd structure, or
> NULL if there are no more entries or an error occurred. If an error
> occurs, errno is set appropriately. If one wants to check errno after
> the call, it should be set to zero before the call.
>
> Without this, genhomedircon can wrongly return the following:
> libsemanage.get_home_dirs: Error while fetching users. Returning list so far.
>
> https://github.com/SELinuxProject/selinux/issues/121
>
> Signed-off-by: Laurent Bigonville <bigon@bigon.be>
> ---
> libsemanage/src/genhomedircon.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 3e61b510..591941fb 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -361,7 +361,11 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>
> errno = 0;
> setpwent();
> - while ((pwbuf = getpwent()) != NULL) {
> + while (1) {
> + errno = 0;
> + pwbuf = getpwent();
> + if (pwbuf == NULL)
> + break;
> if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
> continue;
> if (!semanage_list_find(shells, pwbuf->pw_shell))
> @@ -403,7 +407,6 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
> }
> free(path);
> path = NULL;
> - errno = 0;
Actually I'm wondering if this shouldn't stay there
> }
>
> if (errno) {
> @@ -1101,7 +1104,11 @@ static int get_group_users(genhomedircon_settings_t * s,
> }
>
> setpwent();
> - while ((pw = getpwent()) != NULL) {
> + while (1) {
> + errno = 0;
> + pw = getpwent();
> + if (pw == NULL)
> + break;
> // skip users who also have this group as their
> // primary group
> if (lfind(pw->pw_name, group->gr_mem, &nmembers,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
2019-01-02 14:30 ` Laurent Bigonville
@ 2019-01-04 15:11 ` Stephen Smalley
2019-01-04 15:57 ` Laurent Bigonville
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2019-01-04 15:11 UTC (permalink / raw)
To: Laurent Bigonville, selinux
On Wed, 2019-01-02 at 15:30 +0100, Laurent Bigonville wrote:
> Le 2/01/19 à 14:46, Laurent Bigonville a écrit :
> > From: Laurent Bigonville <bigon@bigon.be>
> >
> > The manpage explicitly states that:
> >
> > The getpwent() function returns a pointer to a passwd
> > structure, or
> > NULL if there are no more entries or an error occurred. If an
> > error
> > occurs, errno is set appropriately. If one wants to check errno
> > after
> > the call, it should be set to zero before the call.
> >
> > Without this, genhomedircon can wrongly return the following:
> > libsemanage.get_home_dirs: Error while fetching
> > users. Returning list so far.
> >
> > https://github.com/SELinuxProject/selinux/issues/121
> >
> > Signed-off-by: Laurent Bigonville <bigon@bigon.be>
> > ---
> > libsemanage/src/genhomedircon.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/libsemanage/src/genhomedircon.c
> > b/libsemanage/src/genhomedircon.c
> > index 3e61b510..591941fb 100644
> > --- a/libsemanage/src/genhomedircon.c
> > +++ b/libsemanage/src/genhomedircon.c
> > @@ -361,7 +361,11 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >
> > errno = 0;
> > setpwent();
> > - while ((pwbuf = getpwent()) != NULL) {
> > + while (1) {
> > + errno = 0;
> > + pwbuf = getpwent();
> > + if (pwbuf == NULL)
> > + break;
> > if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
> > continue;
> > if (!semanage_list_find(shells, pwbuf->pw_shell))
> > @@ -403,7 +407,6 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> > }
> > free(path);
> > path = NULL;
> > - errno = 0;
>
> Actually I'm wondering if this shouldn't stay there
Why?
>
> > }
> >
> > if (errno) {
> > @@ -1101,7 +1104,11 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> > }
> >
> > setpwent();
> > - while ((pw = getpwent()) != NULL) {
> > + while (1) {
> > + errno = 0;
> > + pw = getpwent();
> > + if (pw == NULL)
> > + break;
> > // skip users who also have this group as their
> > // primary group
> > if (lfind(pw->pw_name, group->gr_mem, &nmembers,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
2019-01-04 15:11 ` Stephen Smalley
@ 2019-01-04 15:57 ` Laurent Bigonville
2019-01-04 16:09 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Bigonville @ 2019-01-04 15:57 UTC (permalink / raw)
To: Stephen Smalley, selinux
Le 4/01/19 à 16:11, Stephen Smalley a écrit :
> On Wed, 2019-01-02 at 15:30 +0100, Laurent Bigonville wrote:
>> Le 2/01/19 à 14:46, Laurent Bigonville a écrit :
>>> From: Laurent Bigonville <bigon@bigon.be>
>>>
>>> The manpage explicitly states that:
>>>
>>> The getpwent() function returns a pointer to a passwd
>>> structure, or
>>> NULL if there are no more entries or an error occurred. If an
>>> error
>>> occurs, errno is set appropriately. If one wants to check errno
>>> after
>>> the call, it should be set to zero before the call.
>>>
>>> Without this, genhomedircon can wrongly return the following:
>>> libsemanage.get_home_dirs: Error while fetching
>>> users. Returning list so far.
>>>
>>> https://github.com/SELinuxProject/selinux/issues/121
>>>
>>> Signed-off-by: Laurent Bigonville <bigon@bigon.be>
>>> ---
>>> libsemanage/src/genhomedircon.c | 13 ++++++++++---
>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libsemanage/src/genhomedircon.c
>>> b/libsemanage/src/genhomedircon.c
>>> index 3e61b510..591941fb 100644
>>> --- a/libsemanage/src/genhomedircon.c
>>> +++ b/libsemanage/src/genhomedircon.c
>>> @@ -361,7 +361,11 @@ static semanage_list_t
>>> *get_home_dirs(genhomedircon_settings_t * s)
>>>
>>> errno = 0;
>>> setpwent();
>>> - while ((pwbuf = getpwent()) != NULL) {
>>> + while (1) {
>>> + errno = 0;
>>> + pwbuf = getpwent();
>>> + if (pwbuf == NULL)
>>> + break;
>>> if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
>>> continue;
>>> if (!semanage_list_find(shells, pwbuf->pw_shell))
>>> @@ -403,7 +407,6 @@ static semanage_list_t
>>> *get_home_dirs(genhomedircon_settings_t * s)
>>> }
>>> free(path);
>>> path = NULL;
>>> - errno = 0;
>> Actually I'm wondering if this shouldn't stay there
> Why?
According to strdup() manpage, it can also change errno, isn't there a
risk that errno is changed to an non-zero value and then causing the
warning below to be printed?
>
>>> }
>>>
>>> if (errno) {
>>> @@ -1101,7 +1104,11 @@ static int
>>> get_group_users(genhomedircon_settings_t * s,
>>> }
>>>
>>> setpwent();
>>> - while ((pw = getpwent()) != NULL) {
>>> + while (1) {
>>> + errno = 0;
>>> + pw = getpwent();
>>> + if (pw == NULL)
>>> + break;
>>> // skip users who also have this group as their
>>> // primary group
>>> if (lfind(pw->pw_name, group->gr_mem, &nmembers,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
2019-01-04 15:57 ` Laurent Bigonville
@ 2019-01-04 16:09 ` Stephen Smalley
2019-01-05 15:35 ` Nicolas Iooss
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2019-01-04 16:09 UTC (permalink / raw)
To: Laurent Bigonville, selinux
On Fri, 2019-01-04 at 16:57 +0100, Laurent Bigonville wrote:
> Le 4/01/19 à 16:11, Stephen Smalley a écrit :
> > On Wed, 2019-01-02 at 15:30 +0100, Laurent Bigonville wrote:
> > > Le 2/01/19 à 14:46, Laurent Bigonville a écrit :
> > > > From: Laurent Bigonville <bigon@bigon.be>
> > > >
> > > > The manpage explicitly states that:
> > > >
> > > > The getpwent() function returns a pointer to a passwd
> > > > structure, or
> > > > NULL if there are no more entries or an error occurred. If
> > > > an
> > > > error
> > > > occurs, errno is set appropriately. If one wants to check
> > > > errno
> > > > after
> > > > the call, it should be set to zero before the call.
> > > >
> > > > Without this, genhomedircon can wrongly return the following:
> > > > libsemanage.get_home_dirs: Error while fetching
> > > > users. Returning list so far.
> > > >
> > > > https://github.com/SELinuxProject/selinux/issues/121
> > > >
> > > > Signed-off-by: Laurent Bigonville <bigon@bigon.be>
> > > > ---
> > > > libsemanage/src/genhomedircon.c | 13 ++++++++++---
> > > > 1 file changed, 10 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/libsemanage/src/genhomedircon.c
> > > > b/libsemanage/src/genhomedircon.c
> > > > index 3e61b510..591941fb 100644
> > > > --- a/libsemanage/src/genhomedircon.c
> > > > +++ b/libsemanage/src/genhomedircon.c
> > > > @@ -361,7 +361,11 @@ static semanage_list_t
> > > > *get_home_dirs(genhomedircon_settings_t * s)
> > > >
> > > > errno = 0;
> > > > setpwent();
> > > > - while ((pwbuf = getpwent()) != NULL) {
> > > > + while (1) {
> > > > + errno = 0;
> > > > + pwbuf = getpwent();
> > > > + if (pwbuf == NULL)
> > > > + break;
> > > > if (pwbuf->pw_uid < minuid || pwbuf->pw_uid >
> > > > maxuid)
> > > > continue;
> > > > if (!semanage_list_find(shells, pwbuf-
> > > > >pw_shell))
> > > > @@ -403,7 +407,6 @@ static semanage_list_t
> > > > *get_home_dirs(genhomedircon_settings_t * s)
> > > > }
> > > > free(path);
> > > > path = NULL;
> > > > - errno = 0;
> > > Actually I'm wondering if this shouldn't stay there
> > Why?
>
> According to strdup() manpage, it can also change errno, isn't there
> a
> risk that errno is changed to an non-zero value and then causing the
> warning below to be printed?
On strdup(pwbuf->pw_dir) failure, we'll break out of the loop with non-
zero errno and then correctly report that there was an error that
truncated the list. That's correct behavior. The errno = 0; were are
removing is within the loop and is obsoleted by your setting of it at
the beginning of the loop. So I think your patch is fine as is,
although I have not tested it.
>
>
> > > > }
> > > >
> > > > if (errno) {
> > > > @@ -1101,7 +1104,11 @@ static int
> > > > get_group_users(genhomedircon_settings_t * s,
> > > > }
> > > >
> > > > setpwent();
> > > > - while ((pw = getpwent()) != NULL) {
> > > > + while (1) {
> > > > + errno = 0;
> > > > + pw = getpwent();
> > > > + if (pw == NULL)
> > > > + break;
> > > > // skip users who also have this group as their
> > > > // primary group
> > > > if (lfind(pw->pw_name, group->gr_mem,
> > > > &nmembers,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
2019-01-04 16:09 ` Stephen Smalley
@ 2019-01-05 15:35 ` Nicolas Iooss
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Iooss @ 2019-01-05 15:35 UTC (permalink / raw)
To: Stephen Smalley, Laurent Bigonville; +Cc: selinux
On Fri, Jan 4, 2019 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On Fri, 2019-01-04 at 16:57 +0100, Laurent Bigonville wrote:
> > Le 4/01/19 à 16:11, Stephen Smalley a écrit :
> > > On Wed, 2019-01-02 at 15:30 +0100, Laurent Bigonville wrote:
> > > > Le 2/01/19 à 14:46, Laurent Bigonville a écrit :
> > > > > From: Laurent Bigonville <bigon@bigon.be>
> > > > >
> > > > > The manpage explicitly states that:
> > > > >
> > > > > The getpwent() function returns a pointer to a passwd
> > > > > structure, or
> > > > > NULL if there are no more entries or an error occurred. If
> > > > > an
> > > > > error
> > > > > occurs, errno is set appropriately. If one wants to check
> > > > > errno
> > > > > after
> > > > > the call, it should be set to zero before the call.
> > > > >
> > > > > Without this, genhomedircon can wrongly return the following:
> > > > > libsemanage.get_home_dirs: Error while fetching
> > > > > users. Returning list so far.
> > > > >
> > > > > https://github.com/SELinuxProject/selinux/issues/121
> > > > >
> > > > > Signed-off-by: Laurent Bigonville <bigon@bigon.be>
> > > > > ---
> > > > > libsemanage/src/genhomedircon.c | 13 ++++++++++---
> > > > > 1 file changed, 10 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/libsemanage/src/genhomedircon.c
> > > > > b/libsemanage/src/genhomedircon.c
> > > > > index 3e61b510..591941fb 100644
> > > > > --- a/libsemanage/src/genhomedircon.c
> > > > > +++ b/libsemanage/src/genhomedircon.c
> > > > > @@ -361,7 +361,11 @@ static semanage_list_t
> > > > > *get_home_dirs(genhomedircon_settings_t * s)
> > > > >
> > > > > errno = 0;
> > > > > setpwent();
> > > > > - while ((pwbuf = getpwent()) != NULL) {
> > > > > + while (1) {
> > > > > + errno = 0;
> > > > > + pwbuf = getpwent();
> > > > > + if (pwbuf == NULL)
> > > > > + break;
> > > > > if (pwbuf->pw_uid < minuid || pwbuf->pw_uid >
> > > > > maxuid)
> > > > > continue;
> > > > > if (!semanage_list_find(shells, pwbuf-
> > > > > >pw_shell))
> > > > > @@ -403,7 +407,6 @@ static semanage_list_t
> > > > > *get_home_dirs(genhomedircon_settings_t * s)
> > > > > }
> > > > > free(path);
> > > > > path = NULL;
> > > > > - errno = 0;
> > > > Actually I'm wondering if this shouldn't stay there
> > > Why?
> >
> > According to strdup() manpage, it can also change errno, isn't there
> > a
> > risk that errno is changed to an non-zero value and then causing the
> > warning below to be printed?
>
> On strdup(pwbuf->pw_dir) failure, we'll break out of the loop with non-
> zero errno and then correctly report that there was an error that
> truncated the list. That's correct behavior. The errno = 0; were are
> removing is within the loop and is obsoleted by your setting of it at
> the beginning of the loop. So I think your patch is fine as is,
> although I have not tested it.
I have tested it on my test system. Before applying the patch,
"semanage fcontext -m ..." displayed "libsemanage.get_home_dirs: Error
while fetching users. Returning list so far.". This message
disappears as expected with the patch.
Merged. Thanks!
Nicolas
>
> >
> >
> > > > > }
> > > > >
> > > > > if (errno) {
> > > > > @@ -1101,7 +1104,11 @@ static int
> > > > > get_group_users(genhomedircon_settings_t * s,
> > > > > }
> > > > >
> > > > > setpwent();
> > > > > - while ((pw = getpwent()) != NULL) {
> > > > > + while (1) {
> > > > > + errno = 0;
> > > > > + pw = getpwent();
> > > > > + if (pw == NULL)
> > > > > + break;
> > > > > // skip users who also have this group as their
> > > > > // primary group
> > > > > if (lfind(pw->pw_name, group->gr_mem,
> > > > > &nmembers,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-05 15:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-02 13:46 [PATCH] libsemanage: Always set errno to 0 before calling getpwent() Laurent Bigonville
2019-01-02 14:30 ` Laurent Bigonville
2019-01-04 15:11 ` Stephen Smalley
2019-01-04 15:57 ` Laurent Bigonville
2019-01-04 16:09 ` Stephen Smalley
2019-01-05 15:35 ` Nicolas Iooss
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).