[PATCH] libpwdgrp: fix relying on unknown errno state

[Harald van Dijk]

On 05/03/2025 09:29, Adam Goldman wrote:
> On Tue, Feb 25, 2025 at 03:35:47PM +0200, Baruch Burstein wrote:
>> To give some context:
>> At loginutils/login.c:502 `is_tty_secure()` is called. This function
>> (at libbb/securetty.c:10) attempts to open "/etc/securetty". If the file is
>> not found this is not an error (see comment there), but it does set errno.
>> A few lines later, login.c calls `ask_and_check_password()`, which
>> eventually reaches `massage_data_for_r_func()`, which returns a failure if
>> errno is set at the end of the function.
> 
> So the problem (which your patch fixes) is that if errno is nonzero at
> entry to massage_data_for_r_func, the RETURN VALUE of
> massage_data_for_r_func is wrong, because massage_data_for_r_func ends
> in "return errno;". But a library function should avoid clearing errno.
> It might be better to have it return 0 and leave errno alone.

There is precedent for functions requiring errno to be cleared: it is 
the only way to detect errors when using the standard library function 
strtol(). This applies even if strtol() is used inside a library.

The way massage_data_for_r_func() currently is meant to work is that it 
returns nonzero on any error. It calls convert_to_struct() which reports 
errors by setting errno, and that calls bb_strtol() which also reports 
errors by setting errno. Assuming it makes sense for bb_strtol() to 
match the standard library function strtol(), there is no way around 
setting errno to zero. That is the only way to detect out-of-range 
values. The other error handling could be changed to check things 
differently, but that would come at the expense of no longer detecting 
some errors that in my opinion should continue to be detected.

I think the patch is correct and should be applied as is.

Cheers,
Harald van Dijk