Re: [PATCH 3/3] genhomedircon: avoid use of non-standard `getpwent_r`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4847 bytes --]

On Tue, Jun 20, 2017 at 11:42:59AM -0400, Stephen Smalley wrote:
> On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> > The `getpwent_r` function is a non-standard but re-entrant version of
> > the sdardardized `getpwent` function. Next to the benefit of being
> > re-entrant, though, it avoids compilation with libc implementations
> > that
> > do not provide this glibc-specific extension, for example musl libc.
> > 
> > As the code is usually not run in a threaded environment, it is save
> > to
> > use the non-reentrant version, though. As such, convert code to use
> > `getpwent` instead to fix building with other libc implementations.
> 
> Not sure we are guaranteed that, since libsemanage has external users
> too, e.g. sssd among others.  OTOH, getpwent_r man page says that it is
> not really reentrant either due to shared reading position in the
> stream, so maybe this doesn't matter.

Hum, okay. man-pages differ here in their wording across systems,
but I guess the part you're referring to is

```
The functions getpwent_r() and fgetpwent_r() are the reentrant
versions of getpwent(3) and fgetpwent(3). The former reads the
next passwd entry from the stream initialized by setpwent(3). The
latter reads the next passwd entry from the stream fp.
```

While it indeed seems to imply that the stream by `setpwent` is
used, they also pretend to be reentrant here. Puzzled.

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  libsemanage/src/genhomedircon.c | 22 +++++-----------------
> >  1 file changed, 5 insertions(+), 17 deletions(-)
> > 
> > diff --git a/libsemanage/src/genhomedircon.c
> > b/libsemanage/src/genhomedircon.c
> > index e8c95ee4..f58c17ce 100644
> > --- a/libsemanage/src/genhomedircon.c
> > +++ b/libsemanage/src/genhomedircon.c
> > @@ -295,9 +295,8 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >  	long rbuflen;
> >  	uid_t temp, minuid = 500, maxuid = 60000;
> >  	int minuid_set = 0;
> > -	struct passwd pwstorage, *pwbuf;
> > +	struct passwd *pwbuf;
> >  	struct stat buf;
> > -	int retval;
> >  
> >  	path = semanage_findval(PATH_ETC_USERADD, "HOME", "=");
> >  	if (path && *path) {
> > @@ -369,7 +368,7 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >  	if (rbuf == NULL)
> >  		goto fail;
> >  	setpwent();
> > -	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen,
> > &pwbuf)) == 0) {
> > +	while ((pwbuf = getpwent()) != NULL) {
> 
> Shouldn't you have removed rbuflen and rbuf too?  They are no longer
> used (except to allocate and free, but never used otherwise) after this
> change.

That was an oversight, yeah. Fixed in v2.

> >  		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid >
> > maxuid)
> >  			continue;
> >  		if (!semanage_list_find(shells, pwbuf->pw_shell))
> > @@ -413,7 +412,7 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >  		path = NULL;
> >  	}
> >  
> > -	if (retval && retval != ENOENT) {
> > +	if (errno && errno != ENOENT) {
> 
> Does getpwent() set errno to ENOENT? Not mentioned in its man page,
> unlike for getpwent_r().

You're right, it does not. Will remove this check.

> >  		WARN(s->h_semanage, "Error while fetching users.  "
> >  		     "Returning list so far.");
> >  	}
> > @@ -1064,10 +1063,7 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> >  	long grbuflen;
> >  	char *grbuf = NULL;
> >  	struct group grstorage, *group = NULL;
> > -
> > -	long prbuflen;
> > -	char *pwbuf = NULL;
> > -	struct passwd pwstorage, *pw = NULL;
> > +	struct passwd *pw = NULL;
> >  
> >  	grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
> >  	if (grbuflen <= 0)
> > @@ -1104,15 +1100,8 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> >  		}
> >  	}
> >  
> > -	prbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
> > -	if (prbuflen <= 0)
> > -		goto cleanup;
> > -	pwbuf = malloc(prbuflen);
> > -	if (pwbuf == NULL)
> > -		goto cleanup;
> > -
> >  	setpwent();
> > -	while ((retval = getpwent_r(&pwstorage, pwbuf, prbuflen,
> > &pw)) == 0) {
> > +	while ((pw = getpwent()) != NULL) {
> >  		// skip users who also have this group as their
> >  		// primary group
> >  		if (lfind(pw->pw_name, group->gr_mem, &nmembers,
> 
> Interestingly, this goes on to call add_user(), which calls
> getpwnam_r().  So if that one was ever switched back to just
> getpwnam(), we'd have a potential problem there.

Agreed. But in fact, `getpwnam_r` is part of POSIX, so there is
no need to do so here.

> > @@ -1131,7 +1120,6 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> >  	retval = STATUS_SUCCESS;
> >  cleanup:
> >  	endpwent();
> > -	free(pwbuf);
> >  	free(grbuf);
> >  
> >  	return retval;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]