feature: replace strerror with thread safe version( strerror_r ) #4590

aliihsancengiz · 2025-12-24T13:16:54Z

Closes #4568

Related: #4568

What's new?

Replace strerror with thread safe version of strerror_r

How to test

Checklist

Tests added and pass
Adequate documentation added
(optional) Added Screenshots or videos

AlanGriffiths

Related: #4586

How is this related?

AlanGriffiths · 2026-01-05T10:06:28Z

include/common/mir/errno_utils.h

+    namespace detail {
+
+        // POSIX strerror_r returns int, GNU returns char*.
+        // This overload set normalizes both to std::string.
+        inline std::string strerror_r_to_string(int rc, const char* buf) {
+            // POSIX: rc == 0 on success. On failure, buf may contain something,
+            // but still provide a deterministic message.
+            if (rc == 0 && buf && *buf) return std::string(buf);
+            return "Unknown error (" + std::to_string(rc) + ")";
+        }
+
+        inline std::string strerror_r_to_string(const char* msg, const char* /*buf*/) {
+            // GNU: returns pointer to message (may or may not be buf).
+            if (msg && *msg) return std::string(msg);
+            return "Unknown error";
+        }
+
+
+        // This overload set normalizes both to char*
+        inline const char* strerror_r_to_cstr(int /*ret*/, const char* buf) noexcept {
+            return buf; // POSIX
+        }
+
+        inline const char* strerror_r_to_cstr(char* ret, const char* /*buf*/) noexcept {
+            return ret ? ret : "Unknown error"; // GNU
+        }
+
+
+    } // namespace detail


I think this detail would be better encapsulated in a .cpp file

Looks like a typo

AlanGriffiths · 2026-01-05T10:14:59Z

include/common/mir/errno_utils.h

+    // Thread-safe errno to string conversion.
+    inline std::string errno_to_string(int err) {
+        thread_local char buf[256] = {};
+
+        // Call strerror_r and normalize both GNU/POSIX return types.
+        auto ret = ::strerror_r(err, buf, sizeof(buf));
+        return detail::strerror_r_to_string(ret, buf);
+    }


Do we need this? I can see some uses, but errno_to_cstr() seems adequate in all these cases.

That was my initial opinion, but I think this uses stack memory after returning. See this comment

Ok, after Alan clearing up thread_local to me. I agree with this, we can just use the cstr version and wrap in in std::string{...} in all the places that require it.

AlanGriffiths · 2026-01-05T10:15:22Z

include/common/mir/errno_utils.h

+    inline const char* errno_to_cstr(int err) noexcept {
+        thread_local char buf[256] = {};
+
+        // Call strerror_r and normalize both GNU/POSIX return types.
+        auto ret = ::strerror_r(err, buf, sizeof(buf));
+        return detail::strerror_r_to_cstr(ret, buf);
+    }


The implementation would be better encapsulated in a .cpp file

AlanGriffiths · 2026-01-05T10:21:59Z

include/common/mir/errno_utils.h

+#ifndef MIR_ERRNO_UTILS_H_
+#define MIR_ERRNO_UTILS_H_
+
+#include <cerrno>


Why do we include <cerrno>?

tarek-y-ismail · 2026-01-05T09:17:41Z

examples/mir_demo_server/server_example_test_client.cpp

 */

 #include "server_example_test_client.h"
+#include "mir/errno_utils.h"


Suggested change

#include "mir/errno_utils.h"

#include <mir/errno_utils.h>

This occurs with every inclusion of this header. I won't point it out in every file since it would be too annoying.
Checking the style guide, we forgot to update it to point this out. Opened #4597 to fix this.

tarek-y-ismail · 2026-01-05T09:19:06Z

include/common/mir/errno_utils.h

