Skip to content

repl: fix crash with large buffer tab completion #13817

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

Closed
XadillaX wants to merge 7 commits into from
Closed

repl: fix crash with large buffer tab completion #13817

Show file tree
Hide file tree
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
Prev Previous commit
Next Next commit
update after @TimothyGu's reviewing
  • Loading branch information
@XadillaX
XadillaX committed Jun 21, 2017
commit 11db6628323f885ed356cdae5d7b7a7ff4eba6b6
58 changes: 49 additions & 9 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,18 +690,60 @@ function intFilter(item) {
return /^[A-Za-z_$]/.test(item);
}

const defaultProperties = {
ARRAY: Object.getOwnPropertyNames([]).filter(intFilter),
BUFFER: Object.getOwnPropertyNames(Buffer.alloc(1)).filter(intFilter)
};
const defaultProperties = [
[ Array, Object.getOwnPropertyNames([]).filter(intFilter) ],
[ Buffer, Object.getOwnPropertyNames(Buffer.alloc(0)).filter(intFilter) ],

[ Uint8Array,
Object.getOwnPropertyNames(new Uint8Array()).filter(intFilter) ],
[ Uint16Array,
Object.getOwnPropertyNames(new Uint16Array()).filter(intFilter) ],
[ Uint32Array,
Object.getOwnPropertyNames(new Uint32Array()).filter(intFilter) ],
[
Uint8ClampedArray,
Object.getOwnPropertyNames(new Uint8ClampedArray()).filter(intFilter) ],
[ Int8Array,
Object.getOwnPropertyNames(new Int8Array()).filter(intFilter) ],
[ Int16Array,
Object.getOwnPropertyNames(new Int16Array()).filter(intFilter) ],
[ Int32Array,
Object.getOwnPropertyNames(new Int32Array()).filter(intFilter) ],
[ Float32Array,
Object.getOwnPropertyNames(new Float32Array()).filter(intFilter) ],
[ Float64Array,
Object.getOwnPropertyNames(new Float64Array()).filter(intFilter) ]
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Of all the types enumerated, only plain arrays have own properties by default ('length'). No TypedArray objects have own properties by default.

];
const ARRAY_LENGTH_THRESHOLD = 1e6;

function mayBeLargeObject(obj) {
return (Array.isArray(obj) || Buffer.isBuffer(obj));
// `Buffer.prototype` passes the `Buffer.isBuffer` and
// `instanceof Uint8Array`.
//
// Refs: https://github.com/nodejs/node/pull/11961
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Jun 21, 2017

Choose a reason for hiding this comment

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

This will still throw an error on e.g. Object.create(Uint8Array.prototype).

Instead of all the checks below, it could be implemented simply and reliably as:

const { isTypedArray } = process.binding('util');

function maybeLargeObject(obj) {
  return Array.isArray(obj) || isTypedArray(obj);
}

(Buffers pass the isTypedArray test.)

if (obj === Buffer.prototype) return null;

for (const type of defaultProperties) {
var typeMatch;
if (type[0] === Array) {
typeMatch = Array.isArray(obj);
} else if (type[0] === Buffer) {
typeMatch = Buffer.isBuffer(obj);
} else {
typeMatch = obj instanceof type[0];
}

if (typeMatch) {
return obj.length > ARRAY_LENGTH_THRESHOLD ? type[1] : null;
}
}
return null;
}

function filteredOwnPropertyNames(obj) {
if (!obj) return [];
if (mayBeLargeObject(obj) && obj.length > 1e6) {
const fakeProperties = mayBeLargeObject(obj);
if (fakeProperties !== null) {
this._writeToOutput('\r\n');
process.emitWarning(
'Instance is too large so the completion may missing some custom ' +
Expand All @@ -711,9 +753,7 @@ function filteredOwnPropertyNames(obj) {
undefined,
true);
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Jun 21, 2017

Choose a reason for hiding this comment

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

Personally I'm okay with not showing the warning. Autocomplete is not a feature that necessarily has to work 100% of the time.

Copy link
Copy Markdown
Contributor Author

@XadillaX XadillaX Jun 21, 2017

Choose a reason for hiding this comment

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

But if there's no warning, I think after this PR landed, developers use Node.js may open several issue about this bug "why autocompletion is wrong", though it's not exactly a bug.

Copy link
Copy Markdown
Contributor

@aqrln aqrln Jun 21, 2017

Choose a reason for hiding this comment

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

@XadillaX fwiw, the autocompletion in REPL is far from perfect and I, as a user, would not and do not expect it to always show all the completions (though it would be an awesome thing). Even IDEs don't do that reliably for JavaScript.

Copy link
Copy Markdown
Contributor Author

@XadillaX XadillaX Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@aqrln The point is:

> var ele = Buffer.alloc(1);
> ele.biu = 1;
> ...
> ele.<tab>
ele.__defineGetter__      ele.__defineSetter__      ele.__lookupGetter__      ele.__lookupSetter__
ele.__proto__             ele.constructor           ele.hasOwnProperty        ele.isPrototypeOf
ele.propertyIsEnumerable  ele.toLocaleString        ele.toString              ele.valueOf

...

ele.biu

> var ele = Buffer.alloc(1e6 + 1); ele.biu = 1;
> ele.<tab>
(node:3635) REPLWarning: Instance is too large so the completion may missing some custom properties.

ele.__defineGetter__      ele.__defineSetter__      ele.__lookupGetter__      ele.__lookupSetter__
ele.__proto__             ele.constructor           ele.hasOwnProperty        ele.isPrototypeOf
ele.propertyIsEnumerable  ele.toLocaleString        ele.toString              ele.valueOf

...

Without warning, the developers may be confused that why there's no biu, that completion is what truly they want to find.

Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Jun 21, 2017

Choose a reason for hiding this comment

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

Okay. In that case, the message could be clearer:

The current object has too many own properties. Completion output may be truncated.

or

The current array, Buffer, or TypedArray has too many entries. Certain properties may be missing from completion output.


return Array.isArray(obj) ?
defaultProperties.ARRAY :
defaultProperties.BUFFER;
return fakeProperties;
}
return Object.getOwnPropertyNames(obj).filter(intFilter);
}
Expand Down
40 changes: 36 additions & 4 deletions test/parallel/test-repl-tab-complete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,33 @@ testMe.complete('.b', common.mustCall((error, data) => {
// tab completion for large buffer
const warningRegEx =
/\(node:\d+\) REPLWarning: Instance is too large so the completion may missing some custom properties\./;
Copy link
Copy Markdown
Contributor

@aqrln aqrln Jun 21, 2017

Choose a reason for hiding this comment

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

This line is longer than 80 characters, but the linter passes.

Copy link
Copy Markdown
Contributor Author

@XadillaX XadillaX Jun 21, 2017

Choose a reason for hiding this comment

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

Shall I split it into two lines?

Copy link
Copy Markdown
Contributor

@aqrln aqrln Jun 21, 2017

Choose a reason for hiding this comment

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

I'd say yes.

[ Array, Buffer ].forEach((type) => {
[
Array,
Buffer,

Uint8Array,
Uint16Array,
Uint32Array,

Uint8ClampedArray,
Int8Array,
Int16Array,
Int32Array,
Float32Array,
Float64Array,
].forEach((type) => {
putIn.run(['.clear']);

if (type === Array) {
putIn.run(['var ele = []; for (let i = 0; i < 1e7; i++) ele.push(i);']);
putIn.run([
'var ele = [];',
'for (let i = 0; i < 1e6 + 1; i++) ele[i] = 0;',
'ele.biu = 1;'
]);
} else if (type === Buffer) {
putIn.run(['var ele = Buffer.alloc(1e6 + 1); ele.biu = 1;']);
} else {
putIn.run(['var ele = Buffer.alloc(1e8)']);
putIn.run([`var ele = new ${type.name}(1e6 + 1); ele.biu = 1;`]);
}

common.hijackStderr(common.mustCall((err) => {
Expand All @@ -326,15 +346,27 @@ const warningRegEx =
common.restoreStderr();
assert.ifError(err);

const ele = (type === Array) ? [] : Buffer.alloc(1);
const ele = (type === Array) ?
[] :
(type === Buffer ?
Buffer.alloc(0) :
new type(0));

data[0].forEach((key) => {
if (!key) return;
assert.notStrictEqual(ele[key.substr(4)], undefined);
});

// no `biu`
assert.strictEqual(data.indexOf('ele.biu'), -1);
}));
});

// check Buffer.prototype.length not crashing.
// Refs: Refs: https://github.com/nodejs/node/pull/11961
putIn.run['.clear'];
testMe.complete('Buffer.prototype.', common.mustCall());

const testNonGlobal = repl.start({
input: putIn,
output: putIn,
Expand Down