util: limit util inspect depth to 10 by BridgeAR · Pull Request #20627 · nodejs/node · GitHub
Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions doc/api/util.md
29 changes: 21 additions & 8 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,9 @@ function dnsException(code, syscall, hostname) {
return ex;
}

let maxStack_ErrorName;
let maxStack_ErrorMessage;
let maxStackErrorName;
let maxStackErrorMessage;
let maxStackLimit = 0;
/**
* Returns true if `err.name` and `err.message` are equal to engine-specific
* values indicating max call stack size has been exceeded.
Expand All @@ -669,18 +670,29 @@ let maxStack_ErrorMessage;
* @returns {boolean}
*/
function isStackOverflowError(err) {
if (maxStack_ErrorMessage === undefined) {
if (maxStackErrorMessage === undefined) {
try {
function overflowStack() { overflowStack(); }
function overflowStack() {
maxStackLimit++;
overflowStack();
}
overflowStack();
} catch (err) {
maxStack_ErrorMessage = err.message;
maxStack_ErrorName = err.name;
maxStackErrorMessage = err.message;
maxStackErrorName = err.name;
}
}

return err.name === maxStack_ErrorName &&
err.message === maxStack_ErrorMessage;
return err.name === maxStackErrorName &&
err.message === maxStackErrorMessage;
}

function getMaxStackLimit() {
if (maxStackLimit === 0) {
// Use a fake "error"
isStackOverflowError({ name: '', message: '' });
}
return maxStackLimit;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure, but I wouldn’t expect that stack overflows happen all at the same depths – some stack frames will necessarily take more space than others, so what this would give us is an upper bound rather than something that is necessarily usable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. That is also why I do not directly use it. In the end I use a absolute limit of 1000. That could be lowered if this function returns a low number. That is all I do with that. But I am perfectly fine with just always using 1000 as limit and to remove this again.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be fine with me, yea. (But then again depth: null kinda sounds like opting into accepting stack overflow errors to me 😄)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also pointed out above that I have a working patch that allows to inspect really deep nested objects without such errors. With that patch applied we could set a absolute depth limit of e.g. 10000 (anything above that is going to cause a lot of CPU time and the heap memory will not be enough at some point). What I do is to catch the error, bubble to the top and start over with the part that caused the issue and than assemble everything.

}

module.exports = {
Expand All @@ -689,6 +701,7 @@ module.exports = {
exceptionWithHostPort,
uvException,
isStackOverflowError,
getMaxStackLimit,
getMessage,
AssertionError,
SystemError,
Expand Down
44 changes: 30 additions & 14 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,16 @@

'use strict';

const errors = require('internal/errors');
const {
ERR_FALSY_VALUE_REJECTION,
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE
} = errors.codes;
getMaxStackLimit,
errnoException: _errnoException,
exceptionWithHostPort: _exceptionWithHostPort,
codes: {
ERR_FALSY_VALUE_REJECTION,
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE
}
} = require('internal/errors');
const { TextDecoder, TextEncoder } = require('internal/encoding');
const { isBuffer } = require('buffer').Buffer;

Expand Down Expand Up @@ -80,7 +84,7 @@ const {

const inspectDefaultOptions = Object.seal({
showHidden: false,
depth: null,
depth: 10,
colors: false,
customInspect: true,
showProxy: false,
Expand All @@ -105,6 +109,12 @@ const numberRegExp = /^(0|[1-9][0-9]*)$/;

const readableRegExps = {};

// The actual maximum stack limit has to be divided by the number of function
// calls that are maximal necessary for each depth level. That does not seem
// to be sufficient though, so let us just divide it by 10 and use 1000 as upper
// limit.
const MAX_DEPTH_LIMIT = Math.min(Math.floor(getMaxStackLimit() / 10), 1000);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still seems too magical to me - no less magical than the depth: 2 default at least, but that's simpler.


const MIN_LINE_LENGTH = 16;

// Escaped special characters. Use empty strings to fill up unused entries.
Expand Down Expand Up @@ -287,6 +297,14 @@ function debuglog(set) {
return debugs[set];
}

function getMaxDepth(depth) {
if (depth === null)
return MAX_DEPTH_LIMIT;
if (depth < MAX_DEPTH_LIMIT)
return depth;
return MAX_DEPTH_LIMIT;
}

/**
* Echos the value of any input. Tries to print the value out
* in the best way possible given the different types.
Expand Down Expand Up @@ -332,7 +350,7 @@ function inspect(value, opts) {
}
if (ctx.colors) ctx.stylize = stylizeWithColor;
if (ctx.maxArrayLength === null) ctx.maxArrayLength = Infinity;
return formatValue(ctx, value, ctx.depth);
return formatValue(ctx, value, getMaxDepth(ctx.depth));
}
inspect.custom = customInspectSymbol;

Expand Down Expand Up @@ -677,11 +695,9 @@ function formatValue(ctx, value, recurseTimes, ln) {
}
}

if (recurseTimes != null) {
if (recurseTimes < 0)
return ctx.stylize(`[${constructor || tag || 'Object'}]`, 'special');
recurseTimes -= 1;
}
if (recurseTimes < 0)
return ctx.stylize(`[${constructor || tag || 'Object'}]`, 'special');
recurseTimes -= 1;

ctx.seen.push(value);
const output = formatter(ctx, value, recurseTimes, keys);
Expand Down Expand Up @@ -1246,8 +1262,8 @@ function getSystemErrorName(err) {

// Keep the `exports =` so that various functions can still be monkeypatched
module.exports = exports = {
_errnoException: errors.errnoException,
_exceptionWithHostPort: errors.exceptionWithHostPort,
_errnoException,
_exceptionWithHostPort,
_extend,
callbackify,
debuglog,
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-util-inspect.js