[libc-commits] [libc] [libc] Add recursive locking to EnvironmentManager (PR #231259)
Jeff Bailey via libc-commits
libc-commits at lists.llvm.org
Mon Oct 12 11:50:54 PDT 2026
https://github.com/kaladron created https://github.com/llvm/llvm-project/pull/231259
POSIX notes that environment modifications need not be thread-safe, but real-world software routinely accesses and mutates the environment concurrently and expects the C library to synchronise access. Naive mutex locking introduces self-deadlock because memory allocators (such as Scudo) call getenv to inspect configuration flags while setenv is allocating memory.
We address this with a recursive mutex so re-entrant allocator lookups succeed safely. To prevent use-after-free for concurrent readers during capacity expansions, old environ pointer arrays are retained rather than freed and registered with the leak sanitiser. Initialisation uses double-checked locking to keep the getenv fast path lock-free.
Assisted-by: Automated tooling, human reviewed.
>From 463d6cf5d9eab78e43560627fc38a93e293c0e99 Mon Sep 17 00:00:00 2001
From: Jeff Bailey <jbailey at raspberryginger.com>
Date: Mon, 12 Oct 2026 14:25:13 +0100
Subject: [PATCH] [libc] Add recursive locking to EnvironmentManager
POSIX notes that environment modifications need not be thread-safe, but
real-world software routinely accesses and mutates the environment
concurrently and expects the C library to synchronise access. Naive
mutex locking introduces self-deadlock because memory allocators
(such as Scudo) call getenv to inspect configuration flags while setenv
is allocating memory.
We address this with a recursive mutex so re-entrant allocator lookups
succeed safely. To prevent use-after-free for concurrent readers during
capacity expansions, old environ pointer arrays are retained rather than
freed and registered with the leak sanitiser. Initialisation uses
double-checked locking to keep the getenv fast path lock-free.
Assisted-by: Automated tooling, human reviewed.
---
libc/src/__support/macros/CMakeLists.txt | 2 +
libc/src/__support/macros/sanitizer.h | 32 ++++++
libc/src/stdlib/CMakeLists.txt | 5 +
libc/src/stdlib/environ_internal.cpp | 71 ++++++++++---
libc/src/stdlib/environ_internal.h | 31 ++++--
.../integration/src/stdlib/CMakeLists.txt | 17 +++
.../src/stdlib/env_thread_test.cpp | 100 ++++++++++++++++++
libc/test/src/__support/CMakeLists.txt | 10 ++
libc/test/src/__support/sanitizer_test.cpp | 32 ++++++
9 files changed, 278 insertions(+), 22 deletions(-)
create mode 100644 libc/test/integration/src/stdlib/env_thread_test.cpp
create mode 100644 libc/test/src/__support/sanitizer_test.cpp
diff --git a/libc/src/__support/macros/CMakeLists.txt b/libc/src/__support/macros/CMakeLists.txt
index c600b453af510..d392c27a72357 100644
--- a/libc/src/__support/macros/CMakeLists.txt
+++ b/libc/src/__support/macros/CMakeLists.txt
@@ -28,6 +28,8 @@ add_header_library(
HDRS
sanitizer.h
DEPENDS
+ .attributes
+ .config
libc.src.__support.macros.properties.compiler
)
diff --git a/libc/src/__support/macros/sanitizer.h b/libc/src/__support/macros/sanitizer.h
index abed7a20a35b8..fec638fa34242 100644
--- a/libc/src/__support/macros/sanitizer.h
+++ b/libc/src/__support/macros/sanitizer.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_MACROS_SANITIZER_H
#define LLVM_LIBC_SRC___SUPPORT_MACROS_SANITIZER_H
+#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h" //LIBC_HAS_FEATURE
//-----------------------------------------------------------------------------
@@ -47,4 +48,35 @@
((void)(addr), (void)(size))
#endif
+//-----------------------------------------------------------------------------
+// LeakSanitizer utilities
+//-----------------------------------------------------------------------------
+
+namespace LIBC_NAMESPACE_DECL {
+namespace sanitizer {
+
+extern "C" [[gnu::weak]] void __lsan_ignore_object(const void *);
+
+#if LIBC_HAS_FEATURE(leak_sanitizer) || defined(LIBC_HAS_ADDRESS_SANITIZER)
+constexpr bool HAS_LEAK_SANITIZER = true;
+#else
+constexpr bool HAS_LEAK_SANITIZER = false;
+#endif
+
+template <bool Enabled = HAS_LEAK_SANITIZER>
+LIBC_INLINE void ignore_object(const void *addr) {
+ if constexpr (Enabled) {
+ if (&__lsan_ignore_object && addr)
+ __lsan_ignore_object(addr);
+ } else {
+ (void)addr;
+ }
+}
+
+} // namespace sanitizer
+} // namespace LIBC_NAMESPACE_DECL
+
+#define LIBC_LSAN_IGNORE_OBJECT(addr) \
+ LIBC_NAMESPACE::sanitizer::ignore_object(addr)
+
#endif // LLVM_LIBC_SRC___SUPPORT_MACROS_SANITIZER_H
diff --git a/libc/src/stdlib/CMakeLists.txt b/libc/src/stdlib/CMakeLists.txt
index 8bdca304e5190..87dc818260e72 100644
--- a/libc/src/stdlib/CMakeLists.txt
+++ b/libc/src/stdlib/CMakeLists.txt
@@ -81,11 +81,16 @@ set(environ_internal_deps
libc.config.app_h
libc.hdr.types.size_t
libc.src.__support.alloc_checker
+ libc.src.__support.CPP.atomic
+ libc.src.__support.CPP.mutex
libc.src.__support.CPP.new
libc.src.__support.CPP.optional
+ libc.src.__support.CPP.span
libc.src.__support.CPP.string_view
libc.src.__support.macros.attributes
libc.src.__support.macros.config
+ libc.src.__support.macros.sanitizer
+ libc.src.__support.threads.mutex
libc.src.string.memory_utils.inline_memcpy
)
set(environ_internal_compile_options ${environ_internal_gcc12_workaround})
diff --git a/libc/src/stdlib/environ_internal.cpp b/libc/src/stdlib/environ_internal.cpp
index 7fc2fdafd4867..1c24f8199a063 100644
--- a/libc/src/stdlib/environ_internal.cpp
+++ b/libc/src/stdlib/environ_internal.cpp
@@ -13,10 +13,14 @@
#include "src/stdlib/environ_internal.h"
#include "config/app.h"
+#include "src/__support/CPP/mutex.h"
#include "src/__support/CPP/new.h"
+#include "src/__support/CPP/span.h"
#include "src/__support/CPP/string_view.h"
#include "src/__support/alloc-checker.h"
+#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h"
+#include "src/__support/macros/sanitizer.h"
#include "src/string/memory_utils/inline_memcpy.h"
#ifdef LIBC_COPT_SUPPORT_ENVIRON
#include "src/unistd/environ.h"
@@ -35,7 +39,11 @@ constexpr size_t MIN_ENVIRON_CAPACITY = 32;
constexpr size_t ENVIRON_GROWTH_FACTOR = 2;
void EnvironmentManager::init_once() {
- if (initialized)
+ if (initialized.load(cpp::MemoryOrder::ACQUIRE))
+ return;
+
+ cpp::lock_guard lock(env_lock);
+ if (initialized.load(cpp::MemoryOrder::RELAXED))
return;
// Count entries in the startup environ.
@@ -47,11 +55,11 @@ void EnvironmentManager::init_once() {
count = c;
}
- initialized = true;
+ initialized.store(true, cpp::MemoryOrder::RELEASE);
}
EnvironmentManager &EnvironmentManager::get_instance() {
- static EnvironmentManager mgr;
+ static LIBC_CONSTINIT EnvironmentManager mgr;
mgr.init_once();
return mgr;
}
@@ -62,16 +70,25 @@ char **EnvironmentManager::get_array() {
return reinterpret_cast<char **>(app.env_ptr);
}
-EnvironmentManager::iterator EnvironmentManager::begin() { return get_array(); }
+EnvironmentManager::iterator EnvironmentManager::begin() {
+ cpp::lock_guard lock(env_lock);
+ return get_array();
+}
EnvironmentManager::iterator EnvironmentManager::end() {
+ cpp::lock_guard lock(env_lock);
char **env_array = get_array();
return env_array ? env_array + count : nullptr;
}
-size_t EnvironmentManager::size() const { return count; }
+size_t EnvironmentManager::size() {
+ cpp::lock_guard lock(env_lock);
+ return count;
+}
char *EnvironmentManager::get(cpp::string_view name) {
+ cpp::lock_guard lock(env_lock);
+
cpp::optional<size_t> idx = find_var(name);
if (!idx)
return nullptr;
@@ -83,11 +100,13 @@ cpp::optional<size_t> EnvironmentManager::find_var(cpp::string_view name) {
if (!env_array)
return cpp::nullopt;
- for (size_t i = 0; i < count; i++) {
- cpp::string_view current(env_array[i]);
+ size_t i = 0;
+ for (char *entry : cpp::span<char *>(env_array, count)) {
+ cpp::string_view current(entry);
if (current.starts_with(name) && current.size() > name.size() &&
current[name.size()] == '=')
return i;
+ i++;
}
return cpp::nullopt;
@@ -113,9 +132,14 @@ EnvironmentManager::alloc_and_copy(size_t new_capacity, char **old_storage,
return cpp::nullopt;
}
- for (size_t i = 0; i < copy_count; i++) {
- new_storage[i] = old_storage ? old_storage[i] : nullptr;
- new_ownership[i] = old_ownership ? old_ownership[i] : EnvStringOwnership();
+ if (old_storage) {
+ size_t i = 0;
+ for (char *entry : cpp::span<char *>(old_storage, copy_count)) {
+ new_storage[i] = entry;
+ new_ownership[i] =
+ old_ownership ? old_ownership[i] : EnvStringOwnership();
+ i++;
+ }
}
new_storage[copy_count] = nullptr;
@@ -165,8 +189,8 @@ bool EnvironmentManager::ensure_capacity(size_t needed) {
if (!result)
return false;
- delete[] storage;
- delete[] ownership;
+ char **old_storage = storage;
+ EnvStringOwnership *old_ownership = ownership;
auto [new_storage, new_ownership] = *result;
storage = new_storage;
@@ -179,11 +203,19 @@ bool EnvironmentManager::ensure_capacity(size_t needed) {
environ = storage;
#endif
+ // Old storage is intentionally retained so callers holding a reference
+ // to the previous environ array do not encounter use-after-free.
+ // We free old_ownership since it is internal to EnvironmentManager.
+ delete[] old_ownership;
+ LIBC_LSAN_IGNORE_OBJECT(old_storage);
+
return true;
}
int EnvironmentManager::set(cpp::string_view name, cpp::string_view value,
bool overwrite) {
+ cpp::lock_guard lock(env_lock);
+
cpp::optional<size_t> idx = find_var(name);
// If the variable exists and we're not overwriting, do nothing.
@@ -232,6 +264,11 @@ int EnvironmentManager::set(cpp::string_view name, cpp::string_view value,
}
int EnvironmentManager::unset(cpp::string_view name) {
+ cpp::lock_guard lock(env_lock);
+ return unset_unlocked(name);
+}
+
+int EnvironmentManager::unset_unlocked(cpp::string_view name) {
cpp::optional<size_t> idx = find_var(name);
if (!idx)
return 0; // Variable not found; POSIX defines this as success.
@@ -267,12 +304,14 @@ int EnvironmentManager::unset(cpp::string_view name) {
}
int EnvironmentManager::put(char *string) {
+ cpp::lock_guard lock(env_lock);
+
cpp::string_view sv(string);
size_t eq_pos = sv.find_first_of('=');
// No '=' found: treat as unset (POSIX behavior).
if (eq_pos == cpp::string_view::npos)
- return unset(sv);
+ return unset_unlocked(sv);
cpp::string_view name = sv.substr(0, eq_pos);
@@ -303,10 +342,14 @@ int EnvironmentManager::put(char *string) {
}
int EnvironmentManager::clear() {
- init_once();
+ cpp::lock_guard lock(env_lock);
// Free the pointer array, but leave the string buffers intact so that
// existing getenv pointers remain valid.
+ if (storage) {
+ for (char *entry : cpp::span<char *>(storage, count))
+ LIBC_LSAN_IGNORE_OBJECT(entry);
+ }
delete[] storage;
delete[] ownership;
storage = nullptr;
diff --git a/libc/src/stdlib/environ_internal.h b/libc/src/stdlib/environ_internal.h
index f39726d5ba9c7..b0d14c0237db2 100644
--- a/libc/src/stdlib/environ_internal.h
+++ b/libc/src/stdlib/environ_internal.h
@@ -15,10 +15,13 @@
#define LLVM_LIBC_SRC_STDLIB_ENVIRON_INTERNAL_H
#include "hdr/types/size_t.h"
+#include "src/__support/CPP/atomic.h"
+#include "src/__support/CPP/mutex.h"
#include "src/__support/CPP/optional.h"
#include "src/__support/CPP/string_view.h"
#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h"
+#include "src/__support/threads/mutex.h"
namespace LIBC_NAMESPACE_DECL {
namespace internal {
@@ -44,11 +47,16 @@ struct EnvStringOwnership {
// Centralized manager for environment variable operations.
// This class encapsulates all state and operations related to environment
-// management, including memory management and tracking of string ownership.
+// management, including thread safety and tracking of string ownership.
//
// The manager provides iterator support, allowing callers to iterate over
// the current environment entries using standard begin()/end() semantics.
class EnvironmentManager {
+ // Recursive mutex protecting all environment operations.
+ // Must be recursive to avoid self-deadlock when setenv() invokes memory
+ // allocation and the allocator (e.g. Scudo) calls getenv() to parse options.
+ mutable Mutex env_lock;
+
// Our allocated environ array (nullptr if using startup environ)
char **storage = nullptr;
@@ -63,16 +71,13 @@ class EnvironmentManager {
size_t count = 0;
// True if we have initialized from the startup environment
- bool initialized = false;
+ cpp::Atomic<bool> initialized{false};
// True if we allocated storage (and are responsible for freeing it)
bool is_ours = false;
- EnvironmentManager() = default;
- ~EnvironmentManager() = default;
-
- // Lazily initialize from the startup environment.
- // Called internally by get_instance(); idempotent.
+ // Lazily initialize from the startup environment using double-checked
+ // locking.
void init_once();
// Get a pointer to the current environ array.
@@ -103,7 +108,17 @@ class EnvironmentManager {
EnvStringOwnership *old_ownership,
size_t copy_count);
+ // Unset implementation assuming env_lock is already held.
+ int unset_unlocked(cpp::string_view name);
+
public:
+ LIBC_INLINE constexpr EnvironmentManager()
+ : env_lock(/*is_priority_inherit=*/false, /*is_recursive=*/true,
+ /*is_robust=*/false, /*is_pshared=*/false),
+ storage(nullptr), ownership(nullptr), capacity(0), count(0),
+ initialized(false), is_ours(false) {}
+ ~EnvironmentManager() = default;
+
// Get the singleton instance of the environment manager.
static EnvironmentManager &get_instance();
@@ -117,7 +132,7 @@ class EnvironmentManager {
using iterator = char **;
iterator begin();
iterator end();
- size_t size() const;
+ size_t size();
// Look up a variable by name. Returns a pointer to the value string
// (after the '='), or nullptr if not found.
diff --git a/libc/test/integration/src/stdlib/CMakeLists.txt b/libc/test/integration/src/stdlib/CMakeLists.txt
index 11b1e90fbbee8..c761e9d10d20a 100644
--- a/libc/test/integration/src/stdlib/CMakeLists.txt
+++ b/libc/test/integration/src/stdlib/CMakeLists.txt
@@ -90,6 +90,23 @@ if(${LIBC_TARGET_OS} STREQUAL "linux")
libc.src.unistd.environ
)
+ add_integration_test(
+ env_thread_test
+ SUITE
+ stdlib-integration-tests
+ SRCS
+ env_thread_test.cpp
+ DEPENDS
+ libc.include.threads
+ libc.src.__support.CPP.atomic
+ libc.src.__support.CPP.string_view
+ libc.src.stdlib.getenv
+ libc.src.stdlib.setenv
+ libc.src.stdlib.unsetenv
+ libc.src.threads.thrd_create
+ libc.src.threads.thrd_join
+ )
+
add_integration_test(
abort_test
SUITE
diff --git a/libc/test/integration/src/stdlib/env_thread_test.cpp b/libc/test/integration/src/stdlib/env_thread_test.cpp
new file mode 100644
index 0000000000000..0d8ded85ba1d0
--- /dev/null
+++ b/libc/test/integration/src/stdlib/env_thread_test.cpp
@@ -0,0 +1,100 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Multi-threaded integration test for environment variable operations.
+///
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/CPP/atomic.h"
+#include "src/__support/CPP/string_view.h"
+#include "src/stdlib/getenv.h"
+#include "src/stdlib/setenv.h"
+#include "src/stdlib/unsetenv.h"
+#include "src/threads/thrd_create.h"
+#include "src/threads/thrd_join.h"
+#include "test/IntegrationTest/test.h"
+
+#include <threads.h>
+
+static LIBC_NAMESPACE::cpp::Atomic<bool> stop_flag(false);
+
+static int writer_thread_a(void *) {
+ for (int i = 0; i < 500; ++i) {
+ ASSERT_EQ(LIBC_NAMESPACE::setenv("CONCURRENT_VAR_A", "value_a1", 1), 0);
+ const char *val = LIBC_NAMESPACE::getenv("CONCURRENT_VAR_A");
+ ASSERT_NE(val, nullptr);
+ ASSERT_EQ(LIBC_NAMESPACE::setenv("CONCURRENT_VAR_A", "value_a2", 1), 0);
+ ASSERT_EQ(LIBC_NAMESPACE::unsetenv("CONCURRENT_VAR_A"), 0);
+ }
+ return 0;
+}
+
+static int writer_thread_b(void *) {
+ for (int i = 0; i < 500; ++i) {
+ ASSERT_EQ(LIBC_NAMESPACE::setenv("CONCURRENT_VAR_B", "value_b1", 1), 0);
+ const char *val = LIBC_NAMESPACE::getenv("CONCURRENT_VAR_B");
+ ASSERT_NE(val, nullptr);
+ ASSERT_EQ(LIBC_NAMESPACE::setenv("CONCURRENT_VAR_B", "value_b2", 1), 0);
+ ASSERT_EQ(LIBC_NAMESPACE::unsetenv("CONCURRENT_VAR_B"), 0);
+ }
+ return 0;
+}
+
+static int reader_thread(void *) {
+ while (!stop_flag.load(LIBC_NAMESPACE::cpp::MemoryOrder::RELAXED)) {
+ const char *val_a = LIBC_NAMESPACE::getenv("CONCURRENT_VAR_A");
+ if (val_a) {
+ LIBC_NAMESPACE::cpp::string_view sv(val_a);
+ ASSERT_TRUE(sv == "value_a1" || sv == "value_a2");
+ }
+
+ const char *val_b = LIBC_NAMESPACE::getenv("CONCURRENT_VAR_B");
+ if (val_b) {
+ LIBC_NAMESPACE::cpp::string_view sv(val_b);
+ ASSERT_TRUE(sv == "value_b1" || sv == "value_b2");
+ }
+ }
+ return 0;
+}
+
+TEST_MAIN([[maybe_unused]] int argc, [[maybe_unused]] char **argv,
+ [[maybe_unused]] char **envp) {
+ thrd_t w1, w2, r1, r2;
+
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_create(&r1, reader_thread, nullptr),
+ static_cast<int>(thrd_success));
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_create(&r2, reader_thread, nullptr),
+ static_cast<int>(thrd_success));
+
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_create(&w1, writer_thread_a, nullptr),
+ static_cast<int>(thrd_success));
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_create(&w2, writer_thread_b, nullptr),
+ static_cast<int>(thrd_success));
+
+ int res = 0;
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_join(w1, &res),
+ static_cast<int>(thrd_success));
+ ASSERT_EQ(res, 0);
+
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_join(w2, &res),
+ static_cast<int>(thrd_success));
+ ASSERT_EQ(res, 0);
+
+ stop_flag.store(true, LIBC_NAMESPACE::cpp::MemoryOrder::RELAXED);
+
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_join(r1, &res),
+ static_cast<int>(thrd_success));
+ ASSERT_EQ(res, 0);
+
+ ASSERT_EQ(LIBC_NAMESPACE::thrd_join(r2, &res),
+ static_cast<int>(thrd_success));
+ ASSERT_EQ(res, 0);
+
+ return 0;
+}
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 765eec6b42178..f82141b1604d9 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -363,6 +363,16 @@ add_libc_test(
libc.src.__support.macros.properties.os
)
+add_libc_test(
+ sanitizer_test
+ SUITE
+ libc-support-tests
+ SRCS
+ sanitizer_test.cpp
+ DEPENDS
+ libc.src.__support.macros.sanitizer
+)
+
add_subdirectory(CPP)
add_subdirectory(File)
add_subdirectory(pwd)
diff --git a/libc/test/src/__support/sanitizer_test.cpp b/libc/test/src/__support/sanitizer_test.cpp
new file mode 100644
index 0000000000000..a6683417cce43
--- /dev/null
+++ b/libc/test/src/__support/sanitizer_test.cpp
@@ -0,0 +1,32 @@
+//===-- Unittests for sanitizer utilities ---------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/macros/sanitizer.h"
+#include "test/UnitTest/Test.h"
+
+TEST(LlvmLibcSanitizerTest, LsanIgnoreObject) {
+ int val = 42;
+
+ // Verify explicit invocation of the disabled path compiles and executes.
+ LIBC_NAMESPACE::sanitizer::ignore_object<false>(&val);
+ LIBC_NAMESPACE::sanitizer::ignore_object<false>(nullptr);
+
+ // Verify explicit invocation of the enabled path compiles and executes.
+ // When running without LeakSanitizer, the weak __lsan_ignore_object symbol
+ // resolves to nullptr, safely bypassing the call.
+ LIBC_NAMESPACE::sanitizer::ignore_object<true>(&val);
+ LIBC_NAMESPACE::sanitizer::ignore_object<true>(nullptr);
+
+ // Verify the default template argument (HAS_LEAK_SANITIZER).
+ LIBC_NAMESPACE::sanitizer::ignore_object(&val);
+ LIBC_NAMESPACE::sanitizer::ignore_object(nullptr);
+
+ // Verify the macro wrapper.
+ LIBC_LSAN_IGNORE_OBJECT(&val);
+ LIBC_LSAN_IGNORE_OBJECT(nullptr);
+}
More information about the libc-commits
mailing list