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

[DJ Delorie <dj at redhat dot com>]

"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);

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).

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 ;-)


[*] GCC's analyzer is getting, if not better, at least more agressive.
    I expect the trend to continue.