From 774e30d383d8e83bc84d2a7ba3354eedbbe7f8a2 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Mon, 23 Dec 2024 03:40:51 -0800 Subject: [PATCH] Make originalLine and originalColumn getter calls not observable (#15951) --- src/bun.js/bindings/ZigGlobalObject.cpp | 6 +- src/bun.js/bindings/bindings.cpp | 163 +++++++++++------- test/js/bun/util/inspect-error.test.js | 135 +++++++++++++-- .../parallel/util-inspect.test.js | 21 +-- 4 files changed, 228 insertions(+), 97 deletions(-) diff --git a/src/bun.js/bindings/ZigGlobalObject.cpp b/src/bun.js/bindings/ZigGlobalObject.cpp index 42060e503b..a8a8ea91ce 100644 --- a/src/bun.js/bindings/ZigGlobalObject.cpp +++ b/src/bun.js/bindings/ZigGlobalObject.cpp @@ -457,7 +457,7 @@ WTF::String Bun::formatStackTrace( sb.append(remappedFrame.source_url.toWTFString()); if (remappedFrame.remapped) { - errorInstance->putDirect(vm, builtinNames(vm).originalLinePublicName(), jsNumber(originalLine.oneBasedInt()), 0); + errorInstance->putDirect(vm, builtinNames(vm).originalLinePublicName(), jsNumber(originalLine.oneBasedInt()), PropertyAttribute::DontEnum | 0); hasSet = true; line = remappedFrame.position.line(); } @@ -599,8 +599,8 @@ WTF::String Bun::formatStackTrace( if (remappedFrame.remapped) { if (errorInstance) { - errorInstance->putDirect(vm, builtinNames(vm).originalLinePublicName(), jsNumber(originalLine.oneBasedInt()), 0); - errorInstance->putDirect(vm, builtinNames(vm).originalColumnPublicName(), jsNumber(originalColumn.oneBasedInt()), 0); + errorInstance->putDirect(vm, builtinNames(vm).originalLinePublicName(), jsNumber(originalLine.oneBasedInt()), PropertyAttribute::DontEnum | 0); + errorInstance->putDirect(vm, builtinNames(vm).originalColumnPublicName(), jsNumber(originalColumn.oneBasedInt()), PropertyAttribute::DontEnum | 0); } } } diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index 7df8634f02..cb1a0446df 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -4554,6 +4554,23 @@ static void populateStackTrace(JSC::VM& vm, const WTF::Vector& trace->frames_len = frame_i; } +static JSC::JSValue getNonObservable(JSC::VM& vm, JSC::JSGlobalObject* global, JSC::JSObject* obj, const JSC::PropertyName& propertyName) +{ + PropertySlot slot = PropertySlot(obj, PropertySlot::InternalMethodType::VMInquiry, &vm); + if (obj->getNonIndexPropertySlot(global, propertyName, slot)) { + if (slot.isAccessor()) { + return {}; + } + + JSValue value = slot.getValue(global, propertyName); + if (!value || value.isUndefinedOrNull()) { + return {}; + } + return value; + } + return {}; +} + #define SYNTAX_ERROR_CODE 4 static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, @@ -4593,6 +4610,10 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, except->message = Bun::toStringRef(err->sanitizedMessageString(global)); } + if (UNLIKELY(scope.exception())) { + scope.clearExceptionExceptTermination(); + } + except->name = Bun::toStringRef(err->sanitizedNameString(global)); except->runtime_type = err->runtimeTypeForCause(); @@ -4600,7 +4621,7 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, const auto& names = builtinNames(vm); if (except->code != SYNTAX_ERROR_CODE) { - if (JSC::JSValue syscall = obj->getIfPropertyExists(global, names.syscallPublicName())) { + if (JSC::JSValue syscall = getNonObservable(vm, global, obj, names.syscallPublicName())) { if (syscall.isString()) { except->syscall = Bun::toStringRef(global, syscall); } @@ -4610,7 +4631,7 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, scope.clearExceptionExceptTermination(); } - if (JSC::JSValue code = obj->getIfPropertyExists(global, names.codePublicName())) { + if (JSC::JSValue code = getNonObservable(vm, global, obj, names.codePublicName())) { if (code.isString() || code.isNumber()) { except->code_ = Bun::toStringRef(global, code); } @@ -4620,7 +4641,7 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, scope.clearExceptionExceptTermination(); } - if (JSC::JSValue path = obj->getIfPropertyExists(global, names.pathPublicName())) { + if (JSC::JSValue path = getNonObservable(vm, global, obj, names.pathPublicName())) { if (path.isString()) { except->path = Bun::toStringRef(global, path); } @@ -4630,7 +4651,7 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, scope.clearExceptionExceptTermination(); } - if (JSC::JSValue fd = obj->getIfPropertyExists(global, names.fdPublicName())) { + if (JSC::JSValue fd = getNonObservable(vm, global, obj, names.fdPublicName())) { if (fd.isNumber()) { except->fd = fd.toInt32(global); } @@ -4640,7 +4661,7 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, scope.clearExceptionExceptTermination(); } - if (JSC::JSValue errno_ = obj->getIfPropertyExists(global, names.errnoPublicName())) { + if (JSC::JSValue errno_ = getNonObservable(vm, global, obj, names.errnoPublicName())) { if (errno_.isNumber()) { except->errno_ = errno_.toInt32(global); } @@ -4652,87 +4673,103 @@ static void fromErrorInstance(ZigException* except, JSC::JSGlobalObject* global, } if (getFromSourceURL) { - // we don't want to serialize JSC::StackFrame longer than we need to - // so in this case, we parse the stack trace as a string - if (JSC::JSValue stackValue = obj->getIfPropertyExists(global, vm.propertyNames->stack)) { - if (stackValue.isString()) { - WTF::String stack = stackValue.toWTFString(global); - if (!stack.isEmpty()) { - V8StackTraceIterator iterator(stack); - const uint8_t frame_count = except->stack.frames_len; + { + // we don't want to serialize JSC::StackFrame longer than we need to + // so in this case, we parse the stack trace as a string - except->stack.frames_len = 0; + auto catchScope = DECLARE_CATCH_SCOPE(vm); - iterator.forEachFrame([&](const V8StackTraceIterator::StackFrame& frame, bool& stop) -> void { - ASSERT(except->stack.frames_len < frame_count); - auto& current = except->stack.frames_ptr[except->stack.frames_len]; - current = {}; + // This one intentionally calls getters. + if (JSC::JSValue stackValue = obj->getIfPropertyExists(global, vm.propertyNames->stack)) { + if (stackValue.isString()) { + WTF::String stack = stackValue.toWTFString(global); + if (UNLIKELY(catchScope.exception())) { + catchScope.clearExceptionExceptTermination(); + } + if (!stack.isEmpty()) { - String functionName = frame.functionName.toString(); - String sourceURL = frame.sourceURL.toString(); - current.function_name = Bun::toStringRef(functionName); - current.source_url = Bun::toStringRef(sourceURL); - current.position.line_zero_based = frame.lineNumber.zeroBasedInt(); - current.position.column_zero_based = frame.columnNumber.zeroBasedInt(); + V8StackTraceIterator iterator(stack); + const uint8_t frame_count = except->stack.frames_len; - current.remapped = true; + except->stack.frames_len = 0; - if (frame.isConstructor) { - current.code_type = ZigStackFrameCodeConstructor; - } else if (frame.isGlobalCode) { - current.code_type = ZigStackFrameCodeGlobal; + iterator.forEachFrame([&](const V8StackTraceIterator::StackFrame& frame, bool& stop) -> void { + ASSERT(except->stack.frames_len < frame_count); + auto& current = except->stack.frames_ptr[except->stack.frames_len]; + current = {}; + + String functionName = frame.functionName.toString(); + String sourceURL = frame.sourceURL.toString(); + current.function_name = Bun::toStringRef(functionName); + current.source_url = Bun::toStringRef(sourceURL); + current.position.line_zero_based = frame.lineNumber.zeroBasedInt(); + current.position.column_zero_based = frame.columnNumber.zeroBasedInt(); + + current.remapped = true; + + if (frame.isConstructor) { + current.code_type = ZigStackFrameCodeConstructor; + } else if (frame.isGlobalCode) { + current.code_type = ZigStackFrameCodeGlobal; + } + + except->stack.frames_len += 1; + + stop = except->stack.frames_len >= frame_count; + }); + + if (except->stack.frames_len > 0) { + getFromSourceURL = false; + except->remapped = true; + } else { + except->stack.frames_len = frame_count; } - - except->stack.frames_len += 1; - - stop = except->stack.frames_len >= frame_count; - }); - - if (except->stack.frames_len > 0) { - getFromSourceURL = false; - except->remapped = true; - } else { - except->stack.frames_len = frame_count; } } } + + if (UNLIKELY(catchScope.exception())) { + catchScope.clearExceptionExceptTermination(); + } } - if (getFromSourceURL) { + if (JSC::JSValue sourceURL = getNonObservable(vm, global, obj, vm.propertyNames->sourceURL)) { + if (sourceURL.isString()) { + except->stack.frames_ptr[0].source_url = Bun::toStringRef(global, sourceURL); - if (JSC::JSValue sourceURL = obj->getIfPropertyExists(global, vm.propertyNames->sourceURL)) { - if (sourceURL.isString()) { - except->stack.frames_ptr[0].source_url = Bun::toStringRef(global, sourceURL); + // Take care not to make these getter calls observable. - if (JSC::JSValue column = obj->getIfPropertyExists(global, vm.propertyNames->column)) { - if (column.isNumber()) { - except->stack.frames_ptr[0].position.column_zero_based = OrdinalNumber::fromOneBasedInt(column.toInt32(global)).zeroBasedInt(); - } + if (JSC::JSValue column = getNonObservable(vm, global, obj, vm.propertyNames->column)) { + if (column.isNumber()) { + except->stack.frames_ptr[0].position.column_zero_based = OrdinalNumber::fromOneBasedInt(column.toInt32(global)).zeroBasedInt(); } + } - if (JSC::JSValue line = obj->getIfPropertyExists(global, vm.propertyNames->line)) { - if (line.isNumber()) { - except->stack.frames_ptr[0].position.line_zero_based = OrdinalNumber::fromOneBasedInt(line.toInt32(global)).zeroBasedInt(); + if (JSC::JSValue line = getNonObservable(vm, global, obj, vm.propertyNames->line)) { + if (line.isNumber()) { + except->stack.frames_ptr[0].position.line_zero_based = OrdinalNumber::fromOneBasedInt(line.toInt32(global)).zeroBasedInt(); - if (JSC::JSValue lineText = obj->getIfPropertyExists(global, names.lineTextPublicName())) { - if (lineText.isString()) { - if (JSC::JSString* jsStr = lineText.toStringOrNull(global)) { - auto str = jsStr->value(global); - except->stack.source_lines_ptr[0] = Bun::toStringRef(str); - except->stack.source_lines_numbers[0] = except->stack.frames_ptr[0].position.line(); - except->stack.source_lines_len = 1; - except->remapped = true; - } + if (JSC::JSValue lineText = getNonObservable(vm, global, obj, builtinNames(vm).lineTextPublicName())) { + if (lineText.isString()) { + if (JSC::JSString* jsStr = lineText.toStringOrNull(global)) { + auto str = jsStr->value(global); + except->stack.source_lines_ptr[0] = Bun::toStringRef(str); + except->stack.source_lines_numbers[0] = except->stack.frames_ptr[0].position.line(); + except->stack.source_lines_len = 1; + except->remapped = true; } } } } - - except->stack.frames_len = 1; - except->stack.frames_ptr[0].remapped = obj->hasProperty(global, names.originalLinePublicName()); } } + + { + except->stack.frames_len = 1; + PropertySlot slot = PropertySlot(obj, PropertySlot::InternalMethodType::VMInquiry, &vm); + except->stack.frames_ptr[0].remapped = obj->getNonIndexPropertySlot(global, names.originalLinePublicName(), slot); + } } } diff --git a/test/js/bun/util/inspect-error.test.js b/test/js/bun/util/inspect-error.test.js index a65edd62b3..8e438f9cc1 100644 --- a/test/js/bun/util/inspect-error.test.js +++ b/test/js/bun/util/inspect-error.test.js @@ -1,22 +1,51 @@ -import { expect, test } from "bun:test"; +import { expect, test, describe, jest } from "bun:test"; test("error.cause", () => { const err = new Error("error 1"); const err2 = new Error("error 2", { cause: err }); expect( Bun.inspect(err2) - .replaceAll(import.meta.dir, "[dir]") - .replaceAll("\\", "/"), - ).toMatchSnapshot(); + .replaceAll("\\", "/") + .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]"), + ).toMatchInlineSnapshot(` +"1 | import { expect, test, describe, jest } from "bun:test"; +2 | +3 | test("error.cause", () => { +4 | const err = new Error("error 1"); +5 | const err2 = new Error("error 2", { cause: err }); + ^ +error: error 2 + at [dir]/inspect-error.test.js:5:16 + +1 | import { expect, test, describe, jest } from "bun:test"; +2 | +3 | test("error.cause", () => { +4 | const err = new Error("error 1"); + ^ +error: error 1 + at [dir]/inspect-error.test.js:4:15 +" +`); }); test("Error", () => { const err = new Error("my message"); expect( Bun.inspect(err) - .replaceAll(import.meta.dir, "[dir]") - .replaceAll("\\", "/"), - ).toMatchSnapshot(); + .replaceAll("\\", "/") + .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]"), + ).toMatchInlineSnapshot(` +"27 | " +28 | \`); +29 | }); +30 | +31 | test("Error", () => { +32 | const err = new Error("my message"); + ^ +error: my message + at [dir]/inspect-error.test.js:32:15 +" +`); }); test("BuildMessage", async () => { @@ -26,9 +55,19 @@ test("BuildMessage", async () => { } catch (e) { expect( Bun.inspect(e) - .replaceAll(import.meta.dir, "[dir]") - .replaceAll("\\", "/"), - ).toMatchSnapshot(); + .replaceAll("\\", "/") + .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]"), + ).toMatchInlineSnapshot(` +"2 | const duplicateConstDecl = 456; + ^ +error: "duplicateConstDecl" has already been declared + at [dir]/inspect-error-fixture-bad.js:2:7 + +1 | const duplicateConstDecl = 123; + ^ +note: "duplicateConstDecl" was originally declared here + at [dir]/inspect-error-fixture-bad.js:1:7" +`); } }); @@ -66,11 +105,23 @@ test("Error inside minified file (no color) ", () => { expect( normalizeError( Bun.inspect(e) - .replaceAll(import.meta.dir, "[dir]") .replaceAll("\\", "/") + .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]") .trim(), ), - ).toMatchSnapshot(); + ).toMatchInlineSnapshot(` +"21 | exports.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED=Z; +22 | exports.cache=function(a){return function(){var b=U.current;if(!b)return a.apply(null,arguments);var c=b.getCacheForType(V);b=c.get(a);void 0===b&&(b=W(),c.set(a,b));c=0;for(var f=arguments.length;c { normalizeError( stripANSIColors( Bun.inspect(e, { colors: true }) - .replaceAll(import.meta.dir, "[dir]") .replaceAll("\\", "/") + .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]") .trim(), ).trim(), ), - ).toMatchSnapshot(); + ).toMatchInlineSnapshot(` +"21 | exports.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED=Z; +22 | exports.cache=function(a){return function(){var b=U.current;if(!b)return a.apply(null,arguments);var c=b.getCacheForType(V);b=c.get(a);void 0===b&&(b=W(),c.set(a,b));c=0;for(var f=arguments.length;c { + const err = new Error("my message"); + expect( + require("util") + .inspect(err) + .replaceAll("\\", "/") + .replaceAll(import.meta.path.replaceAll("\\", "/"), "[file]"), + ).toMatchInlineSnapshot(` +"Error: my message + at ([file]:160:19)" +`); +}); + +describe("observable properties", () => { + for (let property of ["sourceURL", "line", "column"]) { + test(`${property} is observable`, () => { + const mock = jest.fn(); + const err = new Error("my message"); + Object.defineProperty(err, property, { + get: mock, + enumerable: true, + configurable: true, + }); + expect(mock).not.toHaveBeenCalled(); + Bun.inspect(err); + expect(mock).not.toHaveBeenCalled(); + }); + } +}); + +test("error.stack throwing an error doesn't lead to a crash", () => { + const err = new Error("my message"); + Object.defineProperty(err, "stack", { + get: () => { + throw new Error("my message"); + }, + enumerable: true, + configurable: true, + }); + expect(() => { + throw err; + }).toThrow(); +}); diff --git a/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js b/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js index 0c4bc55a79..5dffd034fa 100644 --- a/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js +++ b/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js @@ -656,14 +656,9 @@ test("no assertion failures 2", () => { // Prevent non-enumerable error properties from being printed. { - // TODO(bun): Make originalLine and originalColumn non-enumerable let err = new Error(); err.message = "foobar"; - let out = util - .inspect(err) - .replace(/\{\s*originalLine: .+\s*originalColumn: .+\s*\}/, "") - .trim() - .split("\n"); + let out = util.inspect(err).trim().split("\n"); assert.strictEqual(out[0], "Error: foobar"); assert(out.at(-1).startsWith(" at "), 'Expected "' + out.at(-1) + '" to start with " at "'); // Reset the error, the stack is otherwise not recreated. @@ -671,21 +666,13 @@ test("no assertion failures 2", () => { err.message = "foobar"; err.name = "Unique"; Object.defineProperty(err, "stack", { value: err.stack, enumerable: true }); - out = util - .inspect(err) - .replace(/\{\s*originalLine: .+\s*originalColumn: .+\s*\}/, "") - .trim() - .split("\n"); + out = util.inspect(err).trim().split("\n"); assert.strictEqual(out[0], "Unique: foobar"); assert(out.at(-1).startsWith(" at "), 'Expected "' + out.at(-1) + '" to start with " at "'); err.name = "Baz"; - out = util - .inspect(err) - .replace(/\n\s*originalLine: .+\s*originalColumn: .+/, "") - .trim() - .split("\n"); + out = util.inspect(err).trim().split("\n"); assert.strictEqual(out[0], "Unique: foobar"); - assert.strictEqual(out.at(-2), " name: 'Baz',"); + assert.strictEqual(out.at(-2), " name: 'Baz'"); assert.strictEqual(out.at(-1), "}"); }