[libc-commits] [libc] [libc][stdlib] Avoid scanning values during environment lookup (PR #228633)

[via libc-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libc

Author: Nya~ (ChenMiaoi)

<details>
<summary>Changes</summary>

Constructing a string_view for each environment entry scans the entire value to determine its length, even when the name does not match. This makes lookups unnecessarily expensive for long values such as PATH.

Filter entries by their first character, then compare the remaining name up to a mismatch or null terminator. Require a following '=' to match the complete key, preserving support for empty names in putenv.

Closes #<!-- -->196016

---
Full diff: https://github.com/llvm/llvm-project/pull/228633.diff


3 Files Affected:

- (modified) libc/src/stdlib/environ_internal.cpp (+27-4) 
- (modified) libc/test/integration/src/stdlib/CMakeLists.txt (+1) 
- (modified) libc/test/integration/src/stdlib/getenv_test.cpp (+32) 


``````````diff
diff --git a/libc/src/stdlib/environ_internal.cpp b/libc/src/stdlib/environ_internal.cpp
index da2e5c8185ca06..29cb83f22bbe69 100644
--- a/libc/src/stdlib/environ_internal.cpp
+++ b/libc/src/stdlib/environ_internal.cpp
@@ -82,10 +82,33 @@ 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]);
-    if (current.starts_with(name) && current.size() > name.size() &&
-        current[name.size()] == '=')
+  // Compare entries directly: constructing a string_view from the entry's C
+  // string would scan the entire value to determine the entry's length, even
+  // when the name does not match.
+  // Empty names, supported by putenv, match entries starting with '='.
+  const char first = name.empty() ? '=' : name.front();
+
+  // Reject a leading null so matching an empty entry cannot advance past its
+  // terminator when the comparison resumes at index 1.
+  if (first == '\0')
+    return cpp::nullopt;
+  const size_t start = name.empty() ? 0 : 1;
+
+  for (size_t i = 0; i < count; ++i) {
+    const char *entry = env_array[i];
+    // Reject entries with a different first character before comparing the
+    // rest.
+    if (entry[0] != first)
+      continue;
+
+    size_t j = start;
+    // Bound the comparison by the name's length and stop at a mismatch or null
+    // terminator, so shorter entries cannot cause an out-of-bounds read.
+    while (j < name.size() && entry[j] != '\0' && entry[j] == name[j])
+      ++j;
+
+    // Require the full key to match, not just a prefix of a longer key.
+    if (j == name.size() && entry[j] == '=')
       return i;
   }
 
diff --git a/libc/test/integration/src/stdlib/CMakeLists.txt b/libc/test/integration/src/stdlib/CMakeLists.txt
index 7976e3833e282b..da3acfbf74d7dc 100644
--- a/libc/test/integration/src/stdlib/CMakeLists.txt
+++ b/libc/test/integration/src/stdlib/CMakeLists.txt
@@ -11,6 +11,7 @@ add_integration_test(
   SRCS
     getenv_test.cpp
   DEPENDS
+    libc.src.stdlib.environ_internal
     libc.src.stdlib.getenv
   ENV
     FRANCE=Paris
diff --git a/libc/test/integration/src/stdlib/getenv_test.cpp b/libc/test/integration/src/stdlib/getenv_test.cpp
index fe323c948afc48..d0218767ca8faf 100644
--- a/libc/test/integration/src/stdlib/getenv_test.cpp
+++ b/libc/test/integration/src/stdlib/getenv_test.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "src/stdlib/environ_internal.h"
 #include "src/stdlib/getenv.h"
 
 #include "test/IntegrationTest/test.h"
@@ -21,5 +22,36 @@ TEST_MAIN([[maybe_unused]] int argc, [[maybe_unused]] char **argv,
   ASSERT_TRUE(LIBC_NAMESPACE::getenv("FRANC") == nullptr);
   ASSERT_TRUE(LIBC_NAMESPACE::getenv("FRANCE1") == nullptr);
 
+  auto &env = LIBC_NAMESPACE::internal::EnvironmentManager::get_instance();
+  // Remove inherited variables that could interfere with the lookup tests.
+  ASSERT_EQ(env.unset("LIBC_GETENV_TEST"), 0);
+  ASSERT_EQ(env.unset("LIBC_GETENV_TEST_LONG"), 0);
+
+  // Exercise entries shorter than the requested name, including entries
+  // without '='. Lookup must stop at the terminator of each entry.
+  char *const ORIGINAL = *env.begin();
+  static char empty_entry[] = "";
+  static char short_entry[] = "LIBC_GETENV_TEST";
+  static char long_key[] = "LIBC_GETENV_TEST_LONG=value";
+  static char empty_value[] = "LIBC_GETENV_TEST=";
+  static char value_with_equals[] = "LIBC_GETENV_TEST=first=second";
+
+  *env.begin() = empty_entry;
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("LIBC_GETENV_TEST") == nullptr);
+  *env.begin() = short_entry;
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("LIBC_GETENV_TEST") == nullptr);
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("LIBC_GETENV_TEST_LONG") == nullptr);
+
+  *env.begin() = long_key;
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("LIBC_GETENV_TEST") == nullptr);
+
+  *env.begin() = empty_value;
+  ASSERT_STREQ(LIBC_NAMESPACE::getenv("LIBC_GETENV_TEST"), "");
+
+  *env.begin() = value_with_equals;
+  ASSERT_STREQ(LIBC_NAMESPACE::getenv("LIBC_GETENV_TEST"), "first=second");
+
+  *env.begin() = ORIGINAL;
+
   return 0;
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/228633