From 46971c6dbfbc39ebbc74ab1ed8c00edc12859373 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Wed, 20 Apr 2022 16:17:19 +0200 Subject: [PATCH 1/5] util: Replace non-threadsafe strerror Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in #4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives and replace all uses of `strerror` with this. --- src/Makefile.am | 3 +++ src/bitcoind.cpp | 3 ++- src/fs.cpp | 3 ++- src/init.cpp | 3 ++- src/util/sock.cpp | 16 +++------------- src/util/syserror.cpp | 29 +++++++++++++++++++++++++++++ src/util/syserror.h | 16 ++++++++++++++++ src/util/system.cpp | 3 ++- 8 files changed, 59 insertions(+), 17 deletions(-) create mode 100644 src/util/syserror.cpp create mode 100644 src/util/syserror.h diff --git a/src/Makefile.am b/src/Makefile.am index 476ff0a6c5a..8c259290cb2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -271,6 +271,7 @@ BITCOIN_CORE_H = \ util/spanparsing.h \ util/string.h \ util/syscall_sandbox.h \ + util/syserror.h \ util/system.h \ util/thread.h \ util/threadnames.h \ @@ -631,6 +632,7 @@ libbitcoin_util_a_SOURCES = \ util/getuniquepath.cpp \ util/hasher.cpp \ util/sock.cpp \ + util/syserror.cpp \ util/system.cpp \ util/message.cpp \ util/moneystr.cpp \ @@ -853,6 +855,7 @@ bitcoin_chainstate_SOURCES = \ util/settings.cpp \ util/strencodings.cpp \ util/syscall_sandbox.cpp \ + util/syserror.cpp \ util/system.cpp \ util/thread.cpp \ util/threadnames.cpp \ diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 98433826825..bc063faed18 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -206,7 +207,7 @@ static bool AppInit(NodeContext& node, int argc, char* argv[]) } break; case -1: // Error happened. - return InitError(Untranslated(strprintf("fork_daemon() failed: %s\n", strerror(errno)))); + return InitError(Untranslated(strprintf("fork_daemon() failed: %s\n", SysErrorString(errno)))); default: { // Parent: wait and exit. int token = daemon_ep.TokenRead(); if (token) { // Success diff --git a/src/fs.cpp b/src/fs.cpp index 219fdee959a..b61115bf014 100644 --- a/src/fs.cpp +++ b/src/fs.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #ifndef WIN32 #include @@ -44,7 +45,7 @@ fs::path AbsPathJoin(const fs::path& base, const fs::path& path) static std::string GetErrorReason() { - return std::strerror(errno); + return SysErrorString(errno); } FileLock::FileLock(const fs::path& file) diff --git a/src/init.cpp b/src/init.cpp index cccb088eec7..06a7b10f7f0 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -65,6 +65,7 @@ #include #include #include +#include #include #include #include @@ -149,7 +150,7 @@ static fs::path GetPidFile(const ArgsManager& args) #endif return true; } else { - return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), std::strerror(errno))); + return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno))); } } diff --git a/src/util/sock.cpp b/src/util/sock.cpp index b5c1e282941..3579af44585 100644 --- a/src/util/sock.cpp +++ b/src/util/sock.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -344,19 +345,8 @@ std::string NetworkErrorString(int err) #else std::string NetworkErrorString(int err) { - char buf[256]; - buf[0] = 0; - /* Too bad there are two incompatible implementations of the - * thread-safe strerror. */ - const char *s; -#ifdef STRERROR_R_CHAR_P /* GNU variant can return a pointer outside the passed buffer */ - s = strerror_r(err, buf, sizeof(buf)); -#else /* POSIX variant always returns message in buffer */ - s = buf; - if (strerror_r(err, buf, sizeof(buf))) - buf[0] = 0; -#endif - return strprintf("%s (%d)", s, err); + // On BSD sockets implementations, NetworkErrorString is the same as SysErrorString. + return SysErrorString(err); } #endif diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp new file mode 100644 index 00000000000..bcd249200d4 --- /dev/null +++ b/src/util/syserror.cpp @@ -0,0 +1,29 @@ +// Copyright (c) 2020-2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#if defined(HAVE_CONFIG_H) +#include +#endif + +#include +#include + +#include + +std::string SysErrorString(int err) +{ + char buf[256]; + buf[0] = 0; + /* Too bad there are two incompatible implementations of the + * thread-safe strerror. */ + const char *s; +#ifdef STRERROR_R_CHAR_P /* GNU variant can return a pointer outside the passed buffer */ + s = strerror_r(err, buf, sizeof(buf)); +#else /* POSIX variant always returns message in buffer */ + s = buf; + if (strerror_r(err, buf, sizeof(buf))) + buf[0] = 0; +#endif + return strprintf("%s (%d)", s, err); +} diff --git a/src/util/syserror.h b/src/util/syserror.h new file mode 100644 index 00000000000..a54ba553eee --- /dev/null +++ b/src/util/syserror.h @@ -0,0 +1,16 @@ +// Copyright (c) 2010-2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_UTIL_SYSERROR_H +#define BITCOIN_UTIL_SYSERROR_H + +#include + +/** Return system error string from errno value. Use this instead of + * std::strerror, which is not thread-safe. For network errors use + * NetworkErrorString from sock.h instead. + */ +std::string SysErrorString(int err); + +#endif // BITCOIN_UTIL_SYSERROR_H diff --git a/src/util/system.cpp b/src/util/system.cpp index f9a9ad3e209..6845e815eda 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -1374,7 +1375,7 @@ void ScheduleBatchPriority() const static sched_param param{}; const int rc = pthread_setschedparam(pthread_self(), SCHED_BATCH, ¶m); if (rc != 0) { - LogPrintf("Failed to pthread_setschedparam: %s\n", strerror(rc)); + LogPrintf("Failed to pthread_setschedparam: %s\n", SysErrorString(rc)); } #endif } From e7f2f77756d33c6be9c8998a575b263ff2d39270 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Wed, 20 Apr 2022 16:43:07 +0200 Subject: [PATCH 2/5] util: Use strerror_s for SysErrorString on Windows --- src/util/syserror.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp index bcd249200d4..20f89057fc4 100644 --- a/src/util/syserror.cpp +++ b/src/util/syserror.cpp @@ -15,15 +15,21 @@ std::string SysErrorString(int err) { char buf[256]; buf[0] = 0; - /* Too bad there are two incompatible implementations of the + /* Too bad there are three incompatible implementations of the * thread-safe strerror. */ const char *s; +#ifdef WIN32 + s = buf; + if (strerror_s(buf, sizeof(buf), err) != 0) + buf[0] = 0; +#else #ifdef STRERROR_R_CHAR_P /* GNU variant can return a pointer outside the passed buffer */ s = strerror_r(err, buf, sizeof(buf)); #else /* POSIX variant always returns message in buffer */ s = buf; if (strerror_r(err, buf, sizeof(buf))) buf[0] = 0; +#endif #endif return strprintf("%s (%d)", s, err); } From 718da302c7b11b375042c3000d421fd93348c199 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Wed, 20 Apr 2022 19:41:30 +0200 Subject: [PATCH 3/5] util: Refactor SysErrorString logic Deduplicate code and error checks by making sure `s` stays `nullptr` in case of error. Return "Unknown error" instead of an empty string in this case. --- src/util/syserror.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp index 20f89057fc4..d721602bbbd 100644 --- a/src/util/syserror.cpp +++ b/src/util/syserror.cpp @@ -14,22 +14,21 @@ std::string SysErrorString(int err) { char buf[256]; - buf[0] = 0; /* Too bad there are three incompatible implementations of the * thread-safe strerror. */ - const char *s; + const char *s = nullptr; #ifdef WIN32 - s = buf; - if (strerror_s(buf, sizeof(buf), err) != 0) - buf[0] = 0; + if (strerror_s(buf, sizeof(buf), err) == 0) s = buf; #else #ifdef STRERROR_R_CHAR_P /* GNU variant can return a pointer outside the passed buffer */ s = strerror_r(err, buf, sizeof(buf)); #else /* POSIX variant always returns message in buffer */ - s = buf; - if (strerror_r(err, buf, sizeof(buf))) - buf[0] = 0; + if (strerror_r(err, buf, sizeof(buf)) == 0) s = buf; #endif #endif - return strprintf("%s (%d)", s, err); + if (s != nullptr) { + return strprintf("%s (%d)", s, err); + } else { + return strprintf("Unknown error (%d)", err); + } } From f00fb1265a8bc26e1612c771173325dbe49b3612 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Thu, 21 Apr 2022 18:39:56 +0200 Subject: [PATCH 4/5] util: Increase buffer size to 1024 in SysErrorString Increase the error message buffer to 1024 as recommended in the manual page (Thanks Jon Atack) --- src/util/syserror.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp index d721602bbbd..391ddd35602 100644 --- a/src/util/syserror.cpp +++ b/src/util/syserror.cpp @@ -13,7 +13,7 @@ std::string SysErrorString(int err) { - char buf[256]; + char buf[1024]; /* Too bad there are three incompatible implementations of the * thread-safe strerror. */ const char *s = nullptr; From e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Thu, 28 Apr 2022 09:55:45 +0200 Subject: [PATCH 5/5] test: Add `strerror` to locale-dependence linter Add `strerror` to the locale-dependence linter to catch its use. Add exemptions for bdb interface code (false positive) and strerror.cpp (the only allowed use). Also fix a bug in the regexp so that `_r` and `_s` variants are detected again. --- test/lint/lint-locale-dependence.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/lint/lint-locale-dependence.py b/test/lint/lint-locale-dependence.py index 2abf1be6b33..9b2cf4587a3 100755 --- a/test/lint/lint-locale-dependence.py +++ b/test/lint/lint-locale-dependence.py @@ -49,7 +49,9 @@ KNOWN_VIOLATIONS = [ "src/test/fuzz/locale.cpp:.*setlocale", "src/test/fuzz/string.cpp:.*strtol", "src/test/fuzz/string.cpp:.*strtoul", - "src/test/util_tests.cpp:.*strtoll" + "src/test/util_tests.cpp:.*strtoll", + "src/wallet/bdb.cpp:.*DbEnv::strerror", # False positive + "src/util/syserror.cpp:.*strerror", # Outside this function use `SysErrorString` ] REGEXP_EXTERNAL_DEPENDENCIES_EXCLUSIONS = [ @@ -144,7 +146,7 @@ LOCALE_DEPENDENT_FUNCTIONS = [ "strcasecmp", "strcasestr", "strcoll", # LC_COLLATE - #"strerror", + "strerror", "strfmon", "strftime", # LC_TIME "strncasecmp", @@ -218,7 +220,7 @@ LOCALE_DEPENDENT_FUNCTIONS = [ def find_locale_dependent_function_uses(): regexp_locale_dependent_functions = "|".join(LOCALE_DEPENDENT_FUNCTIONS) exclude_args = [":(exclude)" + excl for excl in REGEXP_EXTERNAL_DEPENDENCIES_EXCLUSIONS] - git_grep_command = ["git", "grep", "-E", "[^a-zA-Z0-9_\\`'\"<>](" + regexp_locale_dependent_functions + "(_r|_s)?)[^a-zA-Z0-9_\\`'\"<>]", "--", "*.cpp", "*.h"] + exclude_args + git_grep_command = ["git", "grep", "-E", "[^a-zA-Z0-9_\\`'\"<>](" + regexp_locale_dependent_functions + ")(_r|_s)?[^a-zA-Z0-9_\\`'\"<>]", "--", "*.cpp", "*.h"] + exclude_args git_grep_output = list() try: