From 2cbb196f293fa5429ac01167d7d89b7fd0d0262c Mon Sep 17 00:00:00 2001 From: Zack Radisic <56137411+zackradisic@users.noreply.github.com> Date: Sat, 21 Jun 2025 23:44:59 -0700 Subject: [PATCH] Fix crash with garbage environment variables (#20527) Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: Zack Radisic --- .../bindings/JSEnvironmentVariableMap.cpp | 11 +- test/cli/run/garbage-env.c | 133 ++++++++++++++++++ test/cli/run/garbage-env.test.ts | 25 ++++ test/no-validate-exceptions.txt | 1 + 4 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 test/cli/run/garbage-env.c create mode 100644 test/cli/run/garbage-env.test.ts diff --git a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp index d3f1baafb9..7c1781298f 100644 --- a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp +++ b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp @@ -305,10 +305,13 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) bool hasNodeTLSRejectUnauthorized = false; bool hasBunConfigVerboseFetch = false; + auto* cached_getter_setter = JSC::CustomGetterSetter::create(vm, jsGetterEnvironmentVariable, nullptr); + for (size_t i = 0; i < count; i++) { unsigned char* chars; size_t len = Bun__getEnvKey(list, i, &chars); - auto name = String::fromUTF8(std::span { chars, len }); + // We can't really trust that the OS gives us valid UTF-8 + auto name = String::fromUTF8ReplacingInvalidSequences(std::span { chars, len }); #if OS(WINDOWS) keyArray->putByIndexInline(globalObject, (unsigned)i, jsString(vm, name), false); #endif @@ -347,7 +350,11 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) } } - object->putDirectCustomAccessor(vm, identifier, JSC::CustomGetterSetter::create(vm, jsGetterEnvironmentVariable, jsSetterEnvironmentVariable), JSC::PropertyAttribute::CustomAccessor | 0); + // JSC::PropertyAttribute::CustomValue calls the getter ONCE (the first + // time) and then sets it onto the object, subsequent calls to the + // getter will not go through the getter and instead will just do the + // property lookup. + object->putDirectCustomAccessor(vm, identifier, cached_getter_setter, JSC::PropertyAttribute::CustomValue | 0); } unsigned int TZAttrs = JSC::PropertyAttribute::CustomAccessor | 0; diff --git a/test/cli/run/garbage-env.c b/test/cli/run/garbage-env.c new file mode 100644 index 0000000000..de651b305b --- /dev/null +++ b/test/cli/run/garbage-env.c @@ -0,0 +1,133 @@ +#include +#include +#include +#include +#include +#include + +int main() { + int stdout_pipe[2], stderr_pipe[2]; + pid_t pid; + int status; + char stdout_buffer[4096] = {0}; + char stderr_buffer[4096] = {0}; + + // Create pipes for stdout and stderr + if (pipe(stdout_pipe) == -1 || pipe(stderr_pipe) == -1) { + perror("pipe"); + return 1; + } + + // Create garbage environment variables with stack buffers containing + // arbitrary bytes + char garbage1[64]; + char garbage2[64]; + char garbage3[64]; + char garbage4[64]; + char garbage5[64]; + + // Fill with arbitrary non-ASCII/UTF-8 bytes + for (int i = 0; i < 63; i++) { + garbage1[i] = (char)(0x80 + (i % 128)); // Invalid UTF-8 start bytes + garbage2[i] = (char)(0xFF - (i % 256)); // High bytes + garbage3[i] = (char)(i * 3 + 128); // Mixed garbage + garbage4[i] = (char)(0xC0 | (i & 0x1F)); // Invalid UTF-8 sequences + } + garbage1[63] = '\0'; + garbage2[63] = '\0'; + garbage3[63] = '\0'; + garbage4[63] = '\0'; + + for (int i = 0; i < 10; i++) { + garbage5[i] = (char)(0x80 + (i % 128)); + } + garbage5[10] = '='; + garbage5[11] = 0x81; + garbage5[12] = 0xF5; + garbage5[13] = 0xC1; + garbage5[14] = 0xC2; + + char *garbage_env[] = { + garbage5, + // garbage1, + // garbage2, + // garbage3, + // garbage4, + "PATH=/usr/bin:/bin", // Keep PATH so we can find commands + "BUN_DEBUG_QUIET_LOGS=1", "OOGA=booga", "OOGA=laskdjflsdf", NULL}; + + pid = fork(); + + if (pid == -1) { + perror("fork"); + return 1; + } + + if (pid == 0) { + // Child process + close(stdout_pipe[0]); // Close read end + close(stderr_pipe[0]); // Close read end + + // Redirect stdout and stderr to pipes + dup2(stdout_pipe[1], STDOUT_FILENO); + dup2(stderr_pipe[1], STDERR_FILENO); + + close(stdout_pipe[1]); + close(stderr_pipe[1]); + + char *BUN_PATH = getenv("BUN_PATH"); + if (BUN_PATH == NULL) { + fprintf(stderr, "Missing BUN_PATH!\n"); + fflush(stderr); + exit(1); + } + execve(BUN_PATH, + (char *[]){"bun-debug", "-e", "console.log(process.env)", NULL}, + garbage_env); + + // If both fail, exit with error + perror("execve"); + exit(127); + } else { + // Parent process + close(stdout_pipe[1]); // Close write end + close(stderr_pipe[1]); // Close write end + + // Read from stdout pipe + ssize_t stdout_bytes = + read(stdout_pipe[0], stdout_buffer, sizeof(stdout_buffer) - 1); + if (stdout_bytes > 0) { + stdout_buffer[stdout_bytes] = '\0'; + } + + // Read from stderr pipe + ssize_t stderr_bytes = + read(stderr_pipe[0], stderr_buffer, sizeof(stderr_buffer) - 1); + if (stderr_bytes > 0) { + stderr_buffer[stderr_bytes] = '\0'; + } + + close(stdout_pipe[0]); + close(stderr_pipe[0]); + + // Wait for child process + waitpid(pid, &status, 0); + + // Print results + printf("=== PROCESS OUTPUT ===\n"); + printf("Exit code: %d\n", WEXITSTATUS(status)); + + printf("\n=== STDOUT ===\n"); + printf("%s", stdout_buffer); + fflush(stdout); + + if (stderr_bytes > 0) { + fprintf(stderr, "\n=== STDERR ===\n"); + fprintf(stderr, "%s", stderr_buffer); + fflush(stderr); + } + exit(status); + } + + return 0; +} \ No newline at end of file diff --git a/test/cli/run/garbage-env.test.ts b/test/cli/run/garbage-env.test.ts new file mode 100644 index 0000000000..299e0f213b --- /dev/null +++ b/test/cli/run/garbage-env.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, test } from "bun:test"; +import { bunExe, isPosix } from "harness"; +import path from "path"; + +describe.if(isPosix)("garbage env", () => { + test("garbage env", async () => { + const cfile = path.join(import.meta.dirname, "garbage-env.c"); + { + const cc = Bun.which("clang") || Bun.which("gcc") || Bun.which("cc"); + const { exitCode, stderr } = await Bun.$`${cc} -o garbage-env ${cfile}`; + const stderrText = stderr.toString(); + if (stderrText.length > 0) { + console.error(stderrText); + } + expect(exitCode).toBe(0); + } + + const { exitCode, stderr } = await Bun.$`./garbage-env`.env({ BUN_PATH: bunExe() }); + const stderrText = stderr.toString(); + if (stderrText.length > 0) { + console.error(stderrText); + } + expect(exitCode).toBe(0); + }); +}); diff --git a/test/no-validate-exceptions.txt b/test/no-validate-exceptions.txt index 39d038dc19..ebd4d546bb 100644 --- a/test/no-validate-exceptions.txt +++ b/test/no-validate-exceptions.txt @@ -71,6 +71,7 @@ test/cli/install/catalogs.test.ts test/cli/install/npmrc.test.ts test/cli/install/overrides.test.ts test/cli/run/env.test.ts +test/cli/run/garbage-env.test.ts test/cli/run/esm-defineProperty.test.ts test/cli/run/jsx-namespaced-attributes.test.ts test/cli/run/log-test.test.ts