repl: fix crash with large buffer tab completion #13817

XadillaX · 2017-06-20T11:03:42Z

If the buffer or array is too large to completion, make a dummy smallest
substitute object for it and emit a warning.

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

repl

If the buffer or array is too large to completion, make a dummy smallest substitute object for it and emit a warning. Fixes: nodejs#3136

jasnell · 2017-06-20T14:43:11Z

lib/repl.js

+        warning.type,
+        undefined,
+        undefined,
+        true);


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

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.

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?

@jasnell, For aesthetics.

With last parameter false:

With immediately:

jasnell · 2017-06-20T14:44:48Z

lib/repl.js

+    warning.message =
+      'Instance is too large that the completion may missing ' +
+      'some customized properties.';
+    warning.type = 'REPLWarning';


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

jasnell · 2017-06-20T14:45:20Z

lib/repl.js

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

+  var self = this;


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

But I need use this.

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

XadillaX · 2017-06-20T15:21:45Z

@jasnell I've updated the code and added some test cases.

aqrln · 2017-06-20T16:11:44Z

lib/repl.js

@@ -689,8 +690,31 @@ function intFilter(item) {
  return /^[A-Za-z_$]/.test(item);
 }

+const DEFAULT_PROPERTIES = {


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

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

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.

aqrln · 2017-06-20T16:13:05Z

lib/repl.js

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

+  var self = this;


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

aqrln · 2017-06-20T16:17:11Z

test/parallel/test-repl-tab-complete.js

@@ -23,6 +23,7 @@

 const common = require('../common');
 const assert = require('assert');
+const Buffer = require('buffer').Buffer;


I'm pretty sure you don't need to import it in the test.

aqrln · 2017-06-20T16:25:56Z

lib/repl.js

+  if (mayBeLargeObject(obj) && obj.length > 1e6) {
+    this._writeToOutput('\r\n');
+    process.emitWarning(
+      'Instance is too large that the completion may missing ' +


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

aqrln · 2017-06-20T16:37:32Z

test/parallel/test-repl-tab-complete.js

+
+  common.hijackStderr(common.mustCall((err) => {
+    process.nextTick(function() {
+      assert.ok(/REPLWarning: Instance is too large that the/.test(err));


Can you please test the full warning message here?

aqrln · 2017-06-20T16:41:50Z

test/parallel/test-repl-tab-complete.js

+  }
+
+  common.hijackStderr(common.mustCall((err) => {
+    process.nextTick(function() {


Small and not essential nit: could you please use an arrow function here to be consistent with the surrounding code?

aqrln · 2017-06-20T16:59:54Z

lib/repl.js

+    this._writeToOutput('\r\n');
+    process.emitWarning(
+      'Instance is too large that the completion may missing ' +
+      'some customized properties.',


And "custom" might be better than "customized" here. (As for me, "customized" implies properties that have been changed, not necessarily added.)

aqrln · 2017-06-21T03:21:03Z

test/parallel/test-repl-tab-complete.js

@@ -305,6 +305,36 @@ testMe.complete('.b', common.mustCall((error, data) => {
  assert.deepStrictEqual(data, [['break'], 'b']);
 }));

+// tab completion for large buffer
+const warningRegEx =
+  /\(node:\d+\) REPLWarning: Instance is too large so the completion may missing some custom properties\./;


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

Shall I split it into two lines?

I'd say yes.

aqrln · 2017-06-21T03:22:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8766/

TimothyGu

We should report this issue to V8. Specifically, Object.keys throws an error instead of crashing, which seems what Object.getOwnPropertyNames should do as well

Object.keys(Buffer.alloc(2147483647))
       ^

RangeError: Invalid array length
    at Function.keys (<anonymous>)
    at [eval]:1:8

TimothyGu · 2017-06-21T03:01:05Z

lib/repl.js

@@ -689,8 +690,31 @@ function intFilter(item) {
  return /^[A-Za-z_$]/.test(item);
 }

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


Why not Buffer.alloc(0)?

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

TimothyGu · 2017-06-21T03:01:21Z

lib/repl.js

+};
+
+function mayBeLargeObject(obj) {
+  return (Array.isArray(obj) || Buffer.isBuffer(obj));


This doesn't address other TypedArray types.

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)

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

How about obj instanceof Buffer?

TimothyGu · 2017-06-21T03:01:57Z

lib/repl.js

 function filteredOwnPropertyNames(obj) {
  if (!obj) return [];
+  if (mayBeLargeObject(obj) && obj.length > 1e6) {


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

TimothyGu · 2017-06-21T03:08:03Z

lib/repl.js

+      'REPLWarning',
+      undefined,
+      undefined,
+      true);


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

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.

@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.

@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.

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.

XadillaX · 2017-06-21T04:36:35Z

We should report this issue to V8. Specifically, Object.keys throws an error instead of crashing, which seems what Object.getOwnPropertyNames should do as well

I think we may ask V8 to provide an API to get non-enumarable properties.

TimothyGu · 2017-06-21T08:54:04Z

lib/repl.js

+  // `Buffer.prototype` passes the `Buffer.isBuffer` and
+  // `instanceof Uint8Array`.
+  //
+  // Refs: https://github.com/nodejs/node/pull/11961


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.)

TimothyGu · 2017-06-21T08:55:15Z

lib/repl.js

+  [ Float32Array,
+    Object.getOwnPropertyNames(new Float32Array()).filter(intFilter) ],
+  [ Float64Array,
+    Object.getOwnPropertyNames(new Float64Array()).filter(intFilter) ]


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

TimothyGu · 2017-06-21T09:01:12Z

lib/repl.js

+      'REPLWarning',
+      undefined,
+      undefined,
+      true);


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.

XadillaX · 2017-06-22T11:25:43Z

I think this PR needs CI again.

aqrln · 2017-06-22T11:33:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8797/

XadillaX · 2017-06-26T11:04:57Z

could this be landed now?

aqrln · 2017-06-26T13:52:09Z

lib/repl.js

+  if (Array.isArray(obj)) {
+    return obj.length > ARRAY_LENGTH_THRESHOLD ? ['length'] : null;
+  } else if (utilBinding.isTypedArray(obj)) {
+    return obj.length > ARRAY_LENGTH_THRESHOLD ? [] : null;


But typed arrays have length too, don't they?

TypedArrays' length is implemented as a getter on the prototype instead of an own property, as is the case with Arrays.

Timothy is right.

aqrln

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/8913/

XadillaX · 2017-07-03T10:00:27Z

So I think this PR could be landed now. @jasnell

XadillaX · 2017-07-07T02:59:41Z

For landing, whom to /ping?

TimothyGu · 2017-07-07T05:09:23Z

Replaced this._writeToOutput (a readline private API) with this.outputStream.write, and landed in 7d7ccf0.

If the buffer or array is too large to completion, make a dummy smallest substitute object for it and emit a warning. PR-URL: #13817 Fixes: #3136 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

aqrln · 2017-07-07T08:00:10Z

@XadillaX IRC is a good place for such pings too. And sorry that it took a while, as a person who constantly pinged people for CI and landing some months ago, I know how frustrating it feels sometimes. FWIW, I thought about landing it, but then I was on a little vacation and I did no GitHub, and today I've opened this tab just to see that Timothy beat me to it. I hope that soon you'll be able to land your patches yourself :)

If the buffer or array is too large to completion, make a dummy smallest substitute object for it and emit a warning. PR-URL: #13817 Fixes: #3136 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

MylesBorins · 2017-08-15T01:24:59Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

The test will need to be rewritten as common.hijackStderr doesn't exist on v6.x

repl: fix crash with large buffer tab completion

a59ca52

If the buffer or array is too large to completion, make a dummy smallest substitute object for it and emit a warning. Fixes: nodejs#3136

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 20, 2017

jasnell reviewed Jun 20, 2017

View reviewed changes

XadillaX added 2 commits June 20, 2017 22:56

update after reviewing

c08093e

add some test cases

402070d

use this._writeToOutput to instead of process.stdout.write

59649f3

aqrln reviewed Jun 20, 2017

View reviewed changes

update after reviewing again

876ab6e

XadillaX force-pushed the fix/repl-large-buffer-completion branch from 221000d to 876ab6e Compare June 21, 2017 02:52

aqrln reviewed Jun 21, 2017

View reviewed changes

TimothyGu reviewed Jun 21, 2017

View reviewed changes

update after @TimothyGu's reviewing

11db662

TimothyGu reviewed Jun 21, 2017

View reviewed changes

update after reviewing again

573e252

XadillaX force-pushed the fix/repl-large-buffer-completion branch from 1093500 to 573e252 Compare June 21, 2017 11:40

TimothyGu approved these changes Jun 22, 2017

View reviewed changes

jasnell approved these changes Jun 22, 2017

View reviewed changes

aqrln reviewed Jun 26, 2017

View reviewed changes

aqrln approved these changes Jul 1, 2017

View reviewed changes

TimothyGu closed this Jul 7, 2017

XadillaX mentioned this pull request Jul 9, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the backport-requested-v6.x label Aug 15, 2017

repl: fix crash with large buffer tab completion #13817

repl: fix crash with large buffer tab completion #13817

Conversation

XadillaX commented Jun 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

XadillaX Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Jun 21, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Jun 21, 2017

Choose a reason for hiding this comment

TimothyGu Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Jun 22, 2017 • edited Loading

aqrln commented Jun 22, 2017

XadillaX commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

XadillaX commented Jul 3, 2017

XadillaX commented Jul 7, 2017

TimothyGu commented Jul 7, 2017 • edited Loading

aqrln commented Jul 7, 2017

MylesBorins commented Aug 15, 2017

XadillaX commented Jun 20, 2017 •

edited

Loading

XadillaX Jun 20, 2017 •

edited

Loading

aqrln Jun 20, 2017 •

edited

Loading

XadillaX Jun 21, 2017 •

edited

Loading

TimothyGu Jun 21, 2017 •

edited

Loading

XadillaX commented Jun 22, 2017 •

edited

Loading

TimothyGu commented Jul 7, 2017 •

edited

Loading