From 47f9bb84e8e74717c1a23bf00fef33e363bd2720 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Mon, 24 Feb 2025 01:57:29 -0800 Subject: [PATCH] fix: invalid json import regression (#17612) --- cmake/tools/SetupWebKit.cmake | 2 +- src/bun.js/bindings/Base64Helpers.cpp | 4 +-- src/bun.js/bindings/BunString.cpp | 11 ++++++ src/bun.js/bindings/JSBuffer.cpp | 18 ++++++++-- src/bun.js/bindings/ModuleLoader.cpp | 10 ++++-- src/bun.js/bindings/NodeHTTP.cpp | 6 +++- src/bun.js/bindings/ZigGlobalObject.cpp | 2 +- src/bun.js/bindings/bindings.cpp | 13 ------- src/bun.js/bindings/bindings.zig | 7 ---- .../bindings/decodeURIComponentSIMD.cpp | 2 +- src/bun.js/bindings/headers-handwritten.h | 5 +++ test/regression/issue/17605.test.ts | 36 +++++++++++++++++++ 12 files changed, 84 insertions(+), 32 deletions(-) create mode 100644 test/regression/issue/17605.test.ts diff --git a/cmake/tools/SetupWebKit.cmake b/cmake/tools/SetupWebKit.cmake index 035e9b10c9..a54c202c6c 100644 --- a/cmake/tools/SetupWebKit.cmake +++ b/cmake/tools/SetupWebKit.cmake @@ -2,7 +2,7 @@ option(WEBKIT_VERSION "The version of WebKit to use") option(WEBKIT_LOCAL "If a local version of WebKit should be used instead of downloading") if(NOT WEBKIT_VERSION) - set(WEBKIT_VERSION 763e836175f705b3acb6fcd29270e68dd15d08bb) + set(WEBKIT_VERSION b9c0b519db002bf8cf69532dab853abf4121eaf6) endif() string(SUBSTRING ${WEBKIT_VERSION} 0 16 WEBKIT_VERSION_PREFIX) diff --git a/src/bun.js/bindings/Base64Helpers.cpp b/src/bun.js/bindings/Base64Helpers.cpp index b502ef32cf..c0088a5830 100644 --- a/src/bun.js/bindings/Base64Helpers.cpp +++ b/src/bun.js/bindings/Base64Helpers.cpp @@ -19,7 +19,7 @@ ExceptionOr atob(const String& encodedString) const auto span = encodedString.span16(); size_t expected_length = simdutf::latin1_length_from_utf16(span.size()); std::span ptr; - WTF::String convertedString = WTF::String::createUninitialized(expected_length, ptr); + WTF::String convertedString = WTF::String::tryCreateUninitialized(expected_length, ptr); if (UNLIKELY(convertedString.isNull())) { return WebCore::Exception { OutOfMemoryError }; } @@ -35,7 +35,7 @@ ExceptionOr atob(const String& encodedString) const auto span = encodedString.span8(); size_t result_length = simdutf::maximal_binary_length_from_base64(reinterpret_cast(span.data()), encodedString.length()); std::span ptr; - WTF::String outString = WTF::String::createUninitialized(result_length, ptr); + WTF::String outString = WTF::String::tryCreateUninitialized(result_length, ptr); if (UNLIKELY(outString.isNull())) { return WebCore::Exception { OutOfMemoryError }; } diff --git a/src/bun.js/bindings/BunString.cpp b/src/bun.js/bindings/BunString.cpp index b796010360..e9eb96955e 100644 --- a/src/bun.js/bindings/BunString.cpp +++ b/src/bun.js/bindings/BunString.cpp @@ -662,6 +662,17 @@ WTF::String BunString::toWTFString(ZeroCopyTag) const return WTF::String(); } +WTF::String BunString::toWTFString(NonNullTag) const +{ + WTF::String res = toWTFString(ZeroCopy); + if (res.isNull()) { + // TODO(dylan-conway): also use emptyString in toWTFString(ZeroCopy) and toWTFString. This will + // require reviewing each call site for isNull() checks and most likely changing them to isEmpty() + return WTF::emptyString(); + } + return res; +} + WTF::String BunString::transferToWTFString() { if (this->tag == BunStringTag::ZigString) { diff --git a/src/bun.js/bindings/JSBuffer.cpp b/src/bun.js/bindings/JSBuffer.cpp index d9563967c4..dab9407d52 100644 --- a/src/bun.js/bindings/JSBuffer.cpp +++ b/src/bun.js/bindings/JSBuffer.cpp @@ -1734,7 +1734,11 @@ static JSC::EncodedJSValue jsBufferToString(JSC::VM& vm, JSC::JSGlobalObject* le switch (encoding) { case WebCore::BufferEncodingType::latin1: { std::span data; - auto str = String::createUninitialized(length, data); + auto str = String::tryCreateUninitialized(length, data); + if (UNLIKELY(str.isNull())) { + throwOutOfMemoryError(lexicalGlobalObject, scope); + return JSValue::encode({}); + } memcpy(data.data(), reinterpret_cast(castedThis->vector()) + offset, length); return JSC::JSValue::encode(JSC::jsString(vm, WTFMove(str))); } @@ -1746,7 +1750,11 @@ static JSC::EncodedJSValue jsBufferToString(JSC::VM& vm, JSC::JSGlobalObject* le if (u16length == 0) { return JSC::JSValue::encode(JSC::jsEmptyString(vm)); } else { - auto str = String::createUninitialized(u16length, data); + auto str = String::tryCreateUninitialized(u16length, data); + if (UNLIKELY(str.isNull())) { + throwOutOfMemoryError(lexicalGlobalObject, scope); + return JSValue::encode({}); + } memcpy(reinterpret_cast(data.data()), reinterpret_cast(castedThis->vector()) + offset, u16length * 2); return JSC::JSValue::encode(JSC::jsString(vm, str)); } @@ -1758,7 +1766,11 @@ static JSC::EncodedJSValue jsBufferToString(JSC::VM& vm, JSC::JSGlobalObject* le // ascii: we always know the length // so we might as well allocate upfront std::span data; - auto str = String::createUninitialized(length, data); + auto str = String::tryCreateUninitialized(length, data); + if (UNLIKELY(str.isNull())) { + throwOutOfMemoryError(lexicalGlobalObject, scope); + return JSValue::encode({}); + } Bun__encoding__writeLatin1(reinterpret_cast(castedThis->vector()) + offset, length, data.data(), length, static_cast(encoding)); return JSC::JSValue::encode(JSC::jsString(vm, WTFMove(str))); } diff --git a/src/bun.js/bindings/ModuleLoader.cpp b/src/bun.js/bindings/ModuleLoader.cpp index 041e69d8ab..f6ef7510ab 100644 --- a/src/bun.js/bindings/ModuleLoader.cpp +++ b/src/bun.js/bindings/ModuleLoader.cpp @@ -669,7 +669,7 @@ JSValue fetchCommonJSModule( // When parsing tsconfig.*.json or jsconfig.*.json, we go through Bun's JSON // parser instead to support comments and trailing commas. if (res->result.value.tag == SyntheticModuleType::JSONForObjectLoader) { - WTF::String jsonSource = res->result.value.source_code.toWTFString(BunString::ZeroCopy); + WTF::String jsonSource = res->result.value.source_code.toWTFString(BunString::NonNull); JSC::JSValue value = JSC::JSONParseWithException(globalObject, jsonSource); RETURN_IF_EXCEPTION(scope, {}); @@ -859,9 +859,13 @@ static JSValue fetchESMSourceCode( // When parsing tsconfig.*.json or jsconfig.*.json, we go through Bun's JSON // parser instead to support comments and trailing commas. if (res->result.value.tag == SyntheticModuleType::JSONForObjectLoader) { - WTF::String jsonSource = res->result.value.source_code.toWTFString(BunString::ZeroCopy); + WTF::String jsonSource = res->result.value.source_code.toWTFString(BunString::NonNull); JSC::JSValue value = JSC::JSONParseWithException(globalObject, jsonSource); - RETURN_IF_EXCEPTION(scope, {}); + if (scope.exception()) { + auto* exception = scope.exception(); + scope.clearException(); + return reject(exception); + } // JSON can become strings, null, numbers, booleans so we must handle "export default 123" auto function = generateJSValueModuleSourceCode( diff --git a/src/bun.js/bindings/NodeHTTP.cpp b/src/bun.js/bindings/NodeHTTP.cpp index 8e1d69e053..b399e9d6e3 100644 --- a/src/bun.js/bindings/NodeHTTP.cpp +++ b/src/bun.js/bindings/NodeHTTP.cpp @@ -213,7 +213,11 @@ static EncodedJSValue assignHeadersFromUWebSockets(uWS::HttpRequest* request, JS auto pair = *it; StringView nameView = StringView(std::span { reinterpret_cast(pair.first.data()), pair.first.length() }); std::span data; - auto value = String::createUninitialized(pair.second.length(), data); + auto value = String::tryCreateUninitialized(pair.second.length(), data); + if (UNLIKELY(value.isNull())) { + throwOutOfMemoryError(globalObject, scope); + return JSValue::encode({}); + } if (pair.second.length() > 0) memcpy(data.data(), pair.second.data(), pair.second.length()); diff --git a/src/bun.js/bindings/ZigGlobalObject.cpp b/src/bun.js/bindings/ZigGlobalObject.cpp index b8b4700b99..18fbceff7b 100644 --- a/src/bun.js/bindings/ZigGlobalObject.cpp +++ b/src/bun.js/bindings/ZigGlobalObject.cpp @@ -1733,7 +1733,7 @@ JSC_DEFINE_HOST_FUNCTION(functionBTOA, if (!encodedString.is8Bit()) { std::span ptr; unsigned length = encodedString.length(); - auto dest = WTF::String::createUninitialized(length, ptr); + auto dest = WTF::String::tryCreateUninitialized(length, ptr); if (UNLIKELY(dest.isNull())) { throwOutOfMemoryError(globalObject, throwScope); return {}; diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index e2d901f82b..0d09815cd9 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -2509,19 +2509,6 @@ void JSC__JSValue___then(JSC__JSValue JSValue0, JSC__JSGlobalObject* arg1, JSC__ } } -JSC__JSValue JSC__JSValue__parseJSON(JSC__JSValue JSValue0, JSC__JSGlobalObject* arg1) -{ - JSC::JSValue jsValue = JSC::JSValue::decode(JSValue0); - - JSC::JSValue result = JSC::JSONParse(arg1, jsValue.toWTFString(arg1)); - - if (!result) { - result = JSC::JSValue(JSC::createSyntaxError(arg1->globalObject(), "Failed to parse JSON"_s)); - } - - return JSC::JSValue::encode(result); -} - JSC__JSValue JSC__JSGlobalObject__getCachedObject(JSC__JSGlobalObject* globalObject, const ZigString* arg1) { auto& vm = JSC::getVM(globalObject); diff --git a/src/bun.js/bindings/bindings.zig b/src/bun.js/bindings/bindings.zig index 6e94ec7997..20ef151bee 100644 --- a/src/bun.js/bindings/bindings.zig +++ b/src/bun.js/bindings/bindings.zig @@ -5873,13 +5873,6 @@ pub const JSValue = enum(i64) { }); } - pub fn parseJSON(this: JSValue, globalObject: *JSGlobalObject) JSValue { - return cppFn("parseJSON", .{ - this, - globalObject, - }); - } - pub fn stringIncludes(this: JSValue, globalObject: *JSGlobalObject, other: JSValue) bool { return cppFn("stringIncludes", .{ this, globalObject, other }); } diff --git a/src/bun.js/bindings/decodeURIComponentSIMD.cpp b/src/bun.js/bindings/decodeURIComponentSIMD.cpp index dc2f120897..6e40681e5b 100644 --- a/src/bun.js/bindings/decodeURIComponentSIMD.cpp +++ b/src/bun.js/bindings/decodeURIComponentSIMD.cpp @@ -284,7 +284,7 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionDecodeURIComponentSIMD, (JSC::JSGlobalObject const auto span = string.span16(); size_t expected_length = simdutf::latin1_length_from_utf16(span.size()); std::span ptr; - WTF::String convertedString = WTF::String::createUninitialized(expected_length, ptr); + WTF::String convertedString = WTF::String::tryCreateUninitialized(expected_length, ptr); if (UNLIKELY(convertedString.isNull())) { throwVMError(globalObject, scope, createOutOfMemoryError(globalObject)); return {}; diff --git a/src/bun.js/bindings/headers-handwritten.h b/src/bun.js/bindings/headers-handwritten.h index 00176b94c6..28ceb71086 100644 --- a/src/bun.js/bindings/headers-handwritten.h +++ b/src/bun.js/bindings/headers-handwritten.h @@ -53,6 +53,7 @@ typedef struct BunString { BunStringImpl impl; enum ZeroCopyTag { ZeroCopy }; + enum NonNullTag { NonNull }; // If it's not a WTFStringImpl, this does nothing inline void ref(); @@ -69,6 +70,10 @@ typedef struct BunString { // It's only truly zero-copy if it was already a WTFStringImpl (which it is if it came from JS and we didn't use ZigString) WTF::String toWTFString(ZeroCopyTag) const; + // If the string is empty, this will ensure m_impl is non-null by + // using shared static emptyString. + WTF::String toWTFString(NonNullTag) const; + WTF::String transferToWTFString(); // This one usually will clone the raw bytes. diff --git a/test/regression/issue/17605.test.ts b/test/regression/issue/17605.test.ts new file mode 100644 index 0000000000..3318cdb82d --- /dev/null +++ b/test/regression/issue/17605.test.ts @@ -0,0 +1,36 @@ +import { test, expect } from "bun:test"; +import { tmpdirSync } from "harness"; +import { write } from "bun"; +import { join } from "path"; + +test("empty and invalid JSON import do not crash", async () => { + const testDir = tmpdirSync("empty-and-invalid-json-import-do-not-crash"); + + await Promise.all([ + write(join(testDir, "empty.json"), ""), + write( + join(testDir, "invalid.json"), + ` +{ + "a": 1 + "b": 2 +}`, + ), + ]); + + expect(async () => { + await import(join(testDir, "empty.json") + "?0"); + }).toThrow("JSON Parse error: Unexpected EOF"); + + expect(async () => { + await import(join(testDir, "invalid.json") + "?1"); + }).toThrow("JSON Parse error: Expected '}'"); + + expect(() => { + const json = require(join(testDir, "empty.json")); + }).toThrow("JSON Parse error: Unexpected EOF"); + + expect(() => { + const json = require(join(testDir, "invalid.json")); + }).toThrow("JSON Parse error: Expected '}'"); +});