From 36fc324523e72c36eb4ae8c6a9fa9a0b51c66b5a Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 14 Aug 2024 22:46:45 -0700 Subject: [PATCH] Fixes #13311 (#13319) --- CMakeLists.txt | 7 ++++-- packages/bun-uws/src/TopicTree.h | 7 +++++- src/bun.js/api/server.classes.ts | 3 +++ src/bun.js/api/server.zig | 22 +++++++++++++------ src/bun.js/bindings/JSPropertyIterator.cpp | 2 +- src/bun.js/bindings/bindings.cpp | 16 ++++++++++++-- src/bun.js/bindings/bindings.zig | 2 +- src/codegen/class-definitions.ts | 1 + src/codegen/generate-classes.ts | 13 ++++++----- .../js/bun/websocket/websocket-server.test.ts | 9 ++++++++ test/js/node/crypto/pbkdf2.test.ts | 4 ++-- 11 files changed, 64 insertions(+), 22 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1020b770fe35b5..cfe7af0f5e3e2f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -30,6 +30,7 @@ cmake_path(APPEND GLOBAL_ZIG_CACHE_DIR "global") # Used in process.version, process.versions.node, napi, and elsewhere set(REPORTED_NODEJS_VERSION "22.6.0") + # Used in process.versions.modules and compared while loading V8 modules set(REPORTED_NODEJS_ABI_VERSION "127") @@ -1631,12 +1632,14 @@ endif() if(BUN_TIDY_ONLY) find_program(CLANG_TIDY_EXE NAMES "clang-tidy") - set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "-checks=-*,clang-analyzer-*,-clang-analyzer-webkit.UncountedLambdaCapturesChecker" "--fix" "--fix-errors" "--format-style=webkit" "--warnings-as-errors=*") + set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "-checks=-*,clang-analyzer-*,-clang-analyzer-webkit.UncountedLambdaCapturesChecker,-clang-analyzer-optin.core.EnumCastOutOfRange" "--fix" "--fix-errors" "--format-style=webkit" "--warnings-as-errors=*") set_target_properties(${bun} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}") endif() if(BUN_TIDY_ONLY_EXTRA) find_program(CLANG_TIDY_EXE NAMES "clang-tidy") - set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "-checks=-*,clang-analyzer-*,performance-*,-clang-analyzer-webkit.UncountedLambdaCapturesChecker" "--fix" "--fix-errors" "--format-style=webkit" "--warnings-as-errors=*") + + # -clang-analyzer-optin.core.EnumCastOutOfRange is disabled because it's too noisy, e.g. for JavaScriptCore/Lookup.h + set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "-checks=-*,clang-analyzer-*,performance-*,-clang-analyzer-webkit.UncountedLambdaCapturesChecker,-clang-analyzer-optin.core.EnumCastOutOfRange" "--fix" "--fix-errors" "--format-style=webkit" "--warnings-as-errors=*") set_target_properties(${bun} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}") endif() diff --git a/packages/bun-uws/src/TopicTree.h b/packages/bun-uws/src/TopicTree.h index 0d6e869b6d41f0..215afd28b286c8 100644 --- a/packages/bun-uws/src/TopicTree.h +++ b/packages/bun-uws/src/TopicTree.h @@ -79,9 +79,14 @@ template struct TopicTree { enum IteratorFlags { + // To appease clang-analyzer NONE = 0, + LAST = 1, - FIRST = 2 + FIRST = 2, + + // To appease clang-analyzer + FIRST_AND_LAST = FIRST | LAST }; /* Whomever is iterating this topic is locked to not modify its own list */ diff --git a/src/bun.js/api/server.classes.ts b/src/bun.js/api/server.classes.ts index b2f5610df8075b..ca71dc02fd449a 100644 --- a/src/bun.js/api/server.classes.ts +++ b/src/bun.js/api/server.classes.ts @@ -143,14 +143,17 @@ export default [ close: { fn: "close", length: 3, + passThis: true, }, terminate: { fn: "terminate", length: 0, + passThis: true, }, cork: { fn: "cork", length: 1, + passThis: true, }, getBufferedAmount: { fn: "getBufferedAmount", diff --git a/src/bun.js/api/server.zig b/src/bun.js/api/server.zig index 4b1f58f9f9e023..916d8498974bda 100644 --- a/src/bun.js/api/server.zig +++ b/src/bun.js/api/server.zig @@ -4637,17 +4637,21 @@ pub const ServerWebSocket = struct { this: *ServerWebSocket, globalThis: *JSC.JSGlobalObject, callframe: *JSC.CallFrame, + // Since we're passing the `this` value to the cork function, we need to + // make sure the `this` value is up to date. + this_value: JSC.JSValue, ) JSValue { const args = callframe.arguments(1); + this.this_value = this_value; if (args.len < 1) { - globalThis.throw("cork requires at least 1 argument", .{}); + globalThis.throwNotEnoughArguments("cork", 1, 0); return .zero; } + const callback = args.ptr[0]; if (callback.isEmptyOrUndefinedOrNull() or !callback.isCallable(globalThis.vm())) { - globalThis.throw("cork requires a function", .{}); - return .zero; + return globalThis.throwInvalidArgumentTypeValue("cork", "callback", callback); } if (this.isClosed()) { @@ -4656,16 +4660,13 @@ pub const ServerWebSocket = struct { var corker = Corker{ .globalObject = globalThis, - .this_value = this.this_value, + .this_value = this_value, .callback = callback, }; this.websocket().cork(&corker, Corker.run); const result = corker.result; - if (result.isEmptyOrUndefinedOrNull()) - return JSValue.jsUndefined(); - if (result.isAnyError()) { globalThis.throwValue(result); return JSValue.jsUndefined(); @@ -5034,9 +5035,12 @@ pub const ServerWebSocket = struct { this: *ServerWebSocket, globalThis: *JSC.JSGlobalObject, callframe: *JSC.CallFrame, + // Since close() can lead to the close() callback being called, let's always ensure the `this` value is up to date. + this_value: JSC.JSValue, ) JSValue { const args = callframe.arguments(2); log("close()", .{}); + this.this_value = this_value; if (this.isClosed()) { return .undefined; @@ -5078,12 +5082,16 @@ pub const ServerWebSocket = struct { this: *ServerWebSocket, globalThis: *JSC.JSGlobalObject, callframe: *JSC.CallFrame, + // Since terminate() can lead to close() being called, let's always ensure the `this` value is up to date. + this_value: JSC.JSValue, ) JSValue { _ = globalThis; const args = callframe.arguments(2); _ = args; log("terminate()", .{}); + this.this_value = this_value; + if (this.isClosed()) { return .undefined; } diff --git a/src/bun.js/bindings/JSPropertyIterator.cpp b/src/bun.js/bindings/JSPropertyIterator.cpp index a04a1a474e3bd5..e386c6cc5a2c84 100644 --- a/src/bun.js/bindings/JSPropertyIterator.cpp +++ b/src/bun.js/bindings/JSPropertyIterator.cpp @@ -24,7 +24,7 @@ class JSPropertyIterator { } RefPtr properties; - JSC::VM& vm; + Ref vm; static JSPropertyIterator* create(JSC::VM& vm, RefPtr data) { return new JSPropertyIterator(vm, data); diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index b199d55edcc203..c4cfd25e1fea22 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -2,6 +2,7 @@ #include "root.h" +#include "JavaScriptCore/Exception.h" #include "ErrorCode+List.h" #include "ErrorCode.h" #include "JavaScriptCore/ThrowScope.h" @@ -5069,8 +5070,19 @@ void JSC__VM__throwError(JSC__VM* vm_, JSC__JSGlobalObject* arg1, JSC__JSValue e JSValue value = JSValue::decode(encodedValue); scope.assertNoException(); // can't throw an exception when there's already one. ASSERT(!value.isEmpty()); // can't throw an empty value. - JSC::JSObject* error = value.getObject(); - JSC::Exception* exception = JSC::Exception::create(vm, error); + + // This case can happen if we did not call .toError() on a JSValue. + if (value.isCell()) { + JSC::JSCell* cell = value.asCell(); + if (cell->type() == JSC::CellType && cell->inherits()) { + scope.throwException(arg1, jsCast(value)); + return; + } + } + + // Do not call .getObject() on it. + // https://github.com/oven-sh/bun/issues/13311 + JSC::Exception* exception = JSC::Exception::create(vm, value); scope.throwException(arg1, exception); } diff --git a/src/bun.js/bindings/bindings.zig b/src/bun.js/bindings/bindings.zig index c9a5037bf9d8db..2dcd0bf3eb939f 100644 --- a/src/bun.js/bindings/bindings.zig +++ b/src/bun.js/bindings/bindings.zig @@ -2909,7 +2909,7 @@ pub const JSGlobalObject = opaque { ) JSValue { const ty_str = value.jsTypeString(this).toSlice(this, bun.default_allocator); defer ty_str.deinit(); - this.ERR_INVALID_ARG_TYPE("The \"{s}\" argument must be of type {s}. Received {s}", .{ field, typename, ty_str.slice() }).throw(); + this.ERR_INVALID_ARG_TYPE("The \"{s}\" argument must be of type {s}. Received {}", .{ field, typename, bun.fmt.quote(ty_str.slice()) }).throw(); return .zero; } diff --git a/src/codegen/class-definitions.ts b/src/codegen/class-definitions.ts index 594325b1d01140..6d05b4dcb601cb 100644 --- a/src/codegen/class-definitions.ts +++ b/src/codegen/class-definitions.ts @@ -29,6 +29,7 @@ export type Field = | ({ fn: string; length?: number; + passThis?: boolean; DOMJIT?: { returns: string; args?: [string, string] | [string, string, string] | [string] | []; diff --git a/src/codegen/generate-classes.ts b/src/codegen/generate-classes.ts index 61161c3f93a059..806f280bedb099 100644 --- a/src/codegen/generate-classes.ts +++ b/src/codegen/generate-classes.ts @@ -831,7 +831,8 @@ function renderDecls(symbolName, typeName, proto, supportsObjectCreate = false) `extern JSC_CALLCONV JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES ${symbolName( typeName, proto[name].fn, - )}(void* ptr, JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame);` + "\n"; + )}(void* ptr, JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame${proto[name].passThis ? ", JSC::EncodedJSValue thisValue" : ""});` + + "\n"; rows.push( ` JSC_DECLARE_HOST_FUNCTION(${symbolName(typeName, name)}Callback); @@ -1103,7 +1104,7 @@ JSC_DEFINE_HOST_FUNCTION(${symbolName(typeName, name)}Callback, (JSGlobalObject JSC::EnsureStillAliveScope thisArg = JSC::EnsureStillAliveScope(thisObject); -#ifdef BUN_DEBUG +#if defined(BUN_DEBUG) && BUN_DEBUG /** View the file name of the JS file that called this function * from a debugger */ SourceOrigin sourceOrigin = callFrame->callerSourceOrigin(vm); @@ -1113,14 +1114,14 @@ JSC_DEFINE_HOST_FUNCTION(${symbolName(typeName, name)}Callback, (JSGlobalObject lastFileName = fileName; } - JSC::EncodedJSValue result = ${symbolName(typeName, proto[name].fn)}(thisObject->wrapped(), lexicalGlobalObject, callFrame); + JSC::EncodedJSValue result = ${symbolName(typeName, proto[name].fn)}(thisObject->wrapped(), lexicalGlobalObject, callFrame${proto[name].passThis ? ", JSValue::encode(thisObject)" : ""}); ASSERT_WITH_MESSAGE(!JSValue::decode(result).isEmpty() or DECLARE_CATCH_SCOPE(vm).exception() != 0, \"${typeName}.${proto[name].fn} returned an empty value without an exception\"); return result; #endif - return ${symbolName(typeName, proto[name].fn)}(thisObject->wrapped(), lexicalGlobalObject, callFrame); + return ${symbolName(typeName, proto[name].fn)}(thisObject->wrapped(), lexicalGlobalObject, callFrame${proto[name].passThis ? ", JSValue::encode(thisObject)" : ""}); } `); @@ -1715,9 +1716,9 @@ const JavaScriptCoreBindings = struct { } output += ` - pub fn ${names.fn}(thisValue: *${typeName}, globalObject: *JSC.JSGlobalObject, callFrame: *JSC.CallFrame) callconv(JSC.conv) JSC.JSValue { + pub fn ${names.fn}(thisValue: *${typeName}, globalObject: *JSC.JSGlobalObject, callFrame: *JSC.CallFrame${proto[name].passThis ? ", js_this_value: JSC.JSValue" : ""}) callconv(JSC.conv) JSC.JSValue { if (comptime Environment.enable_logs) zig("${typeName}.${name}({})", .{callFrame}); - return @call(.always_inline, ${typeName}.${fn}, .{thisValue, globalObject, callFrame}); + return @call(.always_inline, ${typeName}.${fn}, .{thisValue, globalObject, callFrame${proto[name].passThis ? ", js_this_value" : ""}}); } `; } diff --git a/test/js/bun/websocket/websocket-server.test.ts b/test/js/bun/websocket/websocket-server.test.ts index e3f06290acb064..ec4de1e559579e 100644 --- a/test/js/bun/websocket/websocket-server.test.ts +++ b/test/js/bun/websocket/websocket-server.test.ts @@ -593,6 +593,15 @@ describe("ServerWebSocket", () => { let count = 0; return { open(ws) { + expect(() => ws.cork()).toThrow(); + expect(() => ws.cork(undefined)).toThrow(); + expect(() => ws.cork({})).toThrow(); + expect(() => + ws.cork(() => { + throw new Error("boom"); + }), + ).toThrow(); + setTimeout(() => { ws.cork(() => { ws.send("1"); diff --git a/test/js/node/crypto/pbkdf2.test.ts b/test/js/node/crypto/pbkdf2.test.ts index 0e8b5f207dcae5..fa2afe26279766 100644 --- a/test/js/node/crypto/pbkdf2.test.ts +++ b/test/js/node/crypto/pbkdf2.test.ts @@ -72,7 +72,7 @@ describe("invalid inputs", () => { for (let input of ["test", {}, [], true, undefined, null]) { test(`${input} is invalid`, () => { expect(() => crypto.pbkdf2("pass", "salt", input, 8, "sha256")).toThrow( - `The "iteration count" argument must be of type integer. Received ${typeof input}`, + `The "iteration count" argument must be of type integer. Received "${typeof input}"`, ); }); } @@ -115,7 +115,7 @@ describe("invalid inputs", () => { [Infinity, -Infinity, NaN].forEach(input => { test(`${input} keylen`, () => { expect(() => crypto.pbkdf2("password", "salt", 1, input, "sha256")).toThrow( - `The \"keylen\" argument must be of type integer. Received number`, + `The \"keylen\" argument must be of type integer. Received "number"`, ); }); });