+
+namespace mir
+{
+    namespace detail {


I believe we don't follow the detail pattern in the codebase. Can you split the implementation details into their own source file?

tarek-y-ismail · 2026-01-05T09:29:40Z

tests/unit-tests/CMakeLists.txt

  test_lockable_callback.cpp
  test_module_deleter.cpp
-    test_posix_rw_mutex.cpp
+  test_posix_rw_mutex.cpp


While this is a correct fix. I'm not sure if we should sneak it in with this PR

include/common/mir/errno_utils.h

aliihsancengiz · 2026-01-05T19:37:33Z

Hey @AlanGriffiths and @tarek-y-ismail , thanks for the review.
I have rebased and addressed comments. Please find the first iteration.

Got rid of errno_to_string
Splitted implementation into .cpp file
Some include tweaking
exported mir::errno_to_cstr*; for symbol map, i did this manually, but not sure that's the right way.

Thanks.

AlanGriffiths · 2026-01-06T10:28:51Z

src/server/console/linux_virtual_terminal.cpp

+#include <mir/fd.h>
+#include <mir/emergency_cleanup_registry.h>
+#include "ioctl_vt_switcher.h"
+#include <mir/errno_utils.h>


Suggested change

#include <mir/fd.h>

#include <mir/emergency_cleanup_registry.h>

#include "ioctl_vt_switcher.h"

#include <mir/errno_utils.h>

#include <mir/errno_utils.h>

AlanGriffiths · 2026-01-06T10:33:30Z

src/common/symbols.map

    mir::dispatch::ThreadedDispatcher::remove_thread*;
    mir::dispatch::epoll_to_fd_event*;
    mir::dispatch::fd_event_to_epoll*;
+    mir::errno_to_cstr*;


This is in the wrong stanza (the symbol didn't exist in Mir-2.22).

You need to add it in a new MIR_COMMON_2.26 stanza. Vis:

MIR_COMMON_2.26 { global: extern "C++" { mir::errno_to_cstr*; }; } MIR_COMMON_INTERNAL_2.24;

(That may look confusing, but there's a pre-existing issue with abusing "_INTERNAL_". We can't address that without breaking ABI.)

AlanGriffiths · 2026-01-06T10:35:52Z

tests/unit-tests/test_errno_utils.cpp

+
+#include <mir/errno_utils.h>
+
+TEST(ErrnoToCstr, ReturnsNonNullAndNonEmpty) {


Project style is snake_case for test names and braces opened on the next line.

Suggested change

TEST(ErrnoToCstr, ReturnsNonNullAndNonEmpty) {

TEST(ErrnoToCstr, returns_non_null_and_non_empty)

{

tests/unit-tests/test_errno_utils.cpp

AlanGriffiths · 2026-01-06T16:25:10Z

src/server/console/linux_virtual_terminal.cpp

+#include <mir/fd.h>
+#include <mir/emergency_cleanup_registry.h>
+#include "ioctl_vt_switcher.h"


That was not intended; indeed, it reverted.

AlanGriffiths · 2026-01-06T16:26:02Z

tests/unit-tests/test_errno_utils.cpp

+#include <thread>
+
+#include <mir/errno_utils.h>
+TEST(ErrnoToCstr, returns_non_null_and_non_empty) {


Project style is to have the opening brace on the next line

AlanGriffiths · 2026-01-06T16:57:21Z

include/common/mir/errno_utils.h

+// This overload set normalizes both to char*
+// inline const char* strerror_r_to_cstr(int ret, const char* buf) noexcept;
+// inline const char* strerror_r_to_cstr(char* ret, const char* buf) noexcept;
+


Confusing comment

Indeed, a leftover from clean-up, Thanks.

AlanGriffiths

A couple of style tweaks

AlanGriffiths · 2026-01-07T09:46:33Z

include/common/mir/errno_utils.h

+#ifndef MIR_ERRNO_UTILS_H_
+#define MIR_ERRNO_UTILS_H_
+
+#include <string>


src/common/errno_utils.cpp

AlanGriffiths

A couple of style tweaks

Pushed these to avoid another review cycle

Copilot

Pull request overview

This PR replaces the thread-unsafe strerror function with the thread-safe strerror_r across the codebase, addressing issue #4568. The implementation provides a wrapper function mir::errno_to_cstr that handles differences between GNU and POSIX versions of strerror_r.

Key changes:

Introduces mir::errno_to_cstr() as a thread-safe replacement for strerror()
Adds comprehensive unit tests for thread safety and correctness
Updates all occurrences of strerror() throughout the codebase to use the new wrapper

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
include/common/mir/errno_utils.h	Declares the thread-safe errno conversion function
src/common/errno_utils.cpp	Implements thread-safe errno-to-string conversion using strerror_r with GNU/POSIX compatibility
tests/unit-tests/test_errno_utils.cpp	Adds unit tests verifying thread safety and correctness of errno_to_cstr
tests/unit-tests/CMakeLists.txt	Registers the new test file in the build system
src/common/CMakeLists.txt	Adds errno_utils.cpp to the build
[multiple platform/server files]	Replaces strerror calls with mir::errno_to_cstr for thread safety

include/common/mir/errno_utils.h

src/common/errno_utils.cpp

jhenstridge

The use of thread_local means the thread safety guarantees are similar to those for plain strerror in modern glibc (although now the return value will always be overwritten on a second call to errno_to_cstr on the same thread).

If we're doing this to improve safety, would a std::string return be more appropriate?

jhenstridge · 2026-01-07T10:28:09Z

src/common/errno_utils.cpp

+// Since we are normalizing between GNU/POSIX impl, one of function will be unused at compile time
+
+[[maybe_unused]]
+inline const char* strerror_r_to_cstr(int /*ret*/, const char* buf) noexcept


If ret is not zero, then buf could contain garbage. It should probably return "Unknown error" in this case, similar to what you've done in the GNU version.

jhenstridge · 2026-01-07T10:30:55Z

src/common/errno_utils.cpp

+    auto ret = ::strerror_r(err, buf, sizeof(buf));
+    return strerror_r_to_cstr(ret, buf);


Is it really worth adding support for two versions of strerror_r? It basically means we'll have a code path that is unused for most of our testing.

Given that this code is in it's own source file now, we know what environment it is being compiled in. It would probably be simpler to put #undef _GNU_SOURCE before the includes, and use an assert to ensure we have the POSIX version. Something like:

static_assert(std::is_same_v<decltype(ret), int>);

That would be a better aproach indeed.
But I am not sure undefining _GNU_SOURCE will force the compiler to use a POSIX/XSI-compliant version.

At least, when I apply it, my compiler is unable to find the POSIX version.

gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0 g++ (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0 ldd (Ubuntu GLIBC 2.39-0ubuntu8.6) 2.39

Maybe i am missing something?

AlanGriffiths · 2026-01-08T12:08:25Z

The use of thread_local means the thread safety guarantees are similar to those for plain strerror in modern glibc

We don't know we're on glibc. For example, Alpine uses musl

aliihsancengiz requested a review from a team as a code owner December 24, 2025 13:16

aliihsancengiz force-pushed the make-errno-to-string-thread-safe branch 3 times, most recently from 7db9e59 to 873b35f Compare December 27, 2025 18:51

AlanGriffiths requested changes Jan 5, 2026

View reviewed changes

tarek-y-ismail requested changes Jan 5, 2026

View reviewed changes

aliihsancengiz force-pushed the make-errno-to-string-thread-safe branch from 0f2832a to b624643 Compare January 5, 2026 19:29

aliihsancengiz requested review from AlanGriffiths and tarek-y-ismail January 5, 2026 19:37

AlanGriffiths requested changes Jan 6, 2026

View reviewed changes

aliihsancengiz force-pushed the make-errno-to-string-thread-safe branch from 1a2cd3b to ae187fc Compare January 6, 2026 16:15

aliihsancengiz requested a review from AlanGriffiths January 6, 2026 16:16

AlanGriffiths requested changes Jan 6, 2026

View reviewed changes

aliihsancengiz force-pushed the make-errno-to-string-thread-safe branch from ae187fc to addb3f2 Compare January 6, 2026 16:51

AlanGriffiths requested changes Jan 6, 2026

View reviewed changes

aliihsancengiz force-pushed the make-errno-to-string-thread-safe branch from addb3f2 to deddc16 Compare January 6, 2026 18:27

aliihsancengiz requested a review from AlanGriffiths January 6, 2026 18:28

AlanGriffiths requested changes Jan 7, 2026

View reviewed changes

AlanGriffiths approved these changes Jan 7, 2026

View reviewed changes

AlanGriffiths requested a review from Copilot January 7, 2026 10:03

Copilot AI reviewed Jan 7, 2026

View reviewed changes

include/common/mir/errno_utils.h Outdated Show resolved Hide resolved

src/common/errno_utils.cpp Outdated Show resolved Hide resolved

jhenstridge reviewed Jan 7, 2026

View reviewed changes

feature: replace strerror with thread safe version( strerror_r )

ec3755f

aliihsancengiz force-pushed the make-errno-to-string-thread-safe branch from 1027a37 to ec3755f Compare January 8, 2026 20:14


		#include <mir/errno_utils.h>

		TEST(ErrnoToCstr, ReturnsNonNullAndNonEmpty) {

	TEST(ErrnoToCstr, ReturnsNonNullAndNonEmpty) {
	TEST(ErrnoToCstr, returns_non_null_and_non_empty)
	{

		auto ret = ::strerror_r(err, buf, sizeof(buf));
		return strerror_r_to_cstr(ret, buf);

feature: replace strerror with thread safe version( strerror_r ) #4590

Are you sure you want to change the base?

feature: replace strerror with thread safe version( strerror_r ) #4590

Conversation

aliihsancengiz commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's new?

How to test

Checklist

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aliihsancengiz commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliihsancengiz commented Dec 24, 2025 •

edited

Loading

aliihsancengiz commented Jan 5, 2026 •

edited

Loading