On Wed, Mar 05, 2025 at 09:53:16AM +0000, Harald van Dijk wrote: > 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.
If a C standard library function, such as sscanf, clears errno before calling strtol(), POSIX requires the library function to restore the original errno value before returning. Busybox library functions aren't bound by the same requirement, but it's still a good idea. The direct rationale is that stomping on errno can obscure a previous error. The more general rationale is that making an unexpected change to a global variable can lead to bugs. > 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. Uh-oh... I now see that errno in massage_data_for_r_func can get set not only by the code in massage_data_for_r_func, but by convert_to_struct. However, both bb_strtou and bb_strtol set errno to 0 if they were successful (in contrast to the standard library function strtol, which leaves errno alone if it was successful). convert_to_struct will always call either bb_strtou or bb_strtol at least once. So this calls the original bug diagnosis into question: If massage_data_for_r_func calls convert_to_struct, and convert_to_struct will always set errno to 0 at some point, then why would the initial value of errno (before ask_and_check_password) matter? But there's another problem lurking in convert_to_struct: If there's a bogus passwd file entry, convert_to_struct is supposed to return NULL. As it parses each part of the entry, it sets errno if it's bad, but continues on to the rest of the entry. Each time it parses a numeric part of the entry, bb_strtou/bb_strtol will clear any errno value previously set by convert_to_struct! So convert_to_struct will fail to report the bad passwd file entry. -- Adam _______________________________________________ busybox mailing list [email protected] https://lists.busybox.net/mailman/listinfo/busybox
