Re: [PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

["Maciej W. Rozycki" <macro@mips.com>]

On Mon, 18 Jun 2018, DJ Delorie wrote:

> "Maciej W. Rozycki" <macro@mips.com> writes:
> > -  if (len == 0 && numstr[len - 1] != '\0')
> > +  if (len == 0 || numstr[len - 1] != '\0')
> 
> I think this part of the patch(es) is good, but a second part might be
> needed to avoid future problems[*]:
> 
> +    if (len > 0)
>        strncpy (first_unused, numstr, len);

 Do you expect GCC to start complaining here sometime even if it cannot 
statically prove that `len' can sometimes be zero, which seems the case 
here?

 It would have to always complain then if it found `strncpy' called with 
the the length argument variable.  And I think that while calling 
`strncpy' with constant zero length might be silly (though not invalid!), 
it certainly is fine to call it with a variable length argument that is 
sometimes zero, especially if the likelihood of it being zero (or more 
generally, just the context of the call) does not justify avoiding the 
call in that case.  This also seems to be the case here, being the 
unlikely error path.

> It looks like that clause handles two cases (if not, ignore the rest of
> this email ;)...
> 
> 1. if we don't have a valid entry, create a zero-length temporary
>    entry.
> 
> 2. If the entry is too long to be nul-terminated (or otherwise isn't
>    nul-terminated), create a temporary nul-terminated one.
> 
> The strncpy is only needed for the second case.  Since we already know
> the length, and are going to nul-terminate it ourselves anyway, a memcpy
> would work just as well (but still need the above if-check), unless the
> nis server is allowed to return an entry with an embedded nul, in which
> case strncpy might prevent data leakage from whatever's after the nul.
> I would consider that a different kind of bug, though, and we're calling
> strtoul on the result anyway.
> 
> However, for the first case, we're always producing a string that causes
> the following "if (numstr[0] == '\0')" test to be true, so the only real
> purpose of going through all that code for zero-length strings is to
> make sure the buffer has room for the trailing nul :-P  (and we need the
> test for other cases anyway).

 Yes, and I wouldn't have written it like this if I were to create this 
piece of code from scratch, however...

> I don't think that's worth changing the code for, though.  Maybe a
> comment clarifying why it's the way it is, if we care that much ;-)

... as I noted in the discussion, this is legacy code, so I chose the path 
of least resistance by reusing correct code already present elsewhere 
(that we don't need to touch), because uniformity has a value too.  We 
could optimise it, rewrite for clarity, etc. across all the instances.  
But as you write, is it worth it?

 I'm not sure even if it's worth adding a comment here: if unsure, then 
run `git blame' and examine the relevant commit description.

  Maciej