From 45c65d6dcf66f3afa47dbec0d0786e4e133ae14c Mon Sep 17 00:00:00 2001 From: Jay Zhuang Date: Wed, 24 Mar 2021 23:06:31 -0700 Subject: [PATCH] Use thread-safe `strerror_r()` to get error message (#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: https://github.com/facebook/rocksdb/pull/8087 Reviewed By: mrambacher Differential Revision: D27267151 Pulled By: jay-zhuang fbshipit-source-id: 4b8856d1ec069d5f239b764750682c56e5be9ddb --- HISTORY.md | 4 +++ db/db_wal_test.cc | 3 +- env/env_chroot.cc | 3 +- env/env_hdfs.cc | 6 ++-- env/env_posix.cc | 2 +- env/env_test.cc | 5 +-- env/fs_posix.cc | 2 +- env/io_posix.cc | 8 ++--- memory/arena.cc | 4 ++- port/port_posix.cc | 5 ++- port/win/env_win.cc | 4 ++- port/win/io_win.h | 8 +++-- util/string_util.cc | 76 ++++++++++++++++++++++++++++++++++++++++++ util/string_util.h | 4 +++ util/threadpool_imp.cc | 4 ++- 15 files changed, 118 insertions(+), 20 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index e4ee4dbb2..bc327af13 100644 --- 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 index a729800ea..cc8c46335 100644 --- 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 index cd23fcabe..4575e98e0 100644 --- 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 index 77a14af9a..e0443dd94 100644 --- 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 index 98ac66ad4..fdcb6f6a3 100644 --- a/env/env_posix.cc +++ b/env/env_posix.cc @@ -315,7 +315,7 @@ class PosixEnv : public CompositeEnv { int ret = gethostname(name, static_cast(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 index d35a42f9b..1c4340b43 100644 --- 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 index 452131083..53d8781d8 100644 --- 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 index 97770d256..297b56794 100644 --- a/env/io_posix.cc +++ b/env/io_posix.cc @@ -58,7 +58,7 @@ 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; } @@ -66,10 +66,10 @@ IOStatus IOError(const std::string& context, const std::string& file_name, 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 index 5f8300448..bcdad5c76 100644 --- a/memory/arena.cc +++ b/memory/arena.cc @@ -12,11 +12,13 @@ #include #endif #include + #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 index 59fa6b89b..112984de2 100644 --- a/port/port_posix.cc +++ b/port/port_posix.cc @@ -23,8 +23,11 @@ #include #include #include + #include +#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 index 408a84aae..1c7aed2df 100644 --- 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 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 index d9a6bd774..4119f5add 100644 --- 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 index c44992f88..c5a7a7cae 100644 --- 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& 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 index 5ff516cac..83fa5781d 100644 --- a/util/string_util.h +++ b/util/string_util.h @@ -141,4 +141,8 @@ bool SerializeIntVector(const std::vector& 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 index dcaf288aa..b6a521714 100644 --- a/util/threadpool_imp.cc +++ b/util/threadpool_imp.cc @@ -19,6 +19,7 @@ #endif #include + #include #include #include @@ -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(); } }