Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: fix crash with large buffer tab completion #13817

Closed
wants to merge 7 commits into from
Closed

repl: fix crash with large buffer tab completion #13817

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a59ca52
repl: fix crash with large buffer tab completion
XadillaX Jun 20, 2017
c08093e
update after reviewing
XadillaX Jun 20, 2017
402070d
add some test cases
XadillaX Jun 20, 2017
59649f3
use this._writeToOutput to instead of process.stdout.write
XadillaX Jun 20, 2017
876ab6e
update after reviewing again
XadillaX Jun 21, 2017
11db662
update after @TimothyGu's reviewing
XadillaX Jun 21, 2017
573e252
update after reviewing again
XadillaX Jun 21, 2017
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
43 changes: 38 additions & 5 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const Module = require('module');
const domain = require('domain');
const debug = util.debuglog('repl');
const errors = require('internal/errors');
const Buffer = require('buffer').Buffer;

const parentModule = module;
const replMap = new WeakMap();
Expand Down Expand Up @@ -689,8 +690,26 @@ function intFilter(item) {
return /^[A-Za-z_$]/.test(item);
}

function filteredOwnPropertyNames(obj) {
const DEFAULT_PROPERTIES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if these should be uppercased. (I'd prefer them not to be, fwiw.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be better by using uppercase on such a constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear: I'm fine with it either way, it just looks a bit weird to me in this specific case, especially the uppercased properties, not the object name itself.

ARRAY: Object.getOwnPropertyNames([]).filter(intFilter),
BUFFER: Object.getOwnPropertyNames(Buffer.alloc(1)).filter(intFilter)
Copy link
Member

Choose a reason for hiding this comment

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

Why not Buffer.alloc(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't know that we can alloc a zero-size buffer.

};

function mayBeLargeObject(obj) {
return (Array.isArray(obj) || Buffer.isBuffer(obj));
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't address other TypedArray types.

Copy link
Member

Choose a reason for hiding this comment

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

Also, currently, Buffer.prototype passes the Buffer.isBuffer test. On the other hand, getting Buffer.prototype.length will throw the following error:

> Buffer.prototype.length
TypeError: Method get TypedArray.prototype.length called on incompatible receiver [object Object]
    at Uint8Array.get length [as length] (<anonymous>)
    at repl:1:17
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:433:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:278:10)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, currently, Buffer.prototype passes the Buffer.isBuffer test.

How about obj instanceof Buffer?

}

function filteredOwnPropertyNames(obj, warning) {
if (!obj) return [];
if (mayBeLargeObject(obj) && obj.length > 1e6) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a constant for 1e6 like const ARRAY_LENGTH_THRESHOLD?

warning.message =
'Instance is too large that the completion may missing ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like "so" might be better than "that" here (but I'm not a native speaker).

'some customized properties.';
warning.type = 'REPLWarning';
Copy link
Member

Choose a reason for hiding this comment

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

Why not just emit the warning here instead of building this warning object?

return Array.isArray(obj) ?
DEFAULT_PROPERTIES.ARRAY :
DEFAULT_PROPERTIES.BUFFER;
}
return Object.getOwnPropertyNames(obj).filter(intFilter);
}

Expand Down Expand Up @@ -732,10 +751,12 @@ function complete(line, callback) {
}
}

var self = this;
Copy link
Member

Choose a reason for hiding this comment

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

would prefer not to introduce a new var self = this into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I need use this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use an arrow function so that you don't need the var self = this trick?

var completions;

// list of completion lists, one for each inheritance "level"
var completionGroups = [];
var warning = {};

var completeOn, i, group, c;

Expand Down Expand Up @@ -843,9 +864,11 @@ function complete(line, callback) {
if (this.useGlobal || vm.isContext(this.context)) {
var contextProto = this.context;
while (contextProto = Object.getPrototypeOf(contextProto)) {
completionGroups.push(filteredOwnPropertyNames(contextProto));
completionGroups.push(
filteredOwnPropertyNames(contextProto, warning));
}
completionGroups.push(filteredOwnPropertyNames(this.context));
completionGroups.push(
filteredOwnPropertyNames(this.context, warning));
addStandardGlobals(completionGroups, filter);
completionGroupsLoaded();
} else {
Expand All @@ -871,7 +894,7 @@ function complete(line, callback) {
if (obj != null) {
if (typeof obj === 'object' || typeof obj === 'function') {
try {
memberGroups.push(filteredOwnPropertyNames(obj));
memberGroups.push(filteredOwnPropertyNames(obj, warning));
} catch (ex) {
// Probably a Proxy object without `getOwnPropertyNames` trap.
// We simply ignore it here, as we don't want to break the
Expand All @@ -889,7 +912,7 @@ function complete(line, callback) {
p = obj.constructor ? obj.constructor.prototype : null;
}
while (p !== null) {
memberGroups.push(filteredOwnPropertyNames(p));
memberGroups.push(filteredOwnPropertyNames(p, warning));
p = Object.getPrototypeOf(p);
// Circular refs possible? Let's guard against that.
sentinel--;
Expand Down Expand Up @@ -928,6 +951,16 @@ function complete(line, callback) {
function completionGroupsLoaded(err) {
if (err) throw err;

if (warning.type) {
self._writeToOutput('\r\n');
process.emitWarning(
warning.message,
warning.type,
undefined,
undefined,
true);
Copy link
Member

Choose a reason for hiding this comment

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

There is no boolean fifth argument to process.emitWarning() so I'm not sure what this is doing.

Copy link
Contributor Author

@XadillaX XadillaX Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

refer https://github.com/nodejs/node/blob/v8.1.2/lib/internal/process/warning.js#L153

if it's true, it won't be emitted on next tick.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh... forgot about that change. It wasn't added to the docs and wasn't very keen on it in the first place. In this case, why does the warning need to be emitted immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell, For aesthetics.

With last parameter false:

image

With immediately:

image

Copy link
Member

Choose a reason for hiding this comment

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

OK.

}

// Filter, sort (within each group), uniq and merge the completion groups.
if (completionGroups.length && filter) {
var newCompletionGroups = [];
Expand Down