Re: [PATCH] nsswitch: Add group merging support

[Stephen Gallagher <sgallagh at redhat dot com>]

On 03/21/2016 08:40 PM, Mike Frysinger wrote:
> On 04 Jan 2016 09:12, Stephen Gallagher wrote:
> 
> overall sounds fine.  i have no strong opinions as i don't use nss myself :).
> style nits follow.
> 
> note: you should also update the NEWS file with a short blurb.
> the commit message is great, albeit too verbose for NEWS.

Carlos told me to just put it in the == NEWS == section of the commit message
and that it would get added to the NEWS file when it got merged in.

> 
>> --- /dev/null
>> +++ b/grp/grp-merge.c
>> @@ -0,0 +1,177 @@
>> +/* Group merging implementation.
>> +   Copyright (C) 2015 Free Software Foundation, Inc.
> 
> date needs updating
> 


Hmm, it looks like I somehow managed to send out a really old patch rather than
the latest one. This is already fixed in the latest version.

>> +#define BUFCHECK(size)		\
>> +  do {				\
>> +    if (c + size > buflen) {	\
>> +        free (members);		\
>> +        return ERANGE;		\
>> +    }				\
>> +  } while(0)
> 
> style is incorrect for GNU code.  perhaps (note use of tabs too):
> 
> #define BUFCHECK(size)		\
>   do {				\
>     if (c + (size) > buflen)	\
>       {				\
> 	free (members);		\
> 	return ERANGE;		\
>       }				\
>   } while(0)
> 

Already fixed in the latest version.


>> +  /* Allocate a temporary holding area for the pointers to the member
>> +     contents, including space for a NULL-terminator.  */
>> +  members = malloc (sizeof (char *) * (memcount + 1));
> 
> do we have to worry about overflow ?  if there's some external logic that
> keeps these from overflowing, might be nice to note somewhere (comes up a
> few times in this patch).
> 

The memcount is determined immediately prior to this and the same loop-control
is used to fill it, so it should be impossible to overflow here.


>> +  if (members == NULL)
>> +      return ENOMEM;
> 
> indented by two too many spaces
> 


Already fixed in the latest version.

>> +  /* Copy the pointers from the members array into the buffer and assign them
>> +     to the gr_mem member of destgrp.  */
>> +  destgrp->gr_mem = (char **) &destbuf[c];
>> +  len = sizeof (char *) * (memcount + 1);
>> +  BUFCHECK (len);
>> +  memcpy (&destbuf[c], members, len);
>> +  c += len;
>> +  free (members);
>> +  members = NULL;
> 
> force of habit clearing members ?  seems pointless otherwise.
> 

Yes and no; I've found in the past that remembering to do this is helpful for
catching mistakes when refactoring in the future. (A NULL-dereference is far
easier to notice than bizarre garbage in a pointer). I can take it out if you
prefer, but it's unlikely to be harmful.

>> +  if (endptr)
>> +      *endptr = destbuf + c;
> 
> too much indentation

Already fixed in latest version.

> 
>> +int
>> +__merge_grp (struct group *savedgrp, char *savedbuf, char *savedend,
>> +	     size_t buflen, struct group *mergegrp, char *mergebuf)
> 
> same comments for this func as above since it seems to be largely the
> same logic wise
> 

Same answers as above.


>> --- /dev/null
>> +++ b/grp/grp-merge.h
>> @@ -0,0 +1,35 @@
>> +/* Group merging implementation.
>> +   Copyright (C) 2015 Free Software Foundation, Inc.
> 
> update the date
> 

Already fixed in latest patch.

>> +/* Duplicate a grp struct (and its members). When no longer needed, the
>> +   calling function must free(newbuf). */
> 
> two spaces after the .
> 
>> +/* Merge the member lists of two grp structs together. */
> 
> same here
> 

Fixed both.


>> +int
>> +__merge_grp (struct group *savedgrp, char *savedbuf, char *savedend,
>> +	     size_t buflen, struct group *mergegrp, char *mergebuf);
> 
> should these two funcs have internal_function & attribute_hidden applied
> to them ?
> 

I talked with Florian about this today. You're definitely right about
internal_function, but he also pointed out that I should add __merge_grp() and
__copy_grp() to the GLIBC_PRIVATE section of Versions and avoid compiling this
file twice (once for NSS and once for nscd).

New patch does so.


>> --- a/manual/nss.texi
>> +++ b/manual/nss.texi
>>
>> +When processing @samp{merge} for @samp{group} membership, the group GID
>> +and name must be identical for both entries. If only one or the other is
> 
> two spaces after the .
> 

Fixed.


>> +a match, the behavior is undefined.
> 
> could you clarify "undefined" ?  people could interpret this as memory
> corruption / crashes, while others are are inconsistent results.  i think
> we just want the latter.
> 

OK, the language "undefined" was suggested to me by Carlos. In reality, the
results are actually *consistent*, but they're consistently different depending
on which attribute was initially searched.

(Meaning getgrnam() will always return the same results and getgrgid() will
always return the same results, but they will not be the same as each other.)

