src: no abort from GetFD if object isn't wrapped #6184

trevnorris · 2016-04-13T19:21:38Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

stream_base, base_object

Description of change

Previously the 0-index slot from
v8::Object::GetAlignedPointerFromInternalField() would return random
data if Wrap() hadn't been run on the object handle. Now immediately set
the slot to nullptr so we can know for sure whether it's been set or
not. Use this information to return -1 from GetFD() instead of aborting.

R=@bnoordhuis
R=@indutny

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

indutny · 2016-04-13T19:29:59Z

test/parallel/test-stream-base-no-abort.js

+  });
+});
+
+dgram.createSocket('udp4').close(checkTCP);


Just curious. What do you mean by "Fun"?

bnoordhuis · 2016-04-14T10:10:23Z

LGTM but Fedor has a point that there are probably more places where the value of wrap should be checked.

jasnell · 2016-04-15T06:53:31Z

LGTM. +1 on seeing if this can hit the other places too.

jasnell · 2016-04-28T22:18:47Z

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

trevnorris · 2016-05-04T19:45:33Z

@bnoordhuis @indutny I've gone through and made it so we always check the return value of Unwrap() before accessing it. Usually performing an early return. Where it looked necessary I've tried to set the appropriate default return value without ever throwing an error.

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

jasnell · 2016-05-04T19:49:18Z

src/js_stream.cc

-  JSStream* wrap = Unwrap<JSStream>(args.Holder());
-  WriteWrap* w = Unwrap<WriteWrap>(args[0].As<Object>());
+  JSStream* wrap;
+  // TODO(trevnorris): Verify that args[0] is actually an Object.


Why leave this as a todo?

I wanted feedback on whether it should throw or just return early.

I'd add a CHECK. If it's not an object, there's a logic error in node somewhere, right?

bnoordhuis · 2016-05-05T10:23:28Z

Left some comments. I do wonder if this approach isn't too blunt. I see a lot places where a nullptr wrap object is arguably an abort-worthy bug.

trevnorris · 2016-05-10T21:25:28Z

@bnoordhuis

Left some comments. I do wonder if this approach isn't too blunt. I see a lot places where a nullptr wrap object is arguably an abort-worthy bug.

I don't disagree here. Originally all I wanted to make sure of is that it would be impossible to abort the process if inspecting the handle at any point in it's lifetime. Similar to how I originally wanted to prevent abort from the C++ getters. And, lazily, it was easier to prevent abort from every unwrap instead of reason about each location. In the hopes that this would highlight every location where it could abort, and from there we could identify which places should definitely abort.

trevnorris · 2016-05-11T07:34:38Z

@bnoordhuis Everything's been fixed. This was a blunt approach to the problem, and I'm fine going back through and being more decisive about when we choose to abort.

trevnorris · 2016-05-16T20:09:42Z

ping @bnoordhuis

@addaleax mind giving this a once over as well?

addaleax · 2016-05-16T20:33:03Z

src/base-object-inl.h

@@ -14,6 +14,10 @@ inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
    : handle_(env->isolate(), handle),
      env_(env) {
  CHECK_EQ(false, handle.IsEmpty());
+  // The zero field holds a pointer to the handle. Immediately set it to
+  // nullptr in case it's accessed by the user before contruction is complete.


nit: construction

Thanks for catching this.

addaleax · 2016-05-16T21:55:22Z

I don’t know yet whether I have a strong opinion on aborting in some of these cases rather than returning silently, but I’ll sleep on that. And this LGTM either way… anything is better than segfaulting. ;)

trevnorris · 2016-05-17T19:14:13Z

@bnoordhuis I can make a best effort at judging which locations where nullptr should cause the application to abort. As an example, I'm thinking TLSWrap would be one, since the constructor is only ever called from C++.

bnoordhuis · 2016-05-20T11:12:07Z

src/node_contextify.cc

+    ContextifyScript* wrapped_script;
+    ASSIGN_OR_RETURN_UNWRAP(&wrapped_script,
+                            args.Holder(),
+                            false);


Fits on a single line.

MylesBorins · 2016-07-11T21:33:44Z

@trevnorris should this be included with the other async wrap changes?

trevnorris · 2016-07-11T22:09:56Z

@thealphanerd Yes.

MylesBorins · 2016-07-11T22:21:14Z

@trevnorris it looks like this will have to be manually backported, will you be able to help with that?

trevnorris · 2016-07-12T17:44:16Z

@thealphanerd sure. I can do that. When would you need it by?

MylesBorins · 2016-07-12T17:45:08Z

@trevnorris there is no rush. But if it is in sooner it can make the cut for v4.5.0

MylesBorins · 2016-10-24T16:37:56Z

ping @trevnorris would like to include this in v4.6.2

bnoordhuis · 2016-10-24T17:43:48Z

I'll take this.

To make sure casting a class of multiple inheritance from a void* to AsyncWrap succeeds make AsyncWrap the first inherited class. PR-URL: nodejs#6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

In case the handle is stored and accessed after the associated C++ class was destructed, set the internal pointer to nullptr so any getters/setters can return accordingly without aborting the application. PR-URL: nodejs#6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Though the TLSWrap constructor is only called via TLSWrap::Wrap() (i.e. tls_wrap.wrap()) internally, it is still exposed to JS. Don't allow the application to abort by inspecting the instance before it has been wrap'd by another handle. PR-URL: nodejs#6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

First cast the pointer to the child Base class before casting to the parent class to make sure it returns the correct pointer. PR-URL: nodejs#6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

