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@xxxxxxxx> > > > > > > > > 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@xxxxxxxx> > > > > --- > > > > 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,