As for "undefined", I think I'd actually prefer to keep it that way, because
it's a strong assertion that you should be careful not to do this. The external
effect to an inconsistent set of responses from NSS is likely to have
wide-ranging negative effects on the system. I'm perfectly content to have
"scary" language in the documentation to guard against that.


>> --- a/nss/getXXbyYY_r.c
>> +++ b/nss/getXXbyYY_r.c
>>
>> +/* Set defaults for merge functions that haven't been defined.  */
>> +#ifndef DEEPCOPY_FN
>> +static inline int
>> +__copy_einval (LOOKUP_TYPE a,
>> +	      const size_t b,
>> +	      LOOKUP_TYPE *c,
>> +	      char *d,
>> +	      char **e)
> 
> indentation of args looks slightly off
> 

Fixed


>> +#ifndef MERGE_FN
>> +static inline int
>> +__merge_einval (LOOKUP_TYPE *a,
>> +	       char *b,
>> +	       char *c,
>> +	       size_t d,
>> +	       LOOKUP_TYPE *e,
>> +	       char *f)
> 
> here too
> 

Fixed


>> +#define CHECK_MERGE(err, status)	\
>> +do {					\
>> +  if (err)				\
>> +    {					\
>> +      __set_errno (err);		\
>> +      if (err == ERANGE)		\
>> +          status = NSS_STATUS_TRYAGAIN;	\
>> +      else				\
>> +          status = NSS_STATUS_UNAVAIL;	\
>> +      break;				\
>> +    }					\
>> +} while(0)
> 
> almost there ;).  8 leading spaces -> 1 tab, too many indents for status
> assignment, and a space before the ( in the "while (0)".
> 

Fixed, I think. Mixing tabs and spaces hurts my brain.


>> +	  if (status == NSS_STATUS_SUCCESS)
>> +	    {
>> +		/* The previous loop saved a buffer for merging.
>> +		   Perform the merge now.  */
>> +		err = MERGE_FN (&mergegrp, mergebuf, endptr, buflen, resbuf,
>> +				buffer);
>> +		CHECK_MERGE (err,status);
>> +		do_merge = 0;
>> +	    }
> 
> indentation here is broken too -- 6 spaces, not 1 tab
> 

Fixed


>> +	  else
>> +	    {
>> +	      /* If the result wasn't SUCCESS, copy the saved buffer back
>> +	         into the result buffer and set the status back to
>> +	         NSS_STATUS_SUCCESS to match the previous pass through the loop.
>> +	          * If the next action is CONTINUE, it will overwrite the value
>> +	            currently in the buffer and return the new value.
>> +	          * If the next action is RETURN, we'll return the previously-
>> +	            acquired values.
>> +	          * If the next action is MERGE, then it will be added to the buffer
>> +	            saved from the previous source.  */
> 
> some of these lines are too long

Fixed

> 
>> +	  if (!mergebuf)
> 
> GNU style says to test values rather than rely on implicit bool.  so this
> would be:
> 	if (mergebuf == NULL)
> 
> this comes up quite a bit in this patch
> 

I see it only here, where else do you see it?


>> +  free(mergebuf);
> 
> space before the (
> 

Fixed

>> --- a/nss/getnssent_r.c
>> +++ b/nss/getnssent_r.c
>>
>> +	{
>> +	  /* This is a special-case. When [SUCCESS=merge] is in play,
> 
> two spaces after the .

Fixed.


> 
>> +	     _nss_next2() will skip to the next database.  Due to the
>> +	     implementation of that function, we can't know whether we're
>> +	     in an enumeration or an individual lookup, which behaves
>> +	     differently with regards to merging.  We'll treat SUCCESS as
>> +	     an indication to start the enumeration at this database.
>> +	   */
> 
> the trailing */ should be on the previous line.
> 

Fixed

>> +      else
>> +	{
>> +	  no_more = __nss_next2 (nip, func_name, NULL, &fct.ptr, status, 0);
>> +	}
> 
> could elide the braces
> 

I am aware, but my personal preference is to always have braces even for
single-line IF/ELSE blocks (helps avoid merge issues like the famous Apple GOTO
bug).


>> +	  if (status == NSS_STATUS_SUCCESS
>> +	      && nss_next_action (*nip, status) == NSS_ACTION_MERGE)
>> +	    {
>> +	      /* This is a special-case. When [SUCCESS=merge] is in play,
> 
> two spaces after the .
> 

Fixed.

>> +	         _nss_next2() will skip to the next database.  Due to the
>> +	         implementation of that function, we can't know whether we're
>> +	         in an enumeration or an individual lookup, which behaves
>> +	         differently with regards to merging.  We'll treat SUCCESS as
>> +	         an indication to return the results here.
>> +	       */
> 
> cuddle up the */
> 

Fixed.


>> +	  else
>> +	    {
>> +	      no_more = __nss_next2 (nip, getent_func_name, NULL, &fct.ptr,
>> +				     status, 0);
>> +	    }
> 
> could elide the braces
> -mike
> 


See above.

Attachment: signature.asc
Description: OpenPGP digital signature