diff --git a/Tests/LibC/CMakeLists.txt b/Tests/LibC/CMakeLists.txt index 1825ed23e4..5163d05254 100644 --- a/Tests/LibC/CMakeLists.txt +++ b/Tests/LibC/CMakeLists.txt @@ -2,6 +2,7 @@ set(TEST_SOURCES TestAbort.cpp TestAssert.cpp TestCType.cpp + TestEnvironment.cpp TestIo.cpp TestLibCExec.cpp TestLibCDirEnt.cpp diff --git a/Tests/LibC/TestEnvironment.cpp b/Tests/LibC/TestEnvironment.cpp new file mode 100644 index 0000000000..adb1f632fa --- /dev/null +++ b/Tests/LibC/TestEnvironment.cpp @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2023, Jelle Raaijmakers + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include +#include + +static int putenv_from_stack(char const* environment_variable) +{ + char environment_buffer[32]; + auto result = snprintf(environment_buffer, 31, "%s", environment_variable); + VERIFY(result > 0); + return putenv(environment_buffer); +} + +static char const* getenv_with_overwritten_stack(char const* environment_variable_name) +{ + char environment_buffer[32]; + memset(environment_buffer, ' ', 31); + environment_buffer[31] = 0; + return getenv(environment_variable_name); +} + +TEST_CASE(putenv_overwrite_invalid_stack_value) +{ + // Write an environment variable using a stack value + auto result = putenv_from_stack("TESTVAR=123"); + EXPECT_EQ(result, 0); + + // Try to retrieve the variable after overwriting the stack + auto environment_variable = getenv_with_overwritten_stack("TESTVAR"); + EXPECT_EQ(environment_variable, nullptr); + + // Try to overwrite the variable now that it's zeroed out + char new_environment_value[32]; + result = snprintf(new_environment_value, 31, "%s", "TESTVAR=456"); + VERIFY(result > 0); + result = putenv(new_environment_value); + EXPECT_EQ(result, 0); + + // Retrieve the variable and verify that it's set correctly + environment_variable = getenv("TESTVAR"); + EXPECT_NE(environment_variable, nullptr); + EXPECT_EQ(strcmp(environment_variable, "456"), 0); + + // Overwrite and retrieve it again to test correct search behavior for '=' + char final_environment_value[32]; + result = snprintf(final_environment_value, 31, "%s", "TESTVAR=789"); + VERIFY(result > 0); + result = putenv(final_environment_value); + EXPECT_EQ(result, 0); + environment_variable = getenv("TESTVAR"); + EXPECT_NE(environment_variable, nullptr); + EXPECT_EQ(strcmp(environment_variable, "789"), 0); +} diff --git a/Userland/Libraries/LibC/stdlib.cpp b/Userland/Libraries/LibC/stdlib.cpp index 8e006129e0..cae596a0ab 100644 --- a/Userland/Libraries/LibC/stdlib.cpp +++ b/Userland/Libraries/LibC/stdlib.cpp @@ -414,9 +414,8 @@ char* getenv(char const* name) size_t varLength = eq - decl; if (vl != varLength) continue; - if (strncmp(decl, name, varLength) == 0) { + if (strncmp(decl, name, varLength) == 0) return eq + 1; - } } return nullptr; } @@ -493,22 +492,23 @@ int serenity_putenv(char const* new_var, size_t length) int putenv(char* new_var) { char* new_eq = strchr(new_var, '='); - if (!new_eq) return unsetenv(new_var); - auto new_var_len = new_eq - new_var; + auto new_var_name_len = new_eq - new_var; int environ_size = 0; for (; environ[environ_size]; ++environ_size) { char* old_var = environ[environ_size]; - char* old_eq = strchr(old_var, '='); - VERIFY(old_eq); - auto old_var_len = old_eq - old_var; + auto old_var_name_max_length = strnlen(old_var, new_var_name_len); + char* old_eq = static_cast(memchr(old_var, '=', old_var_name_max_length + 1)); + if (!old_eq) + continue; // possibly freed or overwritten value - if (new_var_len != old_var_len) + auto old_var_name_len = old_eq - old_var; + if (new_var_name_len != old_var_name_len) continue; // can't match - if (strncmp(new_var, old_var, new_var_len) == 0) { + if (strncmp(new_var, old_var, new_var_name_len) == 0) { free_environment_variable_if_needed(old_var); environ[environ_size] = new_var; return 0; @@ -523,9 +523,8 @@ int putenv(char* new_var) return -1; } - for (int i = 0; environ[i]; ++i) { + for (int i = 0; environ[i]; ++i) new_environ[i] = environ[i]; - } new_environ[environ_size] = new_var; new_environ[environ_size + 1] = nullptr;