Overview: I was reading the implementation of __vfprintf, the function that appears to be main logic for printf and friends, in the file lib/libc/stdio/vfprintf.c in the main branch of the freebsd-src repo. I noticed that, for the %m format specifier, the __vfprintf function obtains the errno message by calling strerror. FreeBSD's implementation of strerror, in the file lib/libc/string/strerror.c in the same branch of same repo, is not thread-safe, since it uses a non-thread-local static buffer for the errno message and returns a pointer to that buffer, so the offending segment in __vfprintf is not thread-safe. Additionally, the offending segment violates both the ISO C standard and POSIX, which both mandate that a conforming implementation behave as if no library functions (including printf and friends) call strerror. Evidently, under FreeBSD's implementation of libc, if a program has a string s returned by strerror and subsequently calls printf (which indirectly calls __vfprintf) with a format string containing the %m format specifier, then the call to printf will possibly modify the string s. Hence, FreeBSD's libc implementation does indeed behave as if some library functions (printf and friends) call strerror. I also noticed that vfprintf_l, in the same file as __vfprintf, has a comment above it reading "MT-safe version" but that vfprintf_l calls __vfprintf, which has a comment above it reading "Non-MT-safe version". I cannot find any other version of __vfprintf, so I assume that vfprintf_l is calling the __vfprintf function defined in the same file. Is the "Non-MT safe version" comment above __vfprintf referring to its use of non-thread-safe functions such as strerrror, or something else? Furthermore, why would vfprintf_l, which is ostensibly thread-safe according to the aforementioned comment above it, call __vfprintf, which is ostensibly non-thread-safe according to the aforementioned comment above it? Steps to Reproduce: I do not run FreeBSD, as I was merely browsing the source code, but I would suggest the following code segment to check for the standards violation: const char *s = strerror(ENOENT); printf("s: %s\n", s); errno = ELOOP; // set errno to a different error number than the one supplied to strerror above printf("%m\n"); printf("s: %s\n", s); Actual Results: I do not run FreeBSD, but my reading of the source code leads me to believe that the bug which I identify here is indeed present. Expected Results: __vfprintf should not call strerror, so that printf and friends will conform to the ISO C standard and POSIX regarding usage of strerror. __vfprintf should also be thread-safe, since ostensibly thread-safe functions like vfprintf_l call it (or, at the very least, functions like vfprintf_l that call __vfprintf should do so in a thread-safe manner). Build Date & Hardware: Not exactly applicable, but the offending segment of __vfprintf is in the main branch of the freebsd-src repo. Additional Builds and Platforms: Not applicable. Additional Information: None. Suggested solution: The offending segment in __vfprintf could easily be fixed by stack-allocating a char buffer of size NL_TEXTMAX in the offending segment and storing the errno message in that buffer via strerror_r, which is thread-safe and would not unexpectedly modify any external data. A size of NL_TEXTMAX for the buffer ensures that the buffer can store any errno message, given that strerror itself uses a buffer of this size for the errno message. I have not read the whole of the __vfprintf function, so I do not know if fixing the strerror situation as above would automatically also make __vfprintf thread-safe or if it would make all present usages of __vfprintf in the source code thread-safe.
I came to the same suggested solution after only reading the first half of your excellent bug report, so I agree with the suggested solution. ML_TEXTMAX is huge, but not really anymore (2k). The following leverages off of the buffer we already need to allocate for digit accumulation: diff --git a/lib/libc/stdio/vfprintf.c b/lib/libc/stdio/vfprintf.c index 6c7c6982c8dc..622cb57f5e7d 100644 --- a/lib/libc/stdio/vfprintf.c +++ b/lib/libc/stdio/vfprintf.c @@ -44,7 +44,7 @@ */ #include "namespace.h" -#include <sys/types.h> +#include <sys/param.h> #include <ctype.h> #include <errno.h> @@ -354,7 +354,7 @@ __vfprintf(FILE *fp, locale_t locale, const char *fmt0, va_list ap) int prsize; /* max size of printed field */ const char *xdigs; /* digits for %[xX] conversion */ struct io_state io; /* I/O buffering state */ - char buf[BUF]; /* buffer with space for digits of uintmax_t */ + char buf[MAX(BUF, NL_TEXTMAX)]; /* buffer with space for digits of uintmax_t or errno string */ char ox[2]; /* space for 0x; ox[1] is either x, X, or \0 */ union arg *argtable; /* args, built due to positional arg */ union arg statargtable [STATIC_ARG_TBL_SIZE]; @@ -829,7 +829,9 @@ reswitch: switch (ch) { break; #endif /* !NO_FLOATING_POINT */ case 'm': - cp = strerror(saved_errno); + if (strerror_r(saved_errno, buf, sizeof(buf)) != 0) + strlcpy(buf, "error not available", sizeof(buf)); + cp = buf; size = (prec >= 0) ? strnlen(cp, prec) : strlen(cp); sign = '\0'; break;
btw, my change is minimal. it could easily just #define BUF to be NL_TEXTMAX and work, though it might have more cache misses given the 2k span of the buffer... we could get rid of the MAX and always do NL_TEXTMAX and use either sizeof or define BUF to be something like (sizeof(uintmax_t) / 3 + 2) which will always be enough and 2k can handle integers up to 6k bits, so we'd future proof the code.
I believe the 'non-MT-safe version' refer to using the global locale in strerror() etc. From this PoV, use of strerror_r() is still wrong. Also, there is at least one more usage of strerror() in the printf() machinery. I put my proposal at https://reviews.freebsd.org/D44916
I'd missed the subtle difference we needed since printf is also supposed to support the current locale, not just the global one... I like the code in the review...
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=f887667694632c829b0599b54ff86a072e93df87 commit f887667694632c829b0599b54ff86a072e93df87 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2024-04-23 17:10:30 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-04-23 19:43:01 +0000 __vprintf(): switch from strerror() to strerror_rl() This eliminates the use of non-thread-safe function in printf*() family, and make the call locale-aware. Also, it stops obliterating the strerror() static buffer, which aligns with the POSIX requirement that implementations must behave as if no standard-mandated functions call strerror(). PR: 278556 Reported by: Jonathan Gruber <jonathan.gruber.jg@gmail.com> Reviewed by: imp Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D44916 lib/libc/stdio/vfprintf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=ade62d406239b728a1c74974bbb57ed493c733e0 commit ade62d406239b728a1c74974bbb57ed493c733e0 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2024-04-23 17:10:30 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-04-30 00:48:10 +0000 __vprintf(): switch from strerror() to strerror_rl() PR: 278556 (cherry picked from commit f887667694632c829b0599b54ff86a072e93df87) lib/libc/stdio/vfprintf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=27e54c9f162879fcbf6f1d9dc221b98c8180f012 commit 27e54c9f162879fcbf6f1d9dc221b98c8180f012 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2024-04-23 17:10:30 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-04-30 00:49:53 +0000 __vprintf(): switch from strerror() to strerror_rl() PR: 278556 (cherry picked from commit f887667694632c829b0599b54ff86a072e93df87) lib/libc/stdio/vfprintf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)