Fix segfault when printing an AggregateError without an errors property

src/jsc/JSValue.rs

@@ -375,19 +375,6 @@ impl JSValue {
         }
         JSC__JSValue__isAggregateError(self, global)
     }
-    /// `JSValue.getErrorsProperty(globalObject)`. Returns the
-    /// own `errors` data property via `JSObject::getDirect` — no prototype
-    /// walk, no getters invoked, nothrow. Used for `AggregateError.errors`.
-    #[inline]
-    pub(crate) fn get_errors_property(self, global: &JSGlobalObject) -> JSValue {
-        unsafe extern "C" {
-            safe fn JSC__JSValue__getErrorsProperty(
-                this: JSValue,
-                global: &JSGlobalObject,
-            ) -> JSValue;
-        }
-        JSC__JSValue__getErrorsProperty(self, global)
-    }
     /// `JSValue.isTerminationException()` — true if this
     /// value is the VM's termination-exception sentinel.
     #[inline]

src/jsc/VirtualMachine.rs

@@ -5149,7 +5149,7 @@ impl VirtualMachine {
         &mut self,
         value: JSValue,
         exception: Option<&Exception>,
-        exception_list: Option<&mut ExceptionList>,
+        mut exception_list: Option<&mut ExceptionList>,
         formatter: &mut crate::console_object::Formatter,
         writer: &mut bun_core::io::Writer,
         allow_ansi_color: bool,
@@ -5160,7 +5160,18 @@ impl VirtualMachine {
         // once the AggregateError branch is taken).
         let global_ref = self.global();
 
-        if value.is_aggregate_error(global_ref) {
+        let errors = if value.is_aggregate_error(global_ref) {
+            match value.fast_get(global_ref, jsc::BuiltinName::errors) {
+                Ok(errors) => errors,
+                Err(_) => {
+                    global_ref.clear_exception();
+                    None
+                }
+            }
+        } else {
+            None
+        };
+        if let Some(errors) = errors {
             // Note: `JSValue::for_each` takes a C-ABI fn
             // pointer + erased ctx, so thread the captures through a struct.
             // The C trampoline erases lifetimes via `*mut c_void`; round-trip
@@ -5172,6 +5183,7 @@ impl VirtualMachine {
                 exception_list: *mut ExceptionList,
                 allow_ansi_color: bool,
                 allow_side_effects: bool,
+                printed_member: bool,
             }
             extern "C" fn agg_iter(
                 _vm: *mut crate::VM,
@@ -5196,6 +5208,7 @@ impl VirtualMachine {
                 // SAFETY: `ctx.writer` borrows the caller's stack local,
                 // live across the synchronous `for_each` call.
                 let writer = unsafe { &mut *ctx.writer };
+                ctx.printed_member = true;
                 vm.print_errorlike_object(
                     next_value,
                     None,
@@ -5207,25 +5220,27 @@ impl VirtualMachine {
                 );
             }
             let mut ctx = AggCtx {
-                formatter: std::ptr::from_mut(formatter),
-                writer: std::ptr::from_mut(writer),
+                formatter: std::ptr::from_mut(&mut *formatter),
+                writer: std::ptr::from_mut(&mut *writer),
                 exception_list: exception_list
-                    .map(std::ptr::from_mut::<ExceptionList>)
-                    .unwrap_or(core::ptr::null_mut()),
+                    .as_deref_mut()
+                    .map_or(core::ptr::null_mut(), std::ptr::from_mut::<ExceptionList>),
                 allow_ansi_color,
                 allow_side_effects,
+                printed_member: false,
             };
-            // `getErrorsProperty` is
-            // `getDirect` (own data prop, nothrow); `for_each` may throw, in
-            // which case the error is swallowed.
-            let errors = value.get_errors_property(global_ref);
-            let _ = errors.for_each(global_ref, (&raw mut ctx).cast(), agg_iter);
-            return;
+            if errors
+                .for_each(global_ref, (&raw mut ctx).cast(), agg_iter)
+                .is_err()
+            {
+                global_ref.clear_exception();
+            }
+            if ctx.printed_member {
+                return;
+            }
+            // `errors` is empty or not iterable: print the AggregateError itself.
         }
 
-        // Note: reborrow so the add-to-error-list tail can still see it after
-        // `print_error_from_maybe_private_data`.
-        let mut exception_list = exception_list;
         let was_internal = self.print_error_from_maybe_private_data(
             value,
             exception_list.as_deref_mut(),

src/jsc/bindings/bindings.cpp

@@ -4857,12 +4857,6 @@ CPP_DECL double Bun__JSValue__toNumber(JSC::EncodedJSValue JSValue0, JSC::JSGlob
     return value.toInt32(arg1);
 }
 
-JSC::EncodedJSValue JSC__JSValue__getErrorsProperty(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* global)
-{
-    JSC::JSObject* obj = JSC::JSValue::decode(JSValue0).getObject();
-    return JSC::JSValue::encode(obj->getDirect(global->vm(), global->vm().propertyNames->errors));
-}
-
 [[ZIG_EXPORT(nothrow)]] JSC::EncodedJSValue JSC__JSValue__jsTDZValue()
 {
     return JSC::JSValue::encode(JSC::jsTDZValue());
@@ -5316,6 +5310,7 @@ enum class BuiltinNamesMap : uint8_t {
     type,
     signal,
     cmd,
+    errors,
     // Private names below: set by builtins via $putByIdDirectPrivate, unreachable from user code.
     internal,
     sharedFd,
@@ -5398,6 +5393,9 @@ static inline const JSC::Identifier& builtinNameMap(JSC::VM& vm, unsigned char n
     case BuiltinNamesMap::cmd: {
         return clientData->builtinNames().cmdPublicName();
     }
+    case BuiltinNamesMap::errors: {
+        return vm.propertyNames->errors;
+    }
     case BuiltinNamesMap::internal: {
         return clientData->builtinNames().internalPrivateName();
     }

src/jsc/lib.rs

@@ -921,6 +921,7 @@ pub enum BuiltinName {
     type_,
     signal,
     cmd,
+    errors,
     /// Private name (`$internal` in builtins); user code cannot set it.
     internal,
     /// Private name (`$sharedFd` in builtins); user code cannot set it.

test/internal/source-lints/jsresult-swallow.inventory.json

@@ -1,7 +1,7 @@
 {
   "src/jsc/VirtualMachine.rs": {
     "JsResult collapsed on the spot": 1,
-    "discarded result of a call that enters script": 2
+    "discarded result of a call that enters script": 1
   },
   "src/jsc/web_worker.rs": {
     "taken exception discarded": 1

test/js/bun/util/inspect-error.test.js

@@ -327,3 +327,132 @@ describe("source map remapping of the printed stack", () => {
     expect(exitCode).toBe(1);
   });
 });
+
+// The printer replaces an AggregateError with the members of its `errors`
+// property. When there is nothing to walk it has to print the AggregateError
+// itself. A deleted `errors` used to crash the process (the empty value was
+// passed to the iteration, which read it as a cell at address 0), an accessor
+// was passed to it as well, the other shapes printed nothing, and a
+// non-iterable `errors` made console.error throw.
+describe.concurrent("AggregateError whose errors cannot be walked", () => {
+  async function run(cmd, files) {
+    using dir = tempDir("inspect-aggregate-error", files ?? {});
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), ...cmd],
+      cwd: String(dir),
+      env: bunEnv,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    return { stdout, stderr, exitCode };
+  }
+
+  const header = "AggregateError: outer message";
+  const make = 'const e = new AggregateError([new Error("inner")], "outer message");';
+  const shapes = {
+    "errors was deleted": "delete e.errors;",
+    "errors is an empty array": "e.errors = [];",
+    "errors is not iterable": "e.errors = {};",
+    "errors is null": "e.errors = null;",
+    "errors is a primitive": "e.errors = 1;",
+    "the errors getter throws": 'Object.defineProperty(e, "errors", { get() { throw new Error("getter"); } });',
+  };
+
+  for (const [name, shape] of Object.entries(shapes)) {
+    test(`console.error: ${name}`, async () => {
+      const { stdout, stderr, exitCode } = await run([
+        "-e",
+        `${make} ${shape} console.error(e); console.log("after");`,
+      ]);
+      expect(stderr).toContain(header);
+      expect(stdout).toBe("after\n");
+      expect(exitCode).toBe(0);
+    });
+
+    test(`uncaught: ${name}`, async () => {
+      const { stderr, exitCode } = await run(["-e", `${make} ${shape} throw e;`]);
+      expect(stderr).toContain(header);
+      expect(exitCode).toBe(1);
+    });
+  }
+
+  test("Bun.inspect: errors was deleted", async () => {
+    const { stdout, stderr, exitCode } = await run([
+      "-e",
+      `${make} delete e.errors; console.log(JSON.stringify(Bun.inspect(e)));`,
+    ]);
+    expect(JSON.parse(stdout)).toContain(header);
+    expect(stderr).toBe("");
+    expect(exitCode).toBe(0);
+  });
+
+  test("errors is an accessor: the members it returns are printed", async () => {
+    const { stdout, stderr, exitCode } = await run([
+      "-e",
+      `${make}
+       Object.defineProperty(e, "errors", { get() { return [new Error("from the getter")]; } });
+       console.error(e);
+       console.log("after");`,
+    ]);
+    expect(stderr).toContain("error: from the getter");
+    expect(stderr).not.toContain(header);
+    expect(stdout).toBe("after\n");
+    expect(exitCode).toBe(0);
+  });
+
+  test("members are still printed in place of the AggregateError", async () => {
+    const { stderr, exitCode } = await run([
+      "-e",
+      'throw new AggregateError([new Error("first member"), new TypeError("second member")], "outer message");',
+    ]);
+    expect(stderr).toContain("error: first member");
+    expect(stderr).toContain("TypeError: second member");
+    expect(stderr).not.toContain(header);
+    expect(exitCode).toBe(1);
+  });
+
+  // Without any user code: a module with two or more build errors rejects its
+  // first load with an AggregateError of BuildMessages. JSC settles every later
+  // load of it from the module registry with a copy made by
+  // JSModuleLoader::duplicateError, which keeps the error type and message but
+  // not the `errors` property.
+  const broken = {
+    "broken.ts": "export function f() {\n  const v = {b: {},),r,};\n}\n",
+  };
+
+  test("the error a second load of a module that failed to build rejects with", async () => {
+    const { stdout, stderr, exitCode } = await run(["main.ts"], {
+      ...broken,
+      "main.ts": `
+        const seen = [];
+        for (let i = 0; i < 2; i++) {
+          try {
+            await import("./broken.ts");
+          } catch (e) {
+            seen.push(e.constructor.name);
+            if (i === 1) console.error(e);
+          }
+        }
+        console.log(JSON.stringify(seen));
+      `,
+    });
+    expect(stdout).toBe('["AggregateError","AggregateError"]\n');
+    expect(stderr).toContain("broken.ts");
+    expect(exitCode).toBe(0);
+  });
+
+  // https://github.com/oven-sh/bun/issues/36963
+  test("bun test: two files import a module that failed to build", async () => {
+    const importer = 'import { f } from "./broken.ts";\nf();\n';
+    const { stderr, exitCode } = await run(["test", "./a.test.ts", "./b.test.ts"], {
+      ...broken,
+      "a.test.ts": importer,
+      "b.test.ts": importer,
+    });
+    expect(stderr).toContain("a.test.ts:");
+    expect(stderr).toContain("b.test.ts:");
+    expect(stderr).toContain("across 2 files");
+    expect(exitCode).toBe(1);
+  });
+});