[PATCH v2] misc: Optimize getusershell.c

[Adhemerval Zanella Netto]

On 22/04/26 14:01, Collin Funk wrote:
> 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.

The current code already read the whole file before parsing it, this
new approach is no worse.  The BSD code also seems to limit each
shell to PATH_MAX, while this approach seems to work in-place and
removes the extra buffer allocation.

> 
>> +  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?
I do not think we have this guarantee for current code, and I am not sure
if would be feasible without some extra support OS.