v8::Object::GetAlignedPointerFromInternalField() returns a random value if Wrap() hasn't been run on the object handle. Causing v8 to abort if certain getters are accessed. It's possible to access these getters and functions during class construction through the AsyncWrap init() callback, and also possible in a subset of those scenarios while running the persistent handle visitor. Mitigate this issue by manually setting the internal aligned pointer field to nullptr in the BaseObject constructor and add necessary logic to return appropriate values when nullptr is encountered. PR-URL: nodejs#6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

bnoordhuis · 2016-10-24T18:28:57Z

#9260

To make sure casting a class of multiple inheritance from a void* to AsyncWrap succeeds make AsyncWrap the first inherited class. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

In case the handle is stored and accessed after the associated C++ class was destructed, set the internal pointer to nullptr so any getters/setters can return accordingly without aborting the application. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Though the TLSWrap constructor is only called via TLSWrap::Wrap() (i.e. tls_wrap.wrap()) internally, it is still exposed to JS. Don't allow the application to abort by inspecting the instance before it has been wrap'd by another handle. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

First cast the pointer to the child Base class before casting to the parent class to make sure it returns the correct pointer. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

v8::Object::GetAlignedPointerFromInternalField() returns a random value if Wrap() hasn't been run on the object handle. Causing v8 to abort if certain getters are accessed. It's possible to access these getters and functions during class construction through the AsyncWrap init() callback, and also possible in a subset of those scenarios while running the persistent handle visitor. Mitigate this issue by manually setting the internal aligned pointer field to nullptr in the BaseObject constructor and add necessary logic to return appropriate values when nullptr is encountered. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

To make sure casting a class of multiple inheritance from a void* to AsyncWrap succeeds make AsyncWrap the first inherited class. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

In case the handle is stored and accessed after the associated C++ class was destructed, set the internal pointer to nullptr so any getters/setters can return accordingly without aborting the application. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Though the TLSWrap constructor is only called via TLSWrap::Wrap() (i.e. tls_wrap.wrap()) internally, it is still exposed to JS. Don't allow the application to abort by inspecting the instance before it has been wrap'd by another handle. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

First cast the pointer to the child Base class before casting to the parent class to make sure it returns the correct pointer. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

v8::Object::GetAlignedPointerFromInternalField() returns a random value if Wrap() hasn't been run on the object handle. Causing v8 to abort if certain getters are accessed. It's possible to access these getters and functions during class construction through the AsyncWrap init() callback, and also possible in a subset of those scenarios while running the persistent handle visitor. Mitigate this issue by manually setting the internal aligned pointer field to nullptr in the BaseObject constructor and add necessary logic to return appropriate values when nullptr is encountered. PR-URL: #6184 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

trevnorris added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 13, 2016

indutny reviewed Apr 13, 2016
View reviewed changes

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

trevnorris force-pushed the no-abort-fd branch from f332669 to 0e28354 Compare May 4, 2016 19:40

jasnell reviewed May 4, 2016
View reviewed changes

trevnorris force-pushed the no-abort-fd branch from 0e28354 to 27cef10 Compare May 11, 2016 07:30

addaleax reviewed May 16, 2016
View reviewed changes

trevnorris force-pushed the no-abort-fd branch from 27cef10 to 3ce3fdc Compare May 17, 2016 20:43

bnoordhuis reviewed May 20, 2016
View reviewed changes

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label Jul 11, 2016

MylesBorins added this to the v4.6.2 milestone Oct 24, 2016

mscdex removed the lts-watch-v4.x label Oct 24, 2016

MylesBorins added land-on-v4.x labels Oct 25, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: no abort from GetFD if object isn't wrapped #6184

src: no abort from GetFD if object isn't wrapped #6184

trevnorris commented Apr 13, 2016

indutny Apr 13, 2016

trevnorris May 24, 2016

bnoordhuis commented Apr 14, 2016

jasnell commented Apr 15, 2016

jasnell commented Apr 28, 2016

trevnorris commented May 4, 2016

jasnell May 4, 2016

trevnorris May 5, 2016

bnoordhuis May 5, 2016

bnoordhuis commented May 5, 2016

trevnorris commented May 10, 2016

trevnorris commented May 11, 2016

trevnorris commented May 16, 2016

addaleax May 16, 2016

trevnorris May 24, 2016

addaleax commented May 16, 2016

trevnorris commented May 17, 2016

bnoordhuis May 20, 2016

MylesBorins commented Jul 11, 2016

trevnorris commented Jul 11, 2016

MylesBorins commented Jul 11, 2016

trevnorris commented Jul 12, 2016

MylesBorins commented Jul 12, 2016

MylesBorins commented Oct 24, 2016

bnoordhuis commented Oct 24, 2016

bnoordhuis commented Oct 24, 2016

src: no abort from GetFD if object isn't wrapped #6184

src: no abort from GetFD if object isn't wrapped #6184

Conversation

trevnorris commented Apr 13, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Apr 14, 2016

jasnell commented Apr 15, 2016

jasnell commented Apr 28, 2016

trevnorris commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 5, 2016

trevnorris commented May 10, 2016

trevnorris commented May 11, 2016

trevnorris commented May 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented May 16, 2016

trevnorris commented May 17, 2016

Choose a reason for hiding this comment

MylesBorins commented Jul 11, 2016

trevnorris commented Jul 11, 2016

MylesBorins commented Jul 11, 2016

trevnorris commented Jul 12, 2016

MylesBorins commented Jul 12, 2016

MylesBorins commented Oct 24, 2016

bnoordhuis commented Oct 24, 2016

bnoordhuis commented Oct 24, 2016