[Bug 278556] strerror-related race condition and standards violation in printf and friends

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278556

            Bug ID: 278556
           Summary: strerror-related race condition and standards
                    violation in printf and friends
           Product: Base System
           Version: Unspecified
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Many People
          Priority: ---
         Component: standards
          Assignee: standards@FreeBSD.org
          Reporter: jonathan.gruber.jg@gmail.com

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.