[PATCH v2] misc: Optimize getusershell.c

[Collin Funk]

Rocket Ma <marocketbd@gmail.com> writes:

> * misc/getusershell.c: Completely rewrite the unit. Only allocate one
> big buffer to store shell names. Add a missing unit test.
>
> The new implementation read the whole file into one buffer, and wipe out
> every byte but shell names. Later when addressing shell names from first
> shell, jump to next '\0' and then jump to next '/'. This could reduce
> memory footprint and shall improve some performance.
>
> Signed-off-by: Rocket Ma <marocketbd@gmail.com>
> ---
> This new patch fixed some unexpected plt links, and use isspace_l to
> force parsing /etc/shells with C locale. (Is that necessary?)

Thanks for the patch.

However, I am against this change. The /etc/shells file can be
arbitrarily long, so we shouldn't read it all into memory. It also
changes the behavior of the function to be different to all the BSD
versions. In this version appending a shell while the process is running
will not affect subsequent calls to getusershell.

> +  if ((fp = fopen (_PATH_SHELLS, "rce")) == NULL)
> +    goto default_out;
> +  if ((__fstat64_time64 (__fileno (fp), &fstat)) == -1)
> +    goto close_out;
> +  /* Consider if buflen will overflow. */
> +  if (fstat.st_size < 2 || fstat.st_size > PTRDIFF_MAX - 1)
> +    goto close_out;
> +  /* 1 byte for \n (will be overwritten as \0). */
> +  buflen = fstat.st_size + 1;
> +  if ((shellbuf = malloc (buflen)) == NULL)
> +    goto close_out;
> +  shellbuf[buflen - 1] = '\n';
> +  _IO_setbuf (fp, NULL);
> +  if ((_IO_fread (shellbuf, 1, fstat.st_size, fp)) != fstat.st_size)
> +    goto free_out;

What if we call fstat while a user is in the middle of adding a shell?
E.g., then it is possible we return a truncated shell because we didn't
allocate enough memory?

Collin