Introduce KMOD_STRERROR to ensure usage is safer.

[crrodriguez]

While recent strerror implementations are thread aware/safe, old ones are not, and the issue of shared library safety remains.

That's why strerrordesc_np() was added that is thread safe, async-signal-safe (does not call malloc) and there is no data race with other functions.

Use that if available, or %m ..else fallback to the closest that is calling strerror_r with a buffer whose scope expires inmediately.

This concerns only libkmod, in the future the toolchain may implement per-dso allocations to make the problem disappear in practice.

Note that strerrordesc_np does not display error in the current locale but only in english, this is an small price to pay for safety.

[evelikov]

Thanks for the MR. Do you have any references wrt the thread (un)safety of strerror() - ideally we'll have that as part of the commit message.

Looking at the patch itself:

• looks like we can move a lot more strerror() users to %m - ideally this will be a separate commit
• for the remaining ones, we should probably be using a wrapper in shared/missing.h which is used when the old/buggy implementation is detected

[crrodriguez]

No, strerror is thread safe now, it is not library safe though. The buffer is thread local might be invalidated by whatever caller libkmod has no control of, This needs per-dso storage to be really safe but it has not (yet) been implemented.

[crrodriguez]

https://sourceware.org/glibc/manual/latest/html_mono/libc.html#Error-Messages-1

"As there is no way to restore the previous state after calling strerror, library code should not call this function because it may interfere with application use of strerror, invalidating the string pointer before the application is done using it. Instead, strerror_r, snprintf with the ‘%m’ or ‘%#m’ specifiers, strerrorname_np, or strerrordesc_np can be used instead."

[evelikov]

Thanks for the correction. If I understood correctly, it sounds like it's better to use strerror_r throughout.

That said, the function comes in POSIX and GNU variants, which similar to basename we'd want the GNU variant. Kmod aims to support glibc, musl and bionic, so we might need a wrapper - see commit 11eb9bc how we handled that for basename.

Edit: theoretically we can try the C11 strerror_s, although it doesn't return the string like the POSIX strerror_r plus it seems like it's not implemented in glibc and musl... So the GNU strerror_r is the most suitable solution IMHO.

[lucasdemarchi]

IMO the changes to %m are good and could be done without any other changes.

Introducing a KMOD_STRERROR() is very ugly. strerror_r() would be good, but then there's the compat nightmare to care about. Honestly any application doing something like this is already broken:

s = strerror(...)
kmod_xyz()
printf("%s\n",  s);

The manpage for strerror() already mentions it:

This string must not be modified by the application, and the returned pointer will be
invalidated on a subsequent call to strerror() or strerror_l(), or if the thread that
obtained the string exits.

Since the caller and libkmod are sharing libc, the caller has no idea if libkmod is going to use that libc function. And if we didn't, some other library used by kmod could still screw it. I don't think there's any hope for an application to get it right other than stop mixing those calls.

If we do decide to use the safe implementation, then just defining STRERROR() without the KMOD_ prefix would be better. Then we can probably copy the relevant part in .github/codeql-queries/PotentiallyDangerousFunction.ql from systemd.

[evelikov]

I've fleshed out the strerror() -> %m changes plus addressed a few more instances with (#346.

Note that I won't be pursuing the remaining instances and/or introduction of STRERROR. HTH