-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix Bun.inspect null deref with Proxy prototypes; stop reporting from inside the sql lazy-property builders #30245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
e2df61c
c5c1f5c
f2190e1
6630bf6
da65188
aad7f3e
4639d3c
e0ef811
756fa7e
0e7406d
ae3fc29
9dffcd1
43f57e5
10d35e0
bf98514
873cddc
6ecdfcd
5414053
230e894
2a20b8d
bafa86f
bfb3414
ff102fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -311,29 +311,52 @@ static JSValue constructPluginObject(VM& vm, JSObject* bunObject) | |
| return pluginFunction; | ||
| } | ||
|
|
||
| // JSC's reifyStaticProperty passes the PropertyCallback result straight to | ||
| // putDirect without checking for exceptions, and JSValue::get asserts | ||
| // (!scope.exception() || !hasSlot). So these callbacks must not leave a | ||
| // pending exception; if loading the module fails we report it and return | ||
| // jsUndefined() so the property is reified to a valid value. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trimmed to one line in 6ecdfcd; the reasoning is in the PR description. |
||
| static JSValue requireBunSqlModule(VM& vm, Zig::GlobalObject* globalObject, JSC::ThrowScope& scope) | ||
| { | ||
| JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql); | ||
| if (auto* exception = scope.exception()) [[unlikely]] { | ||
| (void)scope.tryClearException(); | ||
| globalObject->reportUncaughtExceptionAtEventLoop(globalObject, exception); | ||
| (void)scope.tryClearException(); | ||
| return {}; | ||
| } | ||
| if (!sqlValue || !sqlValue.isObject()) [[unlikely]] | ||
| return {}; | ||
| return sqlValue; | ||
| } | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| static JSValue defaultBunSQLObject(VM& vm, JSObject* bunObject) | ||
| { | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| auto* globalObject = defaultGlobalObject(bunObject->globalObject()); | ||
| JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql); | ||
| #if BUN_DEBUG | ||
| if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception()); | ||
| #endif | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword)); | ||
| JSValue sqlValue = requireBunSqlModule(vm, globalObject, scope); | ||
| if (!sqlValue) return jsUndefined(); | ||
| JSValue result = sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword); | ||
| if (scope.exception()) [[unlikely]] { | ||
| (void)scope.tryClearException(); | ||
| return jsUndefined(); | ||
| } | ||
| return result ? result : jsUndefined(); | ||
| } | ||
|
|
||
| static JSValue constructBunSQLObject(VM& vm, JSObject* bunObject) | ||
| { | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| auto* globalObject = defaultGlobalObject(bunObject->globalObject()); | ||
| JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql); | ||
| #if BUN_DEBUG | ||
| if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception()); | ||
| #endif | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSValue sqlValue = requireBunSqlModule(vm, globalObject, scope); | ||
| if (!sqlValue) return jsUndefined(); | ||
| auto clientData = WebCore::clientData(vm); | ||
| RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, clientData->builtinNames().SQLPublicName())); | ||
| JSValue result = sqlValue.getObject()->get(globalObject, clientData->builtinNames().SQLPublicName()); | ||
| if (scope.exception()) [[unlikely]] { | ||
| (void)scope.tryClearException(); | ||
| return jsUndefined(); | ||
| } | ||
| return result ? result : jsUndefined(); | ||
| } | ||
|
|
||
| extern "C" JSC::EncodedJSValue JSPasswordObject__create(JSGlobalObject*); | ||
|
|
@@ -362,25 +385,33 @@ static JSValue constructBunShell(VM& vm, JSObject* bunObject) | |
| JSC::JSFunction* createShellFn = JSC::JSFunction::create(vm, globalObject, shellCreateBunShellTemplateFunctionCodeGenerator(vm), globalObject); | ||
|
|
||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| auto reportAndClear = [&]() { | ||
| auto* exception = scope.exception(); | ||
| (void)scope.tryClearException(); | ||
| if (exception) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, exception); | ||
| (void)scope.tryClearException(); | ||
| }; | ||
| auto args = JSC::MarkedArgumentBuffer(); | ||
| args.append(createShellInterpreterFunction); | ||
| args.append(createParsedShellScript); | ||
| JSC::JSValue shell = JSC::call(globalObject, createShellFn, args, "BunShell"_s); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (!shell.isObject()) [[unlikely]] { | ||
| throwTypeError(globalObject, scope, "Internal error: BunShell constructor did not return an object"_s); | ||
| return {}; | ||
| if (scope.exception()) [[unlikely]] { | ||
| reportAndClear(); | ||
| return jsUndefined(); | ||
| } | ||
|
|
||
| if (!shell || !shell.isObject()) [[unlikely]] | ||
| return jsUndefined(); | ||
|
|
||
| auto* bunShell = shell.getObject(); | ||
|
|
||
| auto ShellError = bunShell->get(globalObject, JSC::Identifier::fromString(vm, "ShellError"_s)); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| if (!ShellError.isObject()) [[unlikely]] { | ||
| throwTypeError(globalObject, scope, "Internal error: BunShell.ShellError is not an object"_s); | ||
| return {}; | ||
| if (scope.exception()) [[unlikely]] { | ||
| reportAndClear(); | ||
| return jsUndefined(); | ||
| } | ||
| if (!ShellError || !ShellError.isObject()) [[unlikely]] | ||
| return jsUndefined(); | ||
|
|
||
| bunShell->putDirectNativeFunction(vm, globalObject, Identifier::fromString(vm, "braces"_s), 1, Generated::BunObject::jsBraces, ImplementationVisibility::Public, NoIntrinsic, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::ReadOnly | 0); | ||
| bunShell->putDirectNativeFunction(vm, globalObject, Identifier::fromString(vm, "escape"_s), 1, BunObject_callback_shellEscape, ImplementationVisibility::Public, NoIntrinsic, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::ReadOnly | 0); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| // Several lazy PropertyCallback entries on the Bun object (Bun.$, Bun.sql, | ||
| // Bun.SQL, ...) run JS on first access. If that JS throws (e.g. a stack | ||
| // overflow), the callback previously returned an empty JSValue, which JSC's | ||
| // reifyStaticProperty passes straight to putDirect without an exception | ||
| // check, crashing on a null JSCell dereference. | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
| test.concurrent.each(["$", "sql", "SQL", "postgres"] as const)( | ||
| "accessing Bun.%s near stack overflow does not crash", | ||
| async key => { | ||
| const src = ` | ||
| function F() { | ||
| try { new F(); } catch {} | ||
| Bun[${JSON.stringify(key)}]; | ||
| } | ||
| try { new F(); } catch {} | ||
| Bun.gc(true); | ||
| `; | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", src], | ||
| env: bunEnv, | ||
| stdout: "ignore", | ||
| stderr: "ignore", | ||
| }); | ||
| const exitCode = await proc.exited; | ||
| expect(proc.signalCode).toBeNull(); | ||
| expect([0, 1]).toContain(exitCode); | ||
| }, | ||
| ); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| test.concurrent.each(["$", "sql", "SQL", "postgres"] as const)( | ||
| "accessing Bun.%s after clobbering Symbol does not crash", | ||
| async key => { | ||
| const src = ` | ||
| globalThis.Symbol = NaN; | ||
| try { Bun[${JSON.stringify(key)}]; } catch {} | ||
| try { Bun[${JSON.stringify(key)}]; } catch {} | ||
| Bun.gc(true); | ||
| `; | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", src], | ||
| env: bunEnv, | ||
| stdout: "ignore", | ||
| stderr: "ignore", | ||
| }); | ||
| const exitCode = await proc.exited; | ||
| expect(proc.signalCode).toBeNull(); | ||
| expect([0, 1]).toContain(exitCode); | ||
| }, | ||
| ); | ||
Uh oh!
There was an error while loading. Please reload this page.