From f3498cdd2105da77f5746531d2c315e7a205a5b5 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 25 Sep 2024 14:51:41 +0200 Subject: [PATCH] src: fixup Error.stackTraceLimit during snapshot building MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/55121 Fixes: https://github.com/nodejs/node/issues/55100 Reviewed-By: Michaël Zasso Reviewed-By: Chengzhong Wu --- lib/internal/main/mksnapshot.js | 63 ++++++++++++++----- lib/internal/v8/startup_snapshot.js | 10 +++ test/fixtures/snapshot/error-stack.js | 24 +++++++ .../mutate-error-stack-trace-limit.js | 44 +++++++++++++ ...est-snapshot-stack-trace-limit-mutation.js | 46 ++++++++++++++ .../test-snapshot-stack-trace-limit.js | 46 ++++++++++++++ 6 files changed, 219 insertions(+), 14 deletions(-) create mode 100644 test/fixtures/snapshot/error-stack.js create mode 100644 test/fixtures/snapshot/mutate-error-stack-trace-limit.js create mode 100644 test/parallel/test-snapshot-stack-trace-limit-mutation.js create mode 100644 test/parallel/test-snapshot-stack-trace-limit.js diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index 0d00acd6a168ba..63df8c50087aad 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -23,10 +23,10 @@ const { emitWarningSync } = require('internal/process/warning'); const { initializeCallbacks, namespace: { - addSerializeCallback, addDeserializeCallback, isBuildingSnapshot, }, + addAfterUserSerializeCallback, } = require('internal/v8/startup_snapshot'); const { @@ -34,6 +34,7 @@ const { } = 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 index 7c789577aec969..01204b96239406 100644 --- 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 new file mode 100644 index 00000000000000..96afaec22521ee --- /dev/null +++ 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 new file mode 100644 index 00000000000000..e9d704ceb32f23 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..fe6c9fbd45ca0b --- /dev/null +++ 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 new file mode 100644 index 00000000000000..9ba760a9f2373e --- /dev/null +++ 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); + } + }); +}