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: 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 (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index 476ff0a6c5..8c259290cb 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 9843382682..bc063faed1 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 219fdee959..b61115bf01 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 cccb088eec..06a7b10f7f 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 b5c1e28294..3579af4458 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 0000000000..bcd249200d --- /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 0000000000..a54ba553ee --- /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 f9a9ad3e20..6845e815ed 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 } -- cgit v1.2.3 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: util: Use strerror_s for SysErrorString on Windows --- src/util/syserror.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp index bcd249200d..20f89057fc 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); } -- cgit v1.2.3 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: 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(-) (limited to 'src') diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp index 20f89057fc..d721602bbb 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); + } } -- cgit v1.2.3 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: 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(-) (limited to 'src') diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp index d721602bbb..391ddd3560 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; -- cgit v1.2.3