Make system_error::message() thread safe. Fixes PR25598.

[EricWF]

Summary

system_error::message() uses strerror for the generic and system categories. This function is not thread safe.

The fix is to use strerror_r. It has been available since 2001 for GNU libc and since BSD 4.4 on FreeBSD/OS X.
On platforms with GNU libc the extended version is used which always returns a valid string, even if an error occurs.

In single-threaded builds strerror is still used.

See https://llvm.org/bugs/show_bug.cgi?id=25598

[mclow.lists]

In general, I'm OK with this - but I'm concerned about that there's not really any provision for the case where strerror_r does not exist.

Also, there's no reason to have a thread local static array here, if you're going to immediately copy it into a std::string.

[erik65536]

The C++ spec states that error_category::message() shall not change the value of errno (See section 19.5). So errno will have to be saved and restored if strerror_r() fails.

The POSIX version of strerror_r() returns 0 on success, and any other value indicates an error (Reference).

The function may return uninitialized memory if std::snprintf() fails.

You may want to a version for Windows that calls strerror_s().

[EricWF]

In D20903#446922, @mclow.lists wrote:

In general, I'm OK with this - but I'm concerned about that there's not really any provision for the case where strerror_r does not exist.

Also, there's no reason to have a thread local static array here, if you're going to immediately copy it into a std::string.

Ack. I changed the arrays to be neither static nor thread_local.

In D20903#447650, @erik65536 wrote:

The C++ spec states that error_category::message() shall not change the value of errno (See section 19.5). So errno will have to be saved and restored if strerror_r() fails.

Good catch. I only have to do this for the POSIX version. I'll make the change.

The POSIX version of strerror_r() returns 0 on success, and any other value indicates an error (Reference).

The function may return uninitialized memory if std::snprintf() fails.

I don't see how snprintf could possible fail.

You may want to a version for Windows that calls strerror_s().

Libc++ provides no windows support.

[EricWF]

• Use a local buffer instead of a static thread_local one.
• Save and restore errno in the POSIX implementation.

[mclow.lists]

LGTM.

[EricWF]

Use named variable strerror_buff_size variable instead of literal 1024.

[erik65536]

The POSIX version of strerror_r() returns 0 on success, and any other value indicates an error (Reference).

I should have been more explicit when I wrote this recommendation. Checking if strerror_r returns -1 is not portable. For example, OSX and FreeBSD return an error code from strerror_r without setting errno. The POSIX version should check for success by checking to see if strerror_r returns 0. If the function fails and the return value is not -1, assume that the return value is an error code.

[EricWF]

In D20903#457252, @erik65536 wrote:

The POSIX version of strerror_r() returns 0 on success, and any other value indicates an error (Reference).

I should have been more explicit when I wrote this recommendation. Checking if strerror_r returns -1 is not portable. For example, OSX and FreeBSD return an error code from strerror_r without setting errno. The POSIX version should check for success by checking to see if strerror_r returns 0. If the function fails and the return value is not -1, assume that the return value is an error code.

Urg IDK how I missed that in the docs. Thanks for pointing this out.

I fixed it in r272640. I'll follow up with some tests for the FreeBSD and OS X implementations.

[EricWF]

I checked in tests for the reported bug in r272642. They aren't the most portable so hopefully the pass on all supported platforms.

[erik65536]

I don't see any other issues. Thanks for fixing this.

src/system_error.cpp

 //===---------------------- system_error.cpp ------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "__config"
 
 #define _LIBCPP_BUILDING_SYSTEM_ERROR
 #include "system_error"
 
 #include "include/config_elast.h"
+#include "cerrno"
 #include "cstring"
+#include "cstdio"
+#include "cstdlib"
+#include "cassert"
 #include "string"
+#include "string.h"
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // class error_category
 
 error_category::error_category() _NOEXCEPT
 {
 }

 }
 
 bool
 error_category::equivalent(const error_code& code, int condition) const _NOEXCEPT
 {
     return *this == code.category() && code.value() == condition;
 }
 
+namespace {
+
+//  GLIBC also uses 1024 as the maximum buffer size internally.
+constexpr size_t strerror_buff_size = 1024;
+
+string do_strerror_r(int ev);
+
+#if defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC)
+// GNU Extended version
+string do_strerror_r(int ev) {
+    char buffer[strerror_buff_size];
+    char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
+    return string(ret);
+}
+#else
+// POSIX version
+string do_strerror_r(int ev) {
+    char buffer[strerror_buff_size];
+    const int old_errno = errno;
+    if (::strerror_r(ev, buffer, strerror_buff_size) == -1) {
+        const int new_errno = errno;
+        errno = old_errno;
+        if (new_errno == EINVAL) {
+            std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
+            return string(buffer);
+        } else {
+            assert(new_errno == ERANGE);
+            // FIXME maybe? 'strerror_buff_size' is likely to exceed the
+            // maximum error size so ERANGE shouldn't be returned.
+            std::abort();
+        }
+    }
+    return string(buffer);
+}
+#endif
+
+} // end namespace
+
 string
 __do_message::message(int ev) const
 {
-    return string(strerror(ev));
+#if defined(_LIBCPP_HAS_NO_THREADS)
+    return string(::strerror(ev));
+#else
+    return do_strerror_r(ev);
+#endif
 }
 
 class _LIBCPP_HIDDEN __generic_error_category
     : public __do_message
 {
 public:
     virtual const char* name() const _NOEXCEPT;
     virtual string message(int ev) const;

test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/generic_category.pass.cpp

 
 // class error_category
 
 // const error_category& generic_category();
 
 #include <system_error>
 #include <cassert>
 #include <string>
+#include <cerrno>
+
+void test_message_leaves_errno_unchanged() {
+    errno = E2BIG; // something that message will never generate
+    const std::error_category& e_cat1 = std::generic_category();
+    e_cat1.message(-1);
+    assert(errno == E2BIG);
+}
 
 int main()
 {
     const std::error_category& e_cat1 = std::generic_category();
     std::string m1 = e_cat1.name();
     assert(m1 == "generic");
+    {
+        test_message_leaves_errno_unchanged();
+    }
 }

test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp

 
 // class error_category
 
 // const error_category& system_category();
 
 #include <system_error>
 #include <cassert>
 #include <string>
+#include <cerrno>
+
+
+void test_message_leaves_errno_unchanged() {
+    errno = E2BIG; // something that message will never generate
+    const std::error_category& e_cat1 = std::system_category();
+    e_cat1.message(-1);
+    assert(errno == E2BIG);
+}
 
 int main()
 {
     const std::error_category& e_cat1 = std::system_category();
     std::error_condition e_cond = e_cat1.default_error_condition(5);
     assert(e_cond.value() == 5);
     assert(e_cond.category() == std::generic_category());
     e_cond = e_cat1.default_error_condition(5000);
     assert(e_cond.value() == 5000);
     assert(e_cond.category() == std::system_category());
+    {
+        test_message_leaves_errno_unchanged();
+    }
 }