getusershell(3): Don't write past the end of line buffer reading local shells

[kevans]

Summary

_local_initshells does not currently reset cp to the beginning of the line buffer
for every iteration that it calls fgets(3), leading to writing past the end of
line with fairly long /etc/shells or excessively long line lengths. Correct this
by properly resetting cp.

Aside: I had considered instead doing fgets((cp = line), MAXPATHLEN + 1, ... and removing the
initialization of cp prior to the loop and not resetting it within the loop. However, I don't see
such expression usage in other parts that I've dealt with -- would this have been an
acceptable alternative?

PR: 192528

Test Plan

Execute pwd_mkdb -p /etc/passwd with some of the previously failing
example /etc/shells as well as known-good /etc/shells. In my experience, it complains
pretty happily if something goes wrong.

Also double-check kyua tests, although I don't know whether any of the libc/gen tests
would break as a result.

[cem]

Aside: I had considered instead doing fgets((cp = line), MAXPATHLEN + 1, ... and removing the initialization of cp prior to the loop and not resetting it within the loop. However, I don't see such expression usage in other parts that I've dealt with -- would this have been an acceptable alternative?

I don't know if it's explicitly unacceptable, but at least I don't like that construct :-). Simple expressions are best.

You could do while (fgets(line, ...) ...) {, and add cp = line; once at the top of the loop body, instead of repeating it twice? Minor difference and arguably no better.

[kevans]

In D10690#221656, @cem wrote:

Aside: I had considered instead doing fgets((cp = line), MAXPATHLEN + 1, ... and removing the initialization of cp prior to the loop and not resetting it within the loop. However, I don't see such expression usage in other parts that I've dealt with -- would this have been an acceptable alternative?

I don't know if it's explicitly unacceptable, but at least I don't like that construct :-). Simple expressions are best.

Fair. =) It left a somewhat bad taste in my mouth.

You could do while (fgets(line, ...) ...) {, and add cp = line; once at the top of the loop body, instead of repeating it twice? Minor difference and arguably no better.

Indeed, although that might be preferable now that you mention it so that you're not relying on the value of cp to not clobber the stack and it offers a little more clarity.

[kevans]

• Less invasive version; use line directly for fgets(), reset cp at the start of every iteration

[cem]

lib/libc/gen/getusershell.c

127 ↗

(On Diff #28269)

Could even leave this line unchanged if you wanted to.

[jilles]

lib/libc/gen/getusershell.c

127 ↗

(On Diff #28269)

Static analyzers will complain about a dead store if you leave the ++ in place, so please keep the removal.