Re: [PATCH] fix getgrouplist without nscd

[Rich Felker <dalias@libc.org>]

On Wed, Jun 21, 2017 at 07:23:10PM -0400, Rich Felker wrote:
> On Tue, Jul 12, 2016 at 11:30:57AM +0200, Joakim Sindholt wrote:
> > There were two issues here. First, if socket() fails then it's
> > treated as an unrecoverable error and second: the response buffer
> > was not initialized and so a recoverable error (ie. no nscd) would
> > result in UB.
> > 
> > dgp tested this patch and confirmed on IRC that it worked in the
> > no-nscd no-socket case and I have tested it in the has-nscd case.
> 
> > >From 9251aff794832e0e37f5b747b69e756ac836f181 Mon Sep 17 00:00:00 2001
> > From: Joakim Sindholt <opensource@zhasha.com>
> > Date: Tue, 12 Jul 2016 10:13:23 +0200
> > Subject: [PATCH] fix getgrouplist without nscd
> > 
> > ---
> >  src/passwd/getgrouplist.c | 2 +-
> >  src/passwd/nscd_query.c   | 8 +++++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/passwd/getgrouplist.c b/src/passwd/getgrouplist.c
> > index 43e5182..fd0bf5f 100644
> > --- a/src/passwd/getgrouplist.c
> > +++ b/src/passwd/getgrouplist.c
> > @@ -17,7 +17,7 @@ int getgrouplist(const char *user, gid_t gid, gid_t *groups, int *ngroups)
> >  	struct group *res;
> >  	FILE *f;
> >  	int swap = 0;
> > -	int32_t resp[INITGR_LEN];
> > +	int32_t resp[INITGR_LEN] = {0};
> >  	uint32_t *nscdbuf = 0;
> >  	char *buf = 0;
> >  	char **mem = 0;

As noted on irc, this part is redundant; __nscd_query already zeros
the buffer.

> > diff --git a/src/passwd/nscd_query.c b/src/passwd/nscd_query.c
> > index d38e371..8641e4f 100644
> > --- a/src/passwd/nscd_query.c
> > +++ b/src/passwd/nscd_query.c
> > @@ -40,7 +40,13 @@ retry:
> >  	buf[0] = NSCDVERSION;
> >  
> >  	fd = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
> > -	if (fd < 0) return NULL;
> > +	if (fd < 0) {
> > +		if (errno == EACCES || errno == EAFNOSUPPORT || errno == EPROTONOSUPPORT) {
> > +			errno = errno_save;
> > +			return fopen("/dev/null", "re");
> > +		}
> > +		return 0;
> > +	}
> >  
> >  	if(!(f = fdopen(fd, "r"))) {
> >  		close(fd);
> > -- 
> > 2.7.3
> > 
> 
> I just ran across this patch that got lost last year while looking at
> the git history for src/passwd and seeing commit
> 39494a273eaa6b714e0fa0c59ce7a1f5fbc80a1e. Was there any further
> progress or discussion on it I might be overlooking? I think the
> concept looks right but I wonder if there's a good way to avoid the
> spurious /dev/null access.

And the (unwritten, AFAIK) contract of __nscd_query is that the caller
is not expected to attempt to read from the FILE returned unless the
header obtained in the initial query indicates there's data to be
read. This is why it's able to return a FILE for an unconnected
socket, and it also means we should be fine returning a FILE for an
invalid (negative) fd.

This should probably be documented with a quick comment somewhere too,
but I can do that separately.

Anyway I think the main question left is just whether the above errno
cases are correct and cover the ways socket could fail for policy or
configuration reasons (as opposed to transient/resource failures).

Rich