src: fixup Error.stackTraceLimit during snapshot building

joyeecheung · targos · commit 1af5ad61ca54 · 2024-10-04T19:20:19.000+02:00
When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js
@@ -23,17 +23,18 @@ const { emitWarningSync } = require('internal/process/warning');
 const {
   initializeCallbacks,
   namespace: {
-    addSerializeCallback,
     addDeserializeCallback,
     isBuildingSnapshot,
   },
+  addAfterUserSerializeCallback,
 } = require('internal/v8/startup_snapshot');
 
 const {
   prepareMainThreadExecution,
 } = require('internal/process/pre_execution');
 
 const path = require('path');
+const { getOptionValue } = require('internal/options');
 
 const supportedModules = new SafeSet(new SafeArrayIterator([
   // '_http_agent',
@@ -140,22 +141,56 @@ function main() {
   prepareMainThreadExecution(false, false);
   initializeCallbacks();
 
-  let stackTraceLimitDesc;
-  addDeserializeCallback(() => {
-    if (stackTraceLimitDesc !== undefined) {
-      ObjectDefineProperty(Error, 'stackTraceLimit', stackTraceLimitDesc);
+  // In a context created for building snapshots, V8 does not install Error.stackTraceLimit and as
+  // a result, if an error is created during the snapshot building process, error.stack would be
+  // undefined. To prevent users from tripping over this, install Error.stackTraceLimit based on
+  // --stack-trace-limit by ourselves (which defaults to 10).
+  // See https://chromium-review.googlesource.com/c/v8/v8/+/3319481
+  const initialStackTraceLimitDesc = {
+    value: getOptionValue('--stack-trace-limit'),
+    configurable: true,
+    writable: true,
+    enumerable: true,
+    __proto__: null,
+  };
+  ObjectDefineProperty(Error, 'stackTraceLimit', initialStackTraceLimitDesc);
+
+  let stackTraceLimitDescToRestore;
+  // Error.stackTraceLimit needs to be removed during serialization, because when V8 deserializes
+  // the snapshot, it expects Error.stackTraceLimit to be unset so that it can install it as a new property
+  // using the value of --stack-trace-limit.
+  addAfterUserSerializeCallback(() => {
+    const desc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit');
+
+    // If it's modified by users, emit a warning.
+    if (desc && (
+      desc.value !== initialStackTraceLimitDesc.value ||
+        desc.configurable !== initialStackTraceLimitDesc.configurable ||
+        desc.writable !== initialStackTraceLimitDesc.writable ||
+        desc.enumerable !== initialStackTraceLimitDesc.enumerable
+    )) {
+      process._rawDebug('Error.stackTraceLimit has been modified by the snapshot builder script.');
+      // We want to use null-prototype objects to not rely on globally mutable
+      // %Object.prototype%.
+      if (desc.configurable) {
+        stackTraceLimitDescToRestore = desc;
+        ObjectSetPrototypeOf(stackTraceLimitDescToRestore, null);
+        process._rawDebug('It will be preserved after snapshot deserialization and override ' +
+                          '--stack-trace-limit passed into the deserialized application.\n' +
+                          'To allow --stack-trace-limit override in the deserialized application, ' +
+                          'delete Error.stackTraceLimit.');
+      } else {
+        process._rawDebug('It is not configurable and will crash the application upon deserialization.\n' +
+                          'To fix the error, make Error.stackTraceLimit configurable.');
+      }
     }
+
+    delete Error.stackTraceLimit;
   });
-  addSerializeCallback(() => {
-    stackTraceLimitDesc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit');
 
-    if (stackTraceLimitDesc !== undefined) {
-      // We want to use null-prototype objects to not rely on globally mutable
-      // %Object.prototype%.
-      ObjectSetPrototypeOf(stackTraceLimitDesc, null);
-      process._rawDebug('Deleting Error.stackTraceLimit from the snapshot. ' +
-                        'It will be re-installed after deserialization');
-      delete Error.stackTraceLimit;
+  addDeserializeCallback(() => {
+    if (stackTraceLimitDescToRestore) {
+      ObjectDefineProperty(Error, 'stackTraceLimit', stackTraceLimitDescToRestore);
     }
   });
 
diff --git a/lib/internal/v8/startup_snapshot.js b/lib/internal/v8/startup_snapshot.js
@@ -58,11 +58,16 @@ function addDeserializeCallback(callback, data) {
 }
 
 const serializeCallbacks = [];
+const afterUserSerializeCallbacks = [];  // Callbacks to run after user callbacks
 function runSerializeCallbacks() {
   while (serializeCallbacks.length > 0) {
     const { 0: callback, 1: data } = serializeCallbacks.shift();
     callback(data);
   }
+  while (afterUserSerializeCallbacks.length > 0) {
+    const { 0: callback, 1: data } = afterUserSerializeCallbacks.shift();
+    callback(data);
+  }
 }
 
 function addSerializeCallback(callback, data) {
@@ -71,6 +76,10 @@ function addSerializeCallback(callback, data) {
   serializeCallbacks.push([callback, data]);
 }
 
+function addAfterUserSerializeCallback(callback, data) {
+  afterUserSerializeCallbacks.push([callback, data]);
+}
+
 function initializeCallbacks() {
   // Only run the serialize callbacks in snapshot building mode, otherwise
   // they throw.
@@ -117,4 +126,5 @@ module.exports = {
     setDeserializeMainFunction,
     isBuildingSnapshot,
   },
+  addAfterUserSerializeCallback,
 };
diff --git a/test/fixtures/snapshot/error-stack.js b/test/fixtures/snapshot/error-stack.js
@@ -0,0 +1,24 @@
+
+const {
+  setDeserializeMainFunction,
+} = require('v8').startupSnapshot;
+
+console.log(`During snapshot building, Error.stackTraceLimit =`, Error.stackTraceLimit);
+console.log(getError('During snapshot building', 30));
+
+setDeserializeMainFunction(() => {
+  console.log(`After snapshot deserialization, Error.stackTraceLimit =`, Error.stackTraceLimit);
+  console.log(getError('After snapshot deserialization', 30));
+});
+
+function getError(message, depth) {
+  let counter = 1;
+  function recurse() {
+    if (counter++ < depth) {
+      return recurse();
+    }
+    const error = new Error(message);
+    return error.stack;
+  }
+  return recurse();
+}
diff --git a/test/fixtures/snapshot/mutate-error-stack-trace-limit.js b/test/fixtures/snapshot/mutate-error-stack-trace-limit.js
@@ -0,0 +1,44 @@
+
+const {
+  addSerializeCallback,
+  setDeserializeMainFunction,
+} = require('v8').startupSnapshot;
+const assert = require('assert');
+
+if (process.env.TEST_IN_SERIALIZER) {
+  addSerializeCallback(checkMutate);
+} else {
+  checkMutate();
+}
+
+function checkMutate() {
+  // Check that mutation to Error.stackTraceLimit is effective in the snapshot
+  // builder script.
+  assert.strictEqual(typeof Error.stackTraceLimit, 'number');
+  Error.stackTraceLimit = 0;
+  assert.strictEqual(getError('', 30), 'Error');
+}
+
+setDeserializeMainFunction(() => {
+  // Check that the mutation is preserved in the deserialized main function.
+  assert.strictEqual(Error.stackTraceLimit, 0);
+  assert.strictEqual(getError('', 30), 'Error');
+
+  // Check that it can still be mutated.
+  Error.stackTraceLimit = 10;
+  const error = getError('', 30);
+  const matches = [...error.matchAll(/at recurse/g)];
+  assert.strictEqual(matches.length, 10);
+});
+
+function getError(message, depth) {
+  let counter = 1;
+  function recurse() {
+    if (counter++ < depth) {
+      return recurse();
+    }
+    const error = new Error(message);
+    return error.stack;
+  }
+  return recurse();
+}
diff --git a/test/parallel/test-snapshot-stack-trace-limit-mutation.js b/test/parallel/test-snapshot-stack-trace-limit-mutation.js
@@ -0,0 +1,46 @@
+'use strict';
+
+// This tests mutation to Error.stackTraceLimit in both the snapshot builder script
+// and the snapshot main script work.
+
+require('../common');
+const assert = require('assert');
+const tmpdir = require('../common/tmpdir');
+const fixtures = require('../common/fixtures');
+const { spawnSyncAndAssert, spawnSyncAndExitWithoutError } = require('../common/child_process');
+
+const blobPath = tmpdir.resolve('snapshot.blob');
+
+function test(additionalArguments = [], additionalEnv = {}) {
+  tmpdir.refresh();
+  // Check the mutation works without --stack-trace-limit.
+  spawnSyncAndAssert(process.execPath, [
+    ...additionalArguments,
+    '--snapshot-blob',
+    blobPath,
+    '--build-snapshot',
+    fixtures.path('snapshot', 'mutate-error-stack-trace-limit.js'),
+  ], {
+    cwd: tmpdir.path,
+    env: {
+      ...process.env,
+      ...additionalEnv,
+    }
+  }, {
+    stderr(output) {
+      assert.match(output, /Error\.stackTraceLimit has been modified by the snapshot builder script/);
+      assert.match(output, /It will be preserved after snapshot deserialization/);
+    }
+  });
+  spawnSyncAndExitWithoutError(process.execPath, [
+    '--snapshot-blob',
+    blobPath,
+  ], {
+    cwd: tmpdir.path
+  });
+}
+
+test();
+test([], { TEST_IN_SERIALIZER: 1 });
+test(['--stack-trace-limit=50']);
+test(['--stack-trace-limit=50'], { TEST_IN_SERIALIZER: 1 });
diff --git a/test/parallel/test-snapshot-stack-trace-limit.js b/test/parallel/test-snapshot-stack-trace-limit.js
@@ -0,0 +1,46 @@
+'use strict';
+
+// This tests Error.stackTraceLimit is fixed up for snapshot-building contexts,
+// and can be restored after deserialization.
+
+require('../common');
+const assert = require('assert');
+const tmpdir = require('../common/tmpdir');
+const fixtures = require('../common/fixtures');
+const { spawnSyncAndAssert } = require('../common/child_process');
+
+tmpdir.refresh();
+const blobPath = tmpdir.resolve('snapshot.blob');
+{
+  spawnSyncAndAssert(process.execPath, [
+    '--stack-trace-limit=50',
+    '--snapshot-blob',
+    blobPath,
+    '--build-snapshot',
+    fixtures.path('snapshot', 'error-stack.js'),
+  ], {
+    cwd: tmpdir.path
+  }, {
+    stdout(output) {
+      assert.match(output, /During snapshot building, Error\.stackTraceLimit = 50/);
+      const matches = [...output.matchAll(/at recurse/g)];
+      assert.strictEqual(matches.length, 30);
+    }
+  });
+}
+
+{
+  spawnSyncAndAssert(process.execPath, [
+    '--stack-trace-limit=20',
+    '--snapshot-blob',
+    blobPath,
+  ], {
+    cwd: tmpdir.path
+  }, {
+    stdout(output) {
+      assert.match(output, /After snapshot deserialization, Error\.stackTraceLimit = 20/);
+      const matches = [...output.matchAll(/at recurse/g)];
+      assert.strictEqual(matches.length, 20);
+    }
+  });
+}
