Error.appendStackTrace: fix abort with unset stackTraceLimit, assertion on materialized errors, and self-append use-after-free

src/jsc/bindings/FormatStackTraceForJS.cpp

@@ -669,8 +669,23 @@ JSC_DEFINE_HOST_FUNCTION(errorConstructorFuncAppendStackTrace, (JSC::JSGlobalObj
         return {};
     }
 
+    // appendVector() is not self-append safe, and the clear() below would then drop the trace.
+    if (source == destination) {
+        return JSC::JSValue::encode(jsUndefined());
+    }
+
+    // Materializing .stack dropped the frames; frames installed now would replace it in the native printer (same no-op as Bun__attachAsyncStackFromPromise).
+    if (destination->hasMaterializedErrorInfo()) {
+        return JSC::JSValue::encode(jsUndefined());
+    }
+
     if (!destination->stackTrace()) {
-        destination->captureStackTrace(vm, globalObject, 1);
+        // captureStackTrace() unwraps stackTraceLimit(), which is empty when Error.stackTraceLimit is not a number.
+        if (globalObject->stackTraceLimit()) {
+            destination->captureStackTrace(vm, globalObject, 1);
+        } else {
+            destination->setStackFrames(vm, {});
+        }
     }
 
     if (source->stackTrace()) {

test/js/node/v8/capture-stack-trace.test.js

@@ -1,7 +1,7 @@
 import { nativeFrameForTesting } from "bun:internal-for-testing";
 import { noInline } from "bun:jsc";
 import { afterEach, expect, mock, test } from "bun:test";
-import { bunEnv, bunExe, tempDir } from "harness";
+import { bunEnv, bunExe, isASAN, tempDir } from "harness";
 const origPrepareStackTrace = Error.prepareStackTrace;
 afterEach(() => {
   Error.prepareStackTrace = origPrepareStackTrace;
@@ -993,6 +993,198 @@ test("captureStackTrace does not crash when stackTraceLimit is non-numeric", ()
   }
 });
 
+test("Error.appendStackTrace moves the source's frames behind the destination's own", () => {
+  function inner() {
+    try {
+      null();
+    } catch (e) {
+      return e;
+    }
+  }
+  function makeDestination() {
+    const error = new Error("destination");
+    error.name = "DestinationError";
+    return error;
+  }
+  const source = inner();
+  const destination = makeDestination();
+  Error.appendStackTrace(source, destination);
+
+  const lines = destination.stack.split("\n");
+  const destinationFrame = lines.findIndex(line => line.includes("at makeDestination"));
+  const sourceFrame = lines.findIndex(line => line.includes("at inner"));
+  expect(lines[0]).toBe("DestinationError: destination");
+  expect(destinationFrame).toBeGreaterThan(0);
+  expect(sourceFrame).toBeGreaterThan(destinationFrame);
+  // The frames are moved, not copied: the source has none left.
+  expect(source.stack).toBeUndefined();
+});
+
+// The rest of these can abort the process (or trip ASAN) when they fail, so
+// each runs its scenario in a child.
+test.concurrent("Error.appendStackTrace does not abort when stackTraceLimit is non-numeric or deleted", async () => {
+  const src = `
+    class Source {
+      constructor() {
+        this.error = new Error("source");
+      }
+    }
+    const source = new Source().error;
+
+    Error.stackTraceLimit = "foo";
+    const destination = new Error("destination");
+    Error.appendStackTrace(source, destination);
+    Error.appendStackTrace(new Error("a"), new Error("b"));
+
+    Error.stackTraceLimit = undefined;
+    Error.appendStackTrace(new Error("c"), new Error("d"));
+
+    delete Error.stackTraceLimit;
+    Error.appendStackTrace(new Error("e"), new Error("f"));
+
+    process.stdout.write(JSON.stringify({ appended: destination.stack.includes("at new Source") }));
+  `;
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "-e", src],
+    env: bunEnv,
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  expect({ stdout, stderr, exitCode }).toEqual({ stdout: JSON.stringify({ appended: true }), stderr: "", exitCode: 0 });
+});
+
+test.concurrent("Error.appendStackTrace is a no-op once the destination's .stack has been materialized", async () => {
+  // Without the guard, release builds install new frames on the destination
+  // (so the native printer shows the appendStackTrace call site instead of the
+  // frames .stack reports) and empty the source; debug builds also assert when
+  // GC finalizes the destination, which is what the new Function sources and
+  // the Bun.gc() calls provoke.
+  const src = `
+    function makeDestination() {
+      const error = new Error("destination");
+      error.stack;
+      return error;
+    }
+    function appendAll(destination) {
+      let source;
+      for (let i = 0; i < 100; i++) {
+        source = new Function("return new Error('source')")();
+        Error.appendStackTrace(source, destination);
+      }
+      return source;
+    }
+    const destination = makeDestination();
+    const stack = destination.stack;
+    const lastSource = appendAll(destination);
+    const printed = Bun.inspect(destination);
+    Bun.gc(true);
+    Bun.gc(true);
+    process.stdout.write(JSON.stringify({
+      unchanged: destination.stack === stack,
+      printedOwnFrame: printed.includes("at makeDestination"),
+      printedAppendFrame: printed.includes("at appendAll"),
+      sourceIntact: typeof lastSource.stack === "string",
+    }));
+  `;
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "-e", src],
+    env: bunEnv,
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  expect({ stdout, stderr, exitCode }).toEqual({
+    stdout: JSON.stringify({ unchanged: true, printedOwnFrame: true, printedAppendFrame: false, sourceIntact: true }),
+    stderr: "",
+    exitCode: 0,
+  });
+});
+
+test.concurrent(
+  "Error.appendStackTrace onto errors materialized through .sourceURL does not assert when GC finalizes them",
+  async () => {
+    // Reading any of the lazily materialized properties (.stack, .line,
+    // .column, .sourceURL) discards the native frames. The errors are created
+    // inside eval'd functions so that each iteration's frames point at code GC
+    // can reclaim, which is what makes finalizeUnconditionally look at them.
+    // The unmaterialized sources (c) keeping their frames is the part release
+    // builds can observe.
+    const src = `
+      const keep = [];
+      const sources = [];
+      for (let i = 0; i < 200; i++) {
+        eval(\`(function inner\${i}() {
+          const a = new Error();
+          const b = new Error();
+          a.sourceURL;
+          b.sourceURL;
+          Error.appendStackTrace(a, b);
+          keep.push(b);
+          const c = new Error();
+          const d = new Error();
+          d.sourceURL;
+          Error.appendStackTrace(c, d);
+          keep.push(d);
+          sources.push(c);
+        })();\`);
+      }
+      Bun.gc(true);
+      Bun.gc(true);
+      process.stdout.write(JSON.stringify({ sourcesIntact: sources.every(error => typeof error.stack === "string") }));
+    `;
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "-e", src],
+      env: bunEnv,
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect({ stdout, stderr, exitCode }).toEqual({
+      stdout: JSON.stringify({ sourcesIntact: true }),
+      stderr: "",
+      exitCode: 0,
+    });
+  },
+);
+
+test.concurrent(
+  "Error.appendStackTrace with the same error as source and destination leaves its trace alone",
+  async () => {
+    // The trace needs enough frames that appending it to itself reallocates the
+    // vector; the default stackTraceLimit of 10 is plenty once f() has recursed.
+    const src = `
+      function f(n) {
+        if (n > 0) return f(n - 1) + 1;
+        try {
+          null();
+        } catch (e) {
+          Error.appendStackTrace(e, e);
+          // Without the guard the trace ends up empty and .stack is undefined.
+          process.stdout.write(JSON.stringify({ frames: String(e.stack).split("\\n").filter(line => line.includes("at f ")).length }));
+        }
+        return 0;
+      }
+      f(64);
+    `;
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "-e", src],
+      // WTF allocations normally come from bmalloc, where ASAN cannot see the
+      // freed buffer; Malloc=1 routes them through the system allocator.
+      // detect_leaks=0 keeps LeakSanitizer from reporting JSC's exit-time
+      // allocations under that allocator, and symbolize=0 keeps a failing child
+      // from spending seconds symbolizing the report.
+      env: isASAN
+        ? {
+            ...bunEnv,
+            Malloc: "1",
+            ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0", "symbolize=0"].filter(Boolean).join(":"),
+          }
+        : bunEnv,
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect({ stdout, stderr, exitCode }).toEqual({ stdout: JSON.stringify({ frames: 10 }), stderr: "", exitCode: 0 });
+  },
+);
+
 test("Error.stackTraceLimit default matches the limit captureStackTrace applies", async () => {
   // Run in a fresh process so nothing has written to Error.stackTraceLimit yet.
   const src = `