* [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).