From 9646dc324d10d11c5020e120fd732c3fee3e5257 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 29 Nov 2024 09:53:38 +0100 Subject: [PATCH 1/4] Make `getenv` thread-safe In order to return a non-owning pointer without memory leaks the function needs to use a static variable. When calling it from multiple threads there is a data race during the assignment (and conversion) to this variable. Fix by making it `thread_local`. Fixes #189 --- include/boost/nowide/cstdlib.hpp | 7 ++++++- src/cstdlib.cpp | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/include/boost/nowide/cstdlib.hpp b/include/boost/nowide/cstdlib.hpp index a48028a1..cfdac7cf 100644 --- a/include/boost/nowide/cstdlib.hpp +++ b/include/boost/nowide/cstdlib.hpp @@ -21,7 +21,12 @@ namespace nowide { /// /// \brief UTF-8 aware getenv. Returns 0 if the variable is not set. /// - /// This function is not thread safe or reenterable as defined by the standard library + /// The string pointed to shall not be modified by the program. + /// This function is thread-safe as long as no other thread modifies the host environment. + /// However subsequent calls to this function might overwrite the string pointed to. + /// + /// Warning: The returned pointer might only be valid for as long as the calling thread is alive. + /// So avoid passing it across thread boundaries. /// BOOST_NOWIDE_DECL char* getenv(const char* key); diff --git a/src/cstdlib.cpp b/src/cstdlib.cpp index ab0d4455..76f0319f 100644 --- a/src/cstdlib.cpp +++ b/src/cstdlib.cpp @@ -52,7 +52,7 @@ namespace boost { namespace nowide { char* getenv(const char* key) { - static stackstring value; + thread_local stackstring value; const wshort_stackstring name(key); From ff34275b81c330a1517c8b85321193712f02f6d1 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 29 Nov 2024 10:13:16 +0100 Subject: [PATCH 2/4] Update changelog --- doc/changelog.dox | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index ae2b4674..56cc560e 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -1,5 +1,5 @@ // -// Copyright (c) 2019-2023 Alexander Grund +// Copyright (c) 2019-2024 Alexander Grund // // Distributed under the Boost Software License, Version 1.0. // https://www.boost.org/LICENSE_1_0.txt @@ -8,6 +8,10 @@ \section changelog Changelog +\subsection changelog_11_3_1 Nowide 11.3.1 (Boost 1.88) +- Fix redefinition of `_CRT_SECURE_NO_WARNINGS` +- Make `getenv` thread-safe + \subsection changelog_11_3_0 Nowide 11.3.0 (Boost 1.82) - Add `convert_string` overload accepting a string - Add `quoted` to output (quoted) paths (std::filesystem or boost::filesystem) From a2d660f2c1ada415d28bb00b1699dbfa0188b461 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 4 Dec 2024 17:11:21 +0100 Subject: [PATCH 3/4] GHA: Fix MinGW test Using `bash` puts `/mingw64/bin` first in the path but the compiler from `/c/mingw64/bin` is used. This leads to errors running the tests: "Exit code 0xc0000139" (DLL issue) which are related to the use of `thread_local`. Using the powershell works in all cases. --- .github/workflows/ci_tests.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci_tests.yml b/.github/workflows/ci_tests.yml index d2ae15f4..b41337a6 100644 --- a/.github/workflows/ci_tests.yml +++ b/.github/workflows/ci_tests.yml @@ -108,7 +108,10 @@ jobs: # Run test with both bash and powershell and watch for "Using std::cin" on bash but not on powershell - name: Test working-directory: build - run: ctest --output-on-failure -C ${{matrix.buildType}} --verbose + run: | + # The bash shell adds an incompatible PATH for MinGW: https://github.com/actions/runner-images/issues/11102 + [[ "${{runner.os}}" != 'Windows' ]] || export PATH="/c/mingw64/bin:$PATH" + ctest --output-on-failure -C ${{matrix.buildType}} --verbose - name: Test on PowerShell working-directory: build shell: powershell From b9ff85d99a4d39ef656a75436cd3c4c635664566 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 19 Dec 2024 19:33:59 +0100 Subject: [PATCH 4/4] Workaround crash on exit for MinGW 32bit There is a bug in GCC for 32bit MinGW until version 11. This causes a use-after free for destruction of `thread_local` variables that crash the application when the destructor accesses any member. In the tests it shows up as exit code/status `-1073741819` i.e. `0xC0000005`. Workaround this by not destructing the `stackstring` instance used to hold the value of the last `getenv` result. In the case where any call to `getenv` of a thread yielded a large value heap memory will be allocated and not freed due to this missing destructor call causing a memory leak, possibly for each thread. However values up to some length are stored on stack memory and hence the missing destructor call does not cause a memory leak as the type is essentially trivial in this state. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83562 Fixed by https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=7fc0f78c3f43af1967cb7b1ee8f4947f3b890aa2 --- src/cstdlib.cpp | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/src/cstdlib.cpp b/src/cstdlib.cpp index 76f0319f..81533cdf 100644 --- a/src/cstdlib.cpp +++ b/src/cstdlib.cpp @@ -48,11 +48,46 @@ namespace nowide { #include #include +namespace { +// thread_local was broken on MinGW for all 32bit compiler releases prior to 11.x, see +// https://sourceforge.net/p/mingw-w64/bugs/527/ +// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83562 +// Using a non-trivial destructor causes program termination on thread exit. +#if defined(__MINGW32__) && !defined(__MINGW64__) && !defined(__clang__) && (__GNUC__ < 11) +class stackstring_for_thread +{ + union + { + boost::nowide::stackstring s_; + }; + +public: + stackstring_for_thread() : s_(){}; + // Empty destructor so the union member (using a non-trivial destructor) does not get destroyed. + // This will leak memory if any is allocated by the stackstring for each terminated thread + // but as most values fit into the stack buffer this is rare and still better than a crash. + ~stackstring_for_thread(){}; + void convert(const wchar_t* begin, const wchar_t* end) + { + s_.convert(begin, end); + } + + char* get() + { + return s_.get(); + } +}; +#else +using stackstring_for_thread = boost::nowide::stackstring; +#endif + +} // namespace + namespace boost { namespace nowide { char* getenv(const char* key) { - thread_local stackstring value; + thread_local stackstring_for_thread value; const wshort_stackstring name(key); @@ -72,7 +107,7 @@ namespace nowide { return 0; ptr = &tmp[0]; } - value.convert(ptr); + value.convert(ptr, ptr + n); return value.get(); }