Merge bitcoin/bitcoin#24933: util: Replace non-threadsafe strerror

e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a8bc26e1612c771173325dbe49b3612 util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7b11b375042c3000d421fd93348c199 util: Refactor SysErrorString logic (laanwj)
e7f2f77756d33c6be9c8998a575b263ff2d39270 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbfbc39ebbc74ab1ed8c00edc12859373 util: Replace non-threadsafe strerror (laanwj)

Pull request description:

  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 (with code from `NetworkErrorString`) and replace all uses of `strerror` with this.

  Edit: I've also added a commit that refactors the code so that buf[] is never read at all if the function fails, making some fragile-looking code unnecessary.

  Edit2: from the linux manpage:
  ```
  ATTRIBUTES
         For an explanation of the terms used in this section, see attributes(7).

         ┌───────────────────┬───────────────┬─────────────────────────┐
         │Interface          │ Attribute     │ Value                   │
         ├───────────────────┼───────────────┼─────────────────────────┤
         │strerror()         │ Thread safety │ MT-Unsafe race:strerror │
         ├───────────────────┼───────────────┼─────────────────────────┤
  …
         ├───────────────────┼───────────────┼─────────────────────────┤
         │strerror_r(),      │ Thread safety │ MT-Safe                 │
         │strerror_l()       │               │                         │
         └───────────────────┴───────────────┴─────────────────────────┘
  ```
  As the function can be called from any thread at any time, using a non-thread-safe function is unacceptable.

ACKs for top commit:
  jonatack:
    ACK e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c

Tree-SHA512: 20e71ebb9e979d4e1d8cafbb2e32e20c2a63f09115fe72cdde67c8f80ae98c531d286f935fd8a6e92a18b72607d7bd3e846b2d871d9691a6036b0676de8aaf25

src/Makefile.am

@@ -275,6 +275,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 \
@@ -657,6 +658,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 \
@@ -918,6 +920,7 @@ libbitcoinkernel_la_SOURCES = \
   util/settings.cpp \
   util/strencodings.cpp \
   util/syscall_sandbox.cpp \
+  util/syserror.cpp \
   util/system.cpp \
   util/thread.cpp \
   util/threadnames.cpp \

src/bitcoind.cpp

@@ -20,6 +20,7 @@
 #include <util/check.h>
 #include <util/strencodings.h>
 #include <util/syscall_sandbox.h>
+#include <util/syserror.h>
 #include <util/system.h>
 #include <util/threadnames.h>
 #include <util/tokenpipe.h>
@@ -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

src/fs.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <fs.h>
+#include <util/syserror.h>
 
 #ifndef WIN32
 #include <cstring>
@@ -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)

src/init.cpp

@@ -65,6 +65,7 @@
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/syscall_sandbox.h>
+#include <util/syserror.h>
 #include <util/system.h>
 #include <util/thread.h>
 #include <util/threadnames.h>
@@ -150,7 +151,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)));
     }
 }
 

src/util/sock.cpp

@@ -7,6 +7,7 @@
 #include <threadinterrupt.h>
 #include <tinyformat.h>
 #include <util/sock.h>
+#include <util/syserror.h>
 #include <util/system.h>
 #include <util/time.h>
 
@@ -344,19 +345,8 @@ std::string NetworkErrorString(int err)
 #else
 std::string NetworkErrorString(int err)
 {
-    char buf[256];
+    // On BSD sockets implementations, NetworkErrorString is the same as SysErrorString.
-    buf[0] = 0;
+    return SysErrorString(err);
-    /* 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);
 }
 #endif
 

src/util/syserror.cpp

@@ -0,0 +1,34 @@
+// 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 <config/bitcoin-config.h>
+#endif
+
+#include <tinyformat.h>
+#include <util/syserror.h>
+
+#include <cstring>
+
+std::string SysErrorString(int err)
+{
+    char buf[1024];
+    /* Too bad there are three incompatible implementations of the
+     * thread-safe strerror. */
+    const char *s = nullptr;
+#ifdef WIN32
+    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 */
+    if (strerror_r(err, buf, sizeof(buf)) == 0) s = buf;
+#endif
+#endif
+    if (s != nullptr) {
+        return strprintf("%s (%d)", s, err);
+    } else {
+        return strprintf("Unknown error (%d)", err);
+    }
+}

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 <string>
+
+/** 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

src/util/system.cpp

@@ -25,6 +25,7 @@
 #include <util/getuniquepath.h>
 #include <util/strencodings.h>
 #include <util/string.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 
 
@@ -1374,7 +1375,7 @@ void ScheduleBatchPriority()
     const static sched_param param{};
     const int rc = pthread_setschedparam(pthread_self(), SCHED_BATCH, &param);
     if (rc != 0) {
-        LogPrintf("Failed to pthread_setschedparam: %s\n", strerror(rc));
+        LogPrintf("Failed to pthread_setschedparam: %s\n", SysErrorString(rc));
     }
 #endif
 }

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: