From 64fdbff43b467c5cd1bbd8f059e70e1b359e585d Mon Sep 17 00:00:00 2001
From: Jonathan G Rennison <j.g.rennison@gmail.com>
Date: Mon, 10 Jun 2024 18:18:18 +0100
Subject: [PATCH] Remove use of non-threadsafe strerror

Add helper class to use strerror_r or strerror_s
---
 src/music/extmidi.cpp               |  2 +-
 src/network/core/os_abstraction.cpp |  9 +--------
 src/os/unix/crashlog_unix.cpp       |  2 +-
 src/string.cpp                      | 29 +++++++++++++++++++++++++++++
 src/string_func.h                   | 13 +++++++++++++
 5 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/src/music/extmidi.cpp b/src/music/extmidi.cpp
index e12440bf7e..9be861e273 100644
--- a/src/music/extmidi.cpp
+++ b/src/music/extmidi.cpp
@@ -125,7 +125,7 @@ void MusicDriver_ExtMidi::DoPlay()
 		}
 
 		case -1:
-			DEBUG(driver, 0, "extmidi: couldn't fork: %s", strerror(errno));
+			DEBUG(driver, 0, "extmidi: couldn't fork: %s", StrErrorDumper().GetLast());
 			[[fallthrough]];
 
 		default:
diff --git a/src/network/core/os_abstraction.cpp b/src/network/core/os_abstraction.cpp
index a461b436cc..060a4d60b6 100644
--- a/src/network/core/os_abstraction.cpp
+++ b/src/network/core/os_abstraction.cpp
@@ -90,14 +90,7 @@ const char *NetworkError::AsString() const
 			this->message.assign(FS2OTTD(buffer));
 		}
 #else
-		/* Make strerror thread safe by locking access to it. There is a thread safe strerror_r, however
-		 * the non-POSIX variant is available due to defining _GNU_SOURCE meaning it is not portable.
-		 * The problem with the non-POSIX variant is that it does not necessarily fill the buffer with
-		 * the error message but can also return a pointer to a static bit of memory, whereas the POSIX
-		 * variant always fills the buffer. This makes the behaviour too erratic to work with. */
-		static std::mutex mutex;
-		std::lock_guard<std::mutex> guard(mutex);
-		this->message.assign(strerror(this->error));
+		this->message.assign(StrErrorDumper().Get(this->error));
 #endif
 	}
 	return this->message.c_str();
diff --git a/src/os/unix/crashlog_unix.cpp b/src/os/unix/crashlog_unix.cpp
index fdede6c94a..3af564ea0d 100644
--- a/src/os/unix/crashlog_unix.cpp
+++ b/src/os/unix/crashlog_unix.cpp
@@ -275,7 +275,7 @@ class CrashLogUnix : public CrashLog {
 	{
 		struct utsname name;
 		if (uname(&name) < 0) {
-			return buffer + seprintf(buffer, last, "Could not get OS version: %s\n", strerror(errno));
+			return buffer + seprintf(buffer, last, "Could not get OS version: %s\n", StrErrorDumper().GetLast());
 		}
 
 		return buffer + seprintf(buffer, last,
diff --git a/src/string.cpp b/src/string.cpp
index 19ef869d1b..e2ad09c967 100644
--- a/src/string.cpp
+++ b/src/string.cpp
@@ -1392,3 +1392,32 @@ public:
 #endif /* defined(WITH_COCOA) && !defined(STRGEN) && !defined(SETTINGSGEN) */
 
 #endif
+
+const char *StrErrorDumper::Get(int errornum)
+{
+#if defined(_WIN32)
+	if (strerror_s(this->buf, lengthof(this->buf), errornum) == 0) {
+		return this->buf;
+	}
+#else
+	auto result = strerror_r(errornum, this->buf, lengthof(this->buf));
+	static_assert(std::is_same_v<decltype(result), char *> || std::is_same_v<decltype(result), int>);
+	if constexpr (std::is_same_v<decltype(result), char *>) {
+		/* GNU-specific */
+		return result;
+	} else {
+		/* XSI-compliant */
+		if (result == 0) {
+			return this->buf;
+		}
+	}
+#endif
+
+	seprintf(this->buf, lastof(this->buf), "Unknown error %d", errornum);
+	return this->buf;
+}
+
+const char *StrErrorDumper::GetLast()
+{
+	return this->Get(errno);
+}
diff --git a/src/string_func.h b/src/string_func.h
index 87522c0eb7..9b0c405ca4 100644
--- a/src/string_func.h
+++ b/src/string_func.h
@@ -306,4 +306,17 @@ inline bool IsWhitespace(char32_t c)
 char *strcasestr(const char *haystack, const char *needle);
 #endif /* strcasestr is available */
 
+/**
+ * The use of a struct is so that when used as an argument to seprintf/etc, the buffer lives
+ * on the stack with a lifetime which lasts until the end of the statement.
+ * This avoids using a static buffer which is thread-unsafe, or needing to call malloc, which would then nee to be freed.
+ */
+struct StrErrorDumper {
+	const char *Get(int errornum);
+	const char *GetLast();
+
+private:
+	char buf[128];
+};
+
 #endif /* STRING_FUNC_H */