src: make StreamBase prototype accessors more robust #16860

joyeecheung · 2017-11-07T11:53:04Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src

This problem showed up when I was stepping into Writable.prototype.write and hover over this from a console.log call, there was assertion like this:

see assertion

node[90480]: ../src/util-inl.h:243:TypeName *node::Unwrap(v8::Local<v8::Object>) [TypeName = node::LibuvStreamWrap]: Assertion `(object->InternalFieldCount()) > (0)' failed.
 1: node::Abort() [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 2: node::(anonymous namespace)::DomainEnter(node::Environment*, v8::Local<v8::Object>) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 3: node::LibuvStreamWrap::~LibuvStreamWrap() [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 4: void node::StreamBase::GetBytesRead<node::LibuvStreamWrap>(v8::Local<v8::String>, v8::PropertyCallbackInfo<v8::Value> const&) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 5: v8::internal::PropertyCallbackArguments::Call(void (*)(v8::Local<v8::Name>, v8::PropertyCallbackInfo<v8::Value> const&), v8::internal::Handle<v8::internal::Name>) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 6: v8::internal::Object::GetPropertyWithAccessor(v8::internal::LookupIterator*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 7: v8::internal::JSReceiver::GetOwnPropertyDescriptor(v8::internal::LookupIterator*, v8::internal::PropertyDescriptor*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 8: v8::internal::JSReceiver::GetOwnPropertyDescriptor(v8::internal::Isolate*, v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyDescriptor*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 9: v8::internal::Builtin_Impl_ObjectGetOwnPropertyDescriptor(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
10: 0x3f95

because the inspector would wrap the inspected project's prototype when generating previews. This is also reproducible when util.inspect a prototype setup by StreamBase::AddMethods. This PR makes those accessors nonenumerable and check the signatures in the accessors so they throw instead of raise assertions.

joyeecheung · 2017-11-07T12:04:56Z

test/parallel/test-stream-base-prototype-accessors.js

+
+// Should throw instead of raise assertions
+{
+  const msg = /TypeError: Method \w+ called on incompatible receiver/;


This is not engine-agnostic but I am not sure what ChakraCore throws for this..

bnoordhuis · 2017-11-07T14:52:08Z

src/stream_base-inl.h

@@ -31,27 +32,33 @@ void StreamBase::AddMethods(Environment* env,
  HandleScope scope(env->isolate());

  enum PropertyAttribute attributes =
-      static_cast<PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
+      static_cast<PropertyAttribute>(
+        v8::ReadOnly | v8::DontDelete | v8::DontEnum);


Four space indent here and two lines below.

joyeecheung · 2017-11-10T15:02:31Z

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

joyeecheung · 2017-11-11T04:53:48Z

Landed in 1ee3244, thanks!

This PR makes the prototype accessors added by StreamBase::AddMethods nonenumerable and checks the signatures in the accessors so they throw instead of raising assertions when called with incompatible receivers. They could be enumerated when inspecting the prototype with util.inspect or the inspector protocol. PR-URL: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-11-17T15:54:33Z

I've landed this in both v6.x and v8.x staging

please let me know if this should bake longer before releasing

This PR makes the prototype accessors added by StreamBase::AddMethods nonenumerable and checks the signatures in the accessors so they throw instead of raising assertions when called with incompatible receivers. They could be enumerated when inspecting the prototype with util.inspect or the inspector protocol. PR-URL: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jure · 2017-12-08T11:07:36Z

As mentioned in #16949 (comment), isn't there a way to have it not assert, as was the case in 8.8.1 (just return undefined)?

We've been seeing some new test failures with the error thrown here, and the cause is hard to discover (and even harder to fix).

Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860

PR-URL: #17665 Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs#17665 Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Backport-PR-URL: #20456 PR-URL: #17665 Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

* src: replace SetAccessor w/ SetAccessorProperty PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> * test: make test-tls-external-accessor agnostic Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. Backport-PR-URL: nodejs/node#20456 PR-URL: nodejs/node#16272 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 7, 2017

joyeecheung force-pushed the make-stream-robust branch from e8f3077 to d926aaa Compare November 7, 2017 11:59

joyeecheung added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 7, 2017

joyeecheung force-pushed the make-stream-robust branch from d926aaa to b7d63e8 Compare November 7, 2017 12:00

src: make StreamBase prototype accessors more robust

ddb12ac

joyeecheung force-pushed the make-stream-robust branch from b7d63e8 to ddb12ac Compare November 7, 2017 12:03

joyeecheung commented Nov 7, 2017

View reviewed changes

addaleax approved these changes Nov 7, 2017

View reviewed changes

bnoordhuis approved these changes Nov 7, 2017

View reviewed changes

addaleax added the lts-watch-v8.x label Nov 8, 2017

[fixup] 4 space indentation

faee4df

jasnell approved these changes Nov 8, 2017

View reviewed changes

cjihrig approved these changes Nov 8, 2017

View reviewed changes

joyeecheung closed this Nov 11, 2017

TimothyGu mentioned this pull request Nov 11, 2017

Getting ._handle through --inspect with chrome developer tools crashes Node.js #16949

Closed

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

MylesBorins added the lts-watch-v6.x label Nov 17, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Nov 17, 2017

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

erulabs mentioned this pull request Dec 12, 2017

Object.getOwnPropertyDescriptor throws TypeError on process _handle #17636

Closed

jure added a commit to jure/node that referenced this pull request Dec 13, 2017

Change to using SetAccessorProperty instead of SetAccessor in StreamBase

329e162

Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860

jure mentioned this pull request Dec 13, 2017

src,stream: use SetAccessorProperty instead of SetAccessor #17665

Closed

4 tasks

g-sam mentioned this pull request Apr 10, 2018

Bunyan can not be supported in Node versions > 8.9.1 (119) elifesciences/elife-xpub#83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make StreamBase prototype accessors more robust #16860

src: make StreamBase prototype accessors more robust #16860

joyeecheung commented Nov 7, 2017

joyeecheung Nov 7, 2017

bnoordhuis Nov 7, 2017

joyeecheung commented Nov 10, 2017

joyeecheung commented Nov 11, 2017

MylesBorins commented Nov 17, 2017

jure commented Dec 8, 2017 •

edited

src: make StreamBase prototype accessors more robust #16860

src: make StreamBase prototype accessors more robust #16860

Conversation

joyeecheung commented Nov 7, 2017

Checklist

Affected core subsystem(s)

joyeecheung Nov 7, 2017

Choose a reason for hiding this comment

bnoordhuis Nov 7, 2017

Choose a reason for hiding this comment

joyeecheung commented Nov 10, 2017

joyeecheung commented Nov 11, 2017

MylesBorins commented Nov 17, 2017

jure commented Dec 8, 2017 • edited

jure commented Dec 8, 2017 •

edited