util: compact inspect() for sparse arrays

[dcposch]

This prevents console.log(arr) from crashing node when given a small sparse array with large length

Fixes#4905

[Trott]

Thanks for the fix!

• Would you be able to add a test for this?
• Also could you revise the commit message to conform with the CONTRIBUTING.md guidelines? (First line should be 50 chars or less and be of the format util: fix console output for sparse array.)

[thefourtheye]

This has to be explained in the comments. How this calculation determines if the array is sparse or not?

[dcposch]

@thefourtheye added a comment. If less than 1/10th of the slots in an array are populated, util.inspect / console.log now renders it compactly

var arr = []
arr[100] = 1
console.log(arr)

Before:
[ , , , , , (lots of commas) , 1 ]

After:
[ 100:1 ]

[dcposch]

@Trott@thefourtheye sounds good, fixed

[dcposch]

@Trott added test cases, LMK if these look dece

vararr=[];arr[2]=1;// expect util.inspect() to return [ , , 1 ]arr[1000]=1;// expect [ 2: 1, 1000: 1, <sparse array, length 1001> ]')arr['foo']='bar';// expect [ 2: 1, 1000: 1, foo: 'bar', <sparse array, length 1001> ]arr=newArray(1000000);// expect [ <empty array, length 1000000> ]

[thefourtheye]

I think the sparse arrays are used only to prove the point. What happens when you use a dense array?

[dcposch]

@thefourtheye no, I think it's specifically about sparse arrays, where a small object can potentially lead to a huge console.log output. From #4905

this can lead to a DoS attack: for example, when an user-specified JSON {"1000000000":"a"} is merged with some pre-existing array and then printed on console

...by contrast there are lots of ways to make a large JSON produce a large console.log output, even without arrays. For example, an object with millions of keys. To avoid that, we'd need to make util.inspect respect a maximum length, which is a bigger change.

[dcposch]

Say you can crash a server by sending a 10GB POST, I think the fix is just to set request length limits. If you can crash a server by sending a 1KB POST which turns into a 10GB string internally, that's a bigger problem.

[thefourtheye]

Fair enough. But I am not sure if it is acceptable to special case it in the order of 10 like this. Lets hear from @nodejs/collaborators.

[mscdex]

Perhaps have something configurable like with require('buffer').INSPECT_MAX_BYTES.

[thefourtheye]

+1 to @mscdex 's idea. If users want to see more data, they can explicitly adjust the value and invoke inspect

[mcollina]

👍 for @mscdex idea, but with a default, something like 16 * 1024? Anyway that output is already big, and hard to be read by a human.

[vkurchatkin]

-1. We shouldn't treat sparse arrays differently

[mik01aj]

@vkurchatkin how would you fix the crash issue then?

[rvagg]

Not sure how I feel about this, surprising behaviour if you accidentally ran into it and I'm not sure it's intuitive although I guess the message helps.

@vkurchatkin given that sparse arrays can kill Node, and this is an even more surprising and destructive behaviour, I'm not sure there's anything else we can do than treat large arrays differently.

In terms of this actually fixing said issue, it only deals with sparse arrays, not large arrays. Consider:

> a=[]; for (i=0;i<100000000;i++)a[i]=1;
1
> a
... stacktrace spew ...
FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory
Abort trap: 6

That won't be solved here

[mik01aj]

Maybe we should treat large (and not sparse) arrays differently. E.g.: if the array has more than 1000 elements, print a ... at the end and finish.

[vkurchatkin]

Maybe we should treat large (and not sparse) arrays differently

exactly. printing very large array to console is not useful anyway

[dcposch]

I also think adding an INSPECT_MAX_BYTES with a default of few kilobytes is a good idea.

What I was saying above is, that will not just apply to arrays -- it'll have to limit output of strings and objects as well. Doable but a bit involved, esp since lib/util.js is kind of hairy and has few comments.

To give an example of what that would entail, I think we'd have to add a maxBytes argument to a bunch of functions including this one

functionformatProperty(ctx,value,recurseTimes,visibleKeys,key,array){varname,str,desc;desc=Object.getOwnPropertyDescriptor(value,key)||{value: value[key]};if(desc.get){if(desc.set){str=ctx.stylize('[Getter/Setter]','special');}else{str=ctx.stylize('[Getter]','special');}}else{if(desc.set){str=ctx.stylize('[Setter]','special');}}if(!hasOwnProperty(visibleKeys,key)){if(typeofkey==='symbol'){name='['+ctx.stylize(key.toString(),'symbol')+']';}else{name='['+key+']';}}if(!str){if(ctx.seen.indexOf(desc.value)<0){if(recurseTimes===null){str=formatValue(ctx,desc.value,null);}else{str=formatValue(ctx,desc.value,recurseTimes-1);}if(str.indexOf('\n')>-1){if(array){str=str.replace(/\n/g,'\n ');}else{str=str.replace(/(^|\n)/g,'\n ');}}}else{str=ctx.stylize('[Circular]','special');}}if(name===undefined){if(array&&key.match(/^\d+$/)){returnstr;}name=JSON.stringify(''+key);if(name.match(/^"([a-zA-Z_][a-zA-Z_0-9]*)"$/)){name=name.substr(1,name.length-2);name=ctx.stylize(name,'name');}else{name=name.replace(/'/g,"\\'").replace(/\\"/g,'"').replace(/(^"|"$)/g,"'").replace(/\\\\/g,'\\');name=ctx.stylize(name,'string');}}returnname+': '+str;}

[dcposch]

This change is smaller and at least covers the small-HTTP-POST-crashing-node case.

[cjihrig]

To give an example of what that would entail, I think we'd have to add a maxBytes argument to a bunch of functions including this one

That might not be a bad thing. The current algorithm for identifying sparse arrays seems pretty arbitrary. And, as pointed out, it doesn't do anything about large arrays.

[Trott]

This change is similar to the behavior of Chrome's console with sparse arrays. It may be useful to look and see what Blink/Chrome does to decide whether to do this special rendering or the usual expected rendering.

EDIT: Specifically, how does Chrome/Blink decide that an array requires special rendering?

[jasnell]

curious if this would be a feature change or a fix? minor or patch? lts or no?

[cjihrig]

IMO it's a breaking change.

[dcposch]

@Trott just did some experimenting. In Chrome:

var a = []
console.log(a) // prints []
a[4] = 'a'
console.log(a) // prints [4: "a"]
a[1] = 'b'
console.log(a) // prints [1: "b", 4: "a"]
a[0] = 'c'
console.log(a) // prints ["c", "b", 4: "a"]

So it looks like it prints a normal dense array for all indices starting from 0, then as soon as it gets to an index that is not set (which is not the same as an index whose corresponding value is undefined), then it switches to the sparse representation.

If you want I can change this PR slightly to match that behavior.

[Trott]

@dcposch I don't have an opinion on that one way or the other, but someone else might.

[domenic]

Personally I can't understand how this is minor---what new feature does it introduce? It's either major (breaking change to existing feature) or patch (bugfix to existing feature).

[dcposch]

@Trott@rvagg is there anything you'd like me to change?

[rvagg]

I'm finding it hard to conjure up much of an opinion on this one. @nodejs/collaborators anyone else want to champion this one through? I'd like to fix the crashes but am also concerned about how people might already be relying on util.inspect() already (perhaps mistakenly but that's how it goes).

[mscdex]

As I previously mentioned, I wouldn't mind having a INSPECT_MAX_BYTES-type of thing so that arrays(/array-like objects) only show the first n elements (for consistency with Buffer inspection). Maybe have like util.INSPECT_MAX_ELEMENTS or something. I don't really care so much about the actual name, as long as the similar functionality is there.

[Trott]

@dcposch wrote:

@Trott@rvagg is there anything you'd like me to change?

Similar to @rvagg, I'll opt to tread carefully with this and am happy to defer to other collaborators with stronger and more-informed opinions.

[tunniclm]

EDIT: Oops, posted the comment to the wrong place. Moved to: #4905 (comment)

[rvagg]

Building on @tunniclm's suggestion that he's since moved, perhaps the easiest path forward on this one is to add an option to util.inspect() that switches on this behaviour, or multiple options as per @tunniclm's proposal that controls it but have the default behaviour be as it is now. Then we can ship this as a semver-minor and consider the question of making it the default behaviour as separate matter. The fact that this changes output and becomes a semver-major makes it difficult to fully embrace it, but if it's just a feature addition without changing default then we can consider the change without being forced to think about ecosystem impact.

[jasnell]

Looking at this again, I would find it difficult to justify leaving the current crash behavior as the default. inspect()already has an options object, let's:

• Make the default such that large arrays over some default length get the ... treatment just like we currently do with Buffer output
• Add an option like maxArrayLength to inspect() that can override the default if necessary.
• Mark it semver-major and get it into v6

[jasnell]

I've opened an alternative PR that takes a slightly different approach to this. See #6334

[benjamingr]

I think consensus is going towards #6334 - is this still being pursued?

[jasnell]

I'd recommend closing this in favor of #6334 (but I'm a bit biased lol)

[benjamingr]

I think we can close this now and it can always be reopened. It seems stalled and there seems to be consensus is with the other proposal anyway. Feel free to reopen.