Use thread-safe strerror_r() to get error message (facebook#8087)

Summary: `strerror()` is not thread-safe, using `strerror_r()` instead. The API could be different on the different platforms, used the code from https://github.com/facebook/folly/blob/0deef031cb8aab76dc7e736f8b7c22d701d5f36b/folly/String.cpp#L457 Pull Request resolved: facebook#8087 Reviewed By: mrambacher Differential Revision: D27267151 Pulled By: jay-zhuang fbshipit-source-id: 4b8856d1ec069d5f239b764750682c56e5be9ddb
gonzus · Mar 25, 2021 · 45c65d6 · 45c65d6
1 parent f06b761
commit 45c65d6
Show file tree

Hide file tree

Showing 15 changed files with 118 additions and 20 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -1,4 +1,8 @@
 # Rocksdb Change Log
+## Unreleased
+### Bug Fixes
+* Use thread-safe `strerror_r()` to get error messages.
+
 ## 6.19.0 (03/21/2021)
 ### Bug Fixes
 * Fixed the truncation error found in APIs/tools when dumping block-based SST files in a human-readable format. After fix, the block-based table can be fully dumped as a readable file.

diff --git a/db/db_wal_test.cc b/db/db_wal_test.cc
@@ -1862,7 +1862,8 @@ TEST_F(DBWALTest, TruncateLastLogAfterRecoverWithoutFlush) {
   close(fd);
   ASSERT_OK(options.env->DeleteFile(fname_test_fallocate));
   if (err_number == ENOSYS || err_number == EOPNOTSUPP) {
-    fprintf(stderr, "Skipped preallocated space check: %s\n", strerror(err_number));
+    fprintf(stderr, "Skipped preallocated space check: %s\n",
+            errnoStr(err_number).c_str());
     return;
   }
   ASSERT_EQ(0, alloc_status);

diff --git a/env/env_chroot.cc b/env/env_chroot.cc
@@ -19,6 +19,7 @@
 #include "env/composite_env_wrapper.h"
 #include "rocksdb/file_system.h"
 #include "rocksdb/status.h"
+#include "util/string_util.h"
 
 namespace ROCKSDB_NAMESPACE {
 namespace {
@@ -333,7 +334,7 @@ class ChrootFileSystem : public FileSystemWrapper {
     char* normalized_path = realpath(res.second.c_str(), nullptr);
 #endif
     if (normalized_path == nullptr) {
-      res.first = IOStatus::NotFound(res.second, strerror(errno));
+      res.first = IOStatus::NotFound(res.second, errnoStr(errno).c_str());
     } else if (strlen(normalized_path) < chroot_dir_.size() ||
                strncmp(normalized_path, chroot_dir_.c_str(),
                        chroot_dir_.size()) != 0) {

diff --git a/env/env_hdfs.cc b/env/env_hdfs.cc
@@ -37,10 +37,10 @@ namespace {
 // Log error message
 static Status IOError(const std::string& context, int err_number) {
   return (err_number == ENOSPC)
-             ? Status::NoSpace(context, strerror(err_number))
+             ? Status::NoSpace(context, errnoStr(err_number).c_str())
              : (err_number == ENOENT)
-                   ? Status::PathNotFound(context, strerror(err_number))
-                   : Status::IOError(context, strerror(err_number));
+                   ? Status::PathNotFound(context, errnoStr(err_number).c_str())
+                   : Status::IOError(context, errnoStr(err_number).c_str());
 }
 
 // assume that there is one global logger for now. It is not thread-safe,

diff --git a/env/env_posix.cc b/env/env_posix.cc
@@ -315,7 +315,7 @@ class PosixEnv : public CompositeEnv {
     int ret = gethostname(name, static_cast<size_t>(len));
     if (ret < 0) {
       if (errno == EFAULT || errno == EINVAL) {
-        return Status::InvalidArgument(strerror(errno));
+        return Status::InvalidArgument(errnoStr(errno).c_str());
       } else {
         return IOError("GetHostName", name, errno);
       }

diff --git a/env/env_test.cc b/env/env_test.cc
@@ -915,7 +915,7 @@ class IoctlFriendlyTmpdir {
       } else {
         // mkdtemp failed: diagnose it, but don't give up.
         fprintf(stderr, "mkdtemp(%s/...) failed: %s\n", d.c_str(),
-                strerror(errno));
+                errnoStr(errno).c_str());
       }
     }
 
@@ -1040,7 +1040,8 @@ TEST_P(EnvPosixTestWithParam, AllocateTest) {
     int err_number = 0;
     if (alloc_status != 0) {
       err_number = errno;
-      fprintf(stderr, "Warning: fallocate() fails, %s\n", strerror(err_number));
+      fprintf(stderr, "Warning: fallocate() fails, %s\n",
+              errnoStr(err_number).c_str());
     }
     close(fd);
     ASSERT_OK(env_->DeleteFile(fname_test_fallocate));

diff --git a/env/fs_posix.cc b/env/fs_posix.cc
@@ -862,7 +862,7 @@ class PosixFileSystem : public FileSystem {
     char the_path[256];
     char* ret = getcwd(the_path, 256);
     if (ret == nullptr) {
-      return IOStatus::IOError(strerror(errno));
+      return IOStatus::IOError(errnoStr(errno).c_str());
     }
 
     *output_path = ret;

diff --git a/env/io_posix.cc b/env/io_posix.cc
@@ -58,18 +58,18 @@ IOStatus IOError(const std::string& context, const std::string& file_name,
   switch (err_number) {
     case ENOSPC: {
       IOStatus s = IOStatus::NoSpace(IOErrorMsg(context, file_name),
-                                     strerror(err_number));
+                                     errnoStr(err_number).c_str());
       s.SetRetryable(true);
       return s;
     }
     case ESTALE:
       return IOStatus::IOError(IOStatus::kStaleFile);
     case ENOENT:
       return IOStatus::PathNotFound(IOErrorMsg(context, file_name),
-                                    strerror(err_number));
+                                    errnoStr(err_number).c_str());
     default:
       return IOStatus::IOError(IOErrorMsg(context, file_name),
-                               strerror(err_number));
+                               errnoStr(err_number).c_str());
   }
 }
 
@@ -927,7 +927,7 @@ IOStatus PosixMmapFile::MapNewRegion() {
     }
     if (alloc_status != 0) {
       return IOStatus::IOError("Error allocating space to file : " + filename_ +
-                               "Error : " + strerror(alloc_status));
+                               "Error : " + errnoStr(alloc_status).c_str());
     }
   }
 

diff --git a/memory/arena.cc b/memory/arena.cc
@@ -12,11 +12,13 @@
 #include <sys/mman.h>
 #endif
 #include <algorithm>
+
 #include "logging/logging.h"
 #include "port/malloc.h"
 #include "port/port.h"
 #include "rocksdb/env.h"
 #include "test_util/sync_point.h"
+#include "util/string_util.h"
 
 namespace ROCKSDB_NAMESPACE {
 
@@ -170,7 +172,7 @@ char* Arena::AllocateAligned(size_t bytes, size_t huge_page_size,
     if (addr == nullptr) {
       ROCKS_LOG_WARN(logger,
                      "AllocateAligned fail to allocate huge TLB pages: %s",
-                     strerror(errno));
+                     errnoStr(errno).c_str());
       // fail back to malloc
     } else {
       return addr;

diff --git a/port/port_posix.cc b/port/port_posix.cc
@@ -23,8 +23,11 @@
 #include <sys/resource.h>
 #include <sys/time.h>
 #include <unistd.h>
+
 #include <cstdlib>
 
+#include "util/string_util.h"
+
 namespace ROCKSDB_NAMESPACE {
 
 // We want to give users opportunity to default all the mutexes to adaptive if
@@ -45,7 +48,7 @@ namespace port {
 
 static int PthreadCall(const char* label, int result) {
   if (result != 0 && result != ETIMEDOUT) {
-    fprintf(stderr, "pthread %s: %s\n", label, strerror(result));
+    fprintf(stderr, "pthread %s: %s\n", label, errnoStr(result).c_str());
     abort();
   }
   return result;

diff --git a/port/win/env_win.cc b/port/win/env_win.cc
@@ -35,6 +35,7 @@
 #include "rocksdb/env.h"
 #include "rocksdb/slice.h"
 #include "strsafe.h"
+#include "util/string_util.h"
 
 // Undefine the functions  windows might use (again)...
 #undef GetCurrentTime
@@ -61,7 +62,8 @@ typedef std::unique_ptr<void, decltype(FindCloseFunc)> UniqueFindClosePtr;
 
 void WinthreadCall(const char* label, std::error_code result) {
   if (0 != result.value()) {
-    fprintf(stderr, "pthread %s: %s\n", label, strerror(result.value()));
+    fprintf(stderr, "Winthread %s: %s\n", label,
+            errnoStr(result.value()).c_str());
     abort();
   }
 }

diff --git a/port/win/io_win.h b/port/win/io_win.h
@@ -17,6 +17,7 @@
 #include "rocksdb/file_system.h"
 #include "rocksdb/status.h"
 #include "util/aligned_buffer.h"
+#include "util/string_util.h"
 
 namespace ROCKSDB_NAMESPACE {
 namespace port {
@@ -37,10 +38,11 @@ inline IOStatus IOErrorFromLastWindowsError(const std::string& context) {
 
 inline IOStatus IOError(const std::string& context, int err_number) {
   return (err_number == ENOSPC)
-             ? IOStatus::NoSpace(context, strerror(err_number))
+             ? IOStatus::NoSpace(context, errnoStr(err_number).c_str())
              : (err_number == ENOENT)
-                   ? IOStatus::PathNotFound(context, strerror(err_number))
-                   : IOStatus::IOError(context, strerror(err_number));
+                   ? IOStatus::PathNotFound(context,
+                                            errnoStr(err_number).c_str())
+                   : IOStatus::IOError(context, errnoStr(err_number).c_str());
 }
 
 class WinFileData;

diff --git a/util/string_util.cc b/util/string_util.cc
@@ -19,6 +19,20 @@
 #include "port/sys_time.h"
 #include "rocksdb/slice.h"
 
+#ifndef __has_cpp_attribute
+#define ROCKSDB_HAS_CPP_ATTRIBUTE(x) 0
+#else
+#define ROCKSDB_HAS_CPP_ATTRIBUTE(x) __has_cpp_attribute(x)
+#endif
+
+#if ROCKSDB_HAS_CPP_ATTRIBUTE(maybe_unused) && __cplusplus >= 201703L
+#define ROCKSDB_MAYBE_UNUSED [[maybe_unused]]
+#elif ROCKSDB_HAS_CPP_ATTRIBUTE(gnu::unused) || __GNUC__
+#define ROCKSDB_MAYBE_UNUSED [[gnu::unused]]
+#else
+#define ROCKSDB_MAYBE_UNUSED
+#endif
+
 namespace ROCKSDB_NAMESPACE {
 
 const std::string kNullptrString = "nullptr";
@@ -419,4 +433,66 @@ bool SerializeIntVector(const std::vector<int>& vec, std::string* value) {
   return true;
 }
 
+// Copied from folly/string.cpp:
+// https://github.com/facebook/folly/blob/0deef031cb8aab76dc7e736f8b7c22d701d5f36b/folly/String.cpp#L457
+// There are two variants of `strerror_r` function, one returns
+// `int`, and another returns `char*`. Selecting proper version using
+// preprocessor macros portably is extremely hard.
+//
+// For example, on Android function signature depends on `__USE_GNU` and
+// `__ANDROID_API__` macros (https://git.io/fjBBE).
+//
+// So we are using C++ overloading trick: we pass a pointer of
+// `strerror_r` to `invoke_strerror_r` function, and C++ compiler
+// selects proper function.
+
+#if !(defined(_WIN32) && (defined(__MINGW32__) || defined(_MSC_VER)))
+ROCKSDB_MAYBE_UNUSED
+static std::string invoke_strerror_r(int (*strerror_r)(int, char*, size_t),
+                                     int err, char* buf, size_t buflen) {
+  // Using XSI-compatible strerror_r
+  int r = strerror_r(err, buf, buflen);
+
+  // OSX/FreeBSD use EINVAL and Linux uses -1 so just check for non-zero
+  if (r != 0) {
+    snprintf(buf, buflen, "Unknown error %d (strerror_r failed with error %d)",
+             err, errno);
+  }
+  return buf;
+}
+
+ROCKSDB_MAYBE_UNUSED
+static std::string invoke_strerror_r(char* (*strerror_r)(int, char*, size_t),
+                                     int err, char* buf, size_t buflen) {
+  // Using GNU strerror_r
+  return strerror_r(err, buf, buflen);
+}
+#endif  // !(defined(_WIN32) && (defined(__MINGW32__) || defined(_MSC_VER)))
+
+std::string errnoStr(int err) {
+  char buf[1024];
+  buf[0] = '\0';
+
+  std::string result;
+
+  // https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/strerror_r.3.html
+  // http://www.kernel.org/doc/man-pages/online/pages/man3/strerror.3.html
+#if defined(_WIN32) && (defined(__MINGW32__) || defined(_MSC_VER))
+  // mingw64 has no strerror_r, but Windows has strerror_s, which C11 added
+  // as well. So maybe we should use this across all platforms (together
+  // with strerrorlen_s). Note strerror_r and _s have swapped args.
+  int r = strerror_s(buf, sizeof(buf), err);
+  if (r != 0) {
+    snprintf(buf, sizeof(buf),
+             "Unknown error %d (strerror_r failed with error %d)", err, errno);
+  }
+  result.assign(buf);
+#else
+  // Using any strerror_r
+  result.assign(invoke_strerror_r(strerror_r, err, buf, sizeof(buf)));
+#endif
+
+  return result;
+}
+
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/util/string_util.h b/util/string_util.h
@@ -141,4 +141,8 @@ bool SerializeIntVector(const std::vector<int>& vec, std::string* value);
 
 extern const std::string kNullptrString;
 
+// errnoStr() function returns a string that describes the error code passed in
+// the argument err
+extern std::string errnoStr(int err);
+
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/util/threadpool_imp.cc b/util/threadpool_imp.cc
@@ -19,6 +19,7 @@
 #endif
 
 #include <stdlib.h>
+
 #include <algorithm>
 #include <atomic>
 #include <condition_variable>
@@ -31,12 +32,13 @@
 #include "monitoring/thread_status_util.h"
 #include "port/port.h"
 #include "test_util/sync_point.h"
+#include "util/string_util.h"
 
 namespace ROCKSDB_NAMESPACE {
 
 void ThreadPoolImpl::PthreadCall(const char* label, int result) {
   if (result != 0) {
-    fprintf(stderr, "pthread %s: %s\n", label, strerror(result));
+    fprintf(stderr, "pthread %s: %s\n", label, errnoStr(result).c_str());
     abort();
   }
 }