fix integer underflow in getgrnam_r and getpwnam_r

[asomers]

Summary

fix integer underflow in getgrnam_r and getpwnam_r

Sometimes nscd(8) will return a 1-byte buffer for a nonexistent entry. This
triggered an integer underflow in grp_unmarshal_func, causing getgrnam_r to
return ERANGE instead of 0.

Fix the user's buffer size check, and add a correct check for a too-small
nscd buffer.

PR: 248932

Test Plan

Tested on 11.4-RELEASE with nscd and ldap. Could not reproduce
on head.

[asomers]

ping @pi @trasz . Are either of you able to review this change?

[trasz]

lib/libc/gen/getgrent.c

338 ↗

(On Diff #76261)

I don't quite understand this code, but... what's 'p', and aren't you using its uninitialized value?

[asomers]

lib/libc/gen/getgrent.c

338 ↗

(On Diff #76261)

You're right. I copied the _ALIGN(p) - (size_t)p from a few lines below, but p isn't initialized yet. Surprising that the compiler didn't warn about it.

[asomers]

Fix uninitialized value read in the first version of the diff

[markj]

lib/libc/gen/getgrent.c

337 ↗

(On Diff #77225)

Just for my understanding: orig_buf_size is the size of the buffer to which we're copying results, and buffer_size is the size of the cached data returned by nscd?

344 ↗

(On Diff #77225)

In nscd's on_read_request_process() I see that if the agent's lookup function returns NS_NOTFOUND, nscd seemingly writes negative_data, which is a single nul byte. Is that the source of these short reads? Doesn't getpwent() have the same problem?

344 ↗

(On Diff #77225)

Sorry, I missed that you also modified getpwent().

355 ↗

(On Diff #77225)

This brace should be on the preceding line. There are several instances of this in the diff.

[markj]

lib/libc/gen/getgrent.c

347 ↗

(On Diff #77225)

Should this be NS_NOTFOUND? Hard to say unless we know exactly why nscd sends small buffers.

[asomers]

lib/libc/gen/getgrent.c

337 ↗

(On Diff #77225)

Yes.

355 ↗

(On Diff #77225)

Fixed. Note that the brace on line 338 needs its own line; otherwise it overflows 80 columns.

[asomers]

Style changes

[asomers]

return NS_NOTFOUND instead of NS_UNAVAIL if nscd returns 1 byte

[markj]

Seems reasonable to me.

lib/libc/gen/getgrent.c

355 ↗

(On Diff #77225)

The usual way to handle that is to break the conditional, as is done in getpwent.c:392.

[asomers]

lib/libc/gen/getgrent.c

347 ↗

(On Diff #77225)

Probably so. At the very least, it's no more wrong than NS_UNAVAIL. I'll change it.