util: safely inspect getter errors whose message throws #60684

yvele · 2025-11-11T19:27:57Z

yvele · 2025-11-11T19:29:37Z

Note that I based the behaviour from @legendecas comment in this related PR: #60139 (review)

I believe this is an improvement to the util.inspect that avoids throwing in debugging inspection.

aduh95 · 2025-11-12T08:53:33Z

lib/internal/util/inspect.js

+        let messageSuffix;
+        try {
+          // Error message itself may be a getter
+          messageSuffix = ` (${err.message})`;
+        } catch {
+          messageSuffix = '';
+        }
+        const message = `<Inspection threw${messageSuffix}>`;


wdyt of doing this instead?

Suggested change

let messageSuffix;

try {

// Error message itself may be a getter

messageSuffix = ` (${err.message})`;

} catch {

messageSuffix = '';

}

const message = `<Inspection threw${messageSuffix}>`;

let message = '<Inspection threw>';

if (isErrorStackTraceLimitWritable()) {

const stackTraceLimit = Error.stackTraceLimit;

Error.stackTraceLimit = 0;

try { message = `<Inspection threw (${inspect(err)})>`; }

catch {}

finally { Error.stackTraceLimit = stackTraceLimit; }

}

This way the "Inspection threw" text is in two places, which seems strictly worse to me

The suggestion is not about messageSuffix, it's about using inspect

Can we first agree on the behavior? ~~Any value that is thrown and has a message property which is a string will be displayed as the "inspection threw" detail.~~

~~I’ve currently implemented a typeof message === "string" check, should we also consider testing other types, or just display the value as-is (excluding null and undefined)?~~

My suggestion is to use inspect(err) (or at least, reuse the logic from another function in this file) instead of introducing ad-hoc checks that are not consistent with the rest of codebase

Ok I've committed with messageSuffix to avoid duplicating "Inspection threw" text

aduh95 · 2025-11-12T08:55:21Z

test/parallel/test-util-inspect.js

+{
+  const error = {
+    // The message itself is a getter that throws
+    get message() { throw new Error('Oops'); }


Can you add test cases for throwing null and other uncommon values?

I've added throwing null, undefined, {}, true, Error can you think about something else? 🤔

test/parallel/test-util-inspect.js

yvele · 2025-11-12T12:23:36Z

test/parallel/test-util-inspect.js

+    // Heh? I don't know how to override the error stakc to make it predictable
+    '      [Symbol(Symbol.species)]: ' +
+    "[Getter: <Inspection threw (TypeError: Symbol.prototype.toString requires that 'this' be a Symbol\n" +
+    '    at Bar.toString (<anonymous>)\n' +
+    '    at formatPrimitive (node:internal/util/inspect:2246:13)\n' +
+    '    at formatProperty (node:internal/util/inspect:2556:29)\n' +
+    '    at addPrototypeProperties (node:internal/util/inspect:1010:21)\n' +
+    '    at getConstructorName (node:internal/util/inspect:918:11)\n' +
+    '    at formatRaw (node:internal/util/inspect:1194:23)\n' +
+    '    at formatValue (node:internal/util/inspect:1184:10)\n' +
+    '    at formatProperty (node:internal/util/inspect:2536:11)\n' +
+    '    at formatRaw (node:internal/util/inspect:1429:9)\n' +
+    '    at formatValue (node:internal/util/inspect:1184:10))>]\n' +


Hum, do you know if it's because isErrorStackTraceLimitWritable() returns false, or if it's because the stack was computed before we try to set stackTraceLimit to 0? In any case, if that's indeed the output, we can use RegExp.escape and assert.match to validate only what we need (i.e. not the line numbers of internal modules)

Looks like Error.stackTraceLimit = 0; in implementation does not work and the whole stack is returned everytime 🤔 In my tests (an potentially in real cases) errors are created before being thrown.

See other tests: https://github.com/yvele/node/blob/9b05e4ee82d3627deb9647c094a315610f1cc855/test/parallel/test-util-inspect.js#L2597-L2608

assert.strictEqual( inspect(thrower, { getters: true }), '{\n' + ' foo: [Getter: <Inspection threw (Error: Oops\n' + ' at get foo (/foo/node_modules/foo.js:2:7)\n' + ' at get bar (/foo/node_modules/bar.js:827:30))>]\n' + '}', );

Also stackTraceLimit is serialized https://github.com/yvele/node/blob/9b05e4ee82d3627deb9647c094a315610f1cc855/test/parallel/test-util-inspect.js#L2653-L2662

assert.strictEqual( inspect({ get foo() { throw Error; } }, { getters: true }), '{\n' + ' foo: [Getter: <Inspection threw ([Function: Error] { stackTraceLimit: 0 })>]\n' + '}' );

Also, why not returning the whole stack? Indentation seems to work

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Nov 11, 2025

yvele mentioned this pull request Nov 11, 2025

util.inspect crashes with a getter whose error.message throws #60683

Open

yvele force-pushed the defensive-inspect-getter-error-message branch from cff9e6a to ab97c2a Compare November 11, 2025 19:31

util: safely inspect getter errors whose message throws

09d8f0b

yvele force-pushed the defensive-inspect-getter-error-message branch from ab97c2a to 09d8f0b Compare November 11, 2025 19:47

aduh95 reviewed Nov 12, 2025

View reviewed changes

fixup! util: add test cases with getters throwing uncommon values

796a225

aduh95 reviewed Nov 12, 2025

View reviewed changes

test/parallel/test-util-inspect.js Outdated Show resolved Hide resolved

yvele added 4 commits November 12, 2025 11:07

fixup! util: handle uncommon thrown errors

d1b18c0

fixup! util: ESLint exception line styling

bcb98f7

fixup! util: add test case with getter throwing undefined

f85647b

fixup! util: implement consistent inspect(err)

94a702e

yvele commented Nov 12, 2025

View reviewed changes

fixup! util: improve test readability

9b05e4e

yvele force-pushed the defensive-inspect-getter-error-message branch from 89f0e53 to 9b05e4e Compare November 12, 2025 13:53

-        let messageSuffix;
-        try {
-          // Error message itself may be a getter
-          messageSuffix = ` (${err.message})`;
-        } catch {
-          messageSuffix = '';
-        }
-        const message = `<Inspection threw${messageSuffix}>`;
+       let message = '<Inspection threw>';
+       if (isErrorStackTraceLimitWritable()) {
+         const stackTraceLimit = Error.stackTraceLimit;
+         Error.stackTraceLimit = 0;
+         try { message = `<Inspection threw (${inspect(err)})>`; }
+         catch {}
+         finally { Error.stackTraceLimit = stackTraceLimit; }
+       }

Uh oh!

util: safely inspect getter errors whose message throws #60684

Are you sure you want to change the base?

util: safely inspect getter errors whose message throws #60684

Conversation

yvele commented Nov 11, 2025

Uh oh!

yvele commented Nov 11, 2025

Uh oh!

aduh95 Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

aduh95 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

yvele Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yvele Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

yvele Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yvele Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

aduh95 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

yvele Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduh95 Nov 12, 2025 •

edited

Loading

yvele Nov 12, 2025 •

edited

Loading

aduh95 Nov 12, 2025 •

edited

Loading

yvele Nov 12, 2025 •

edited

Loading

yvele Nov 12, 2025 •

edited

Loading

yvele Nov 12, 2025 •

edited

Loading