mirror of
https://github.com/LadybirdBrowser/ladybird.git
synced 2024-09-30 00:31:14 +00:00
LibC: Do not crash if putenv
variable is invalid
Dr. POSIX says: Although the space used by string is no longer used once a new string which defines name is passed to putenv(), if any thread in the application has used getenv() to retrieve a pointer to this variable, it should not be freed by calling free(). If the changed environment variable is one known by the system (such as the locale environment variables) the application should never free the buffer used by earlier calls to putenv() for the same variable. Applications _should_ not free the data passed to `putenv`, but they _could_ in practice. I found that our Quake II port misbehaves in this way, but does not crash on other platforms because glibc/musl `putenv` does not assume that environment variables are correctly formatted. The new behavior ignores environment variables without a '=' present, and prevents excessively reading beyond the variable's name if the data pointed to by the environment entry does not contain any null bytes. With this change, our Quake II port no longer crashes when switching from fullscreen to windowed mode.
This commit is contained in:
parent
2420effa7d
commit
f081a89cd9
Notes:
sideshowbarker
2024-07-16 23:52:10 +09:00
Author: https://github.com/gmta Commit: https://github.com/SerenityOS/serenity/commit/f081a89cd9 Pull-request: https://github.com/SerenityOS/serenity/pull/19251
|
@ -2,6 +2,7 @@ set(TEST_SOURCES
|
|||
TestAbort.cpp
|
||||
TestAssert.cpp
|
||||
TestCType.cpp
|
||||
TestEnvironment.cpp
|
||||
TestIo.cpp
|
||||
TestLibCExec.cpp
|
||||
TestLibCDirEnt.cpp
|
||||
|
|
59
Tests/LibC/TestEnvironment.cpp
Normal file
59
Tests/LibC/TestEnvironment.cpp
Normal file
|
@ -0,0 +1,59 @@
|
|||
/*
|
||||
* Copyright (c) 2023, Jelle Raaijmakers <jelle@gmta.nl>
|
||||
*
|
||||
* SPDX-License-Identifier: BSD-2-Clause
|
||||
*/
|
||||
|
||||
#include <LibTest/TestCase.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
||||
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);
|
||||
}
|
|
@ -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<char*>(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;
|
||||
|
|
Loading…
Reference in a new issue