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.

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

stream_base,tls_wrap: notify on destruct #11947

Closed
wants to merge 2 commits into from

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Mar 20, 2017

Classes like TLSWrap are given a pointer to a StreamBase instance;
stored on the private member TLSWrap::stream_. Unfortunately the
lifetime of stream_ is not reported to the TLSWrap instance and if
free'd will leave an invalid pointer; which can still be accessed by the
TLSWrap instance. Causing the application to segfault if accessed.

Give StreamBase a destruct callback to notify when the class is being
cleaned up. Use this in TLSWrap to set stream_ = nullptr.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

stream_base, tls_wrap

CI: https://ci.nodejs.org/job/node-test-commit/8575/

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Mar 20, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Can you also undo the lib/ and src/ changes from #11776 here? They should just be unnecessary overhead now.

@trevnorris
Copy link
Contributor Author

@addaleax I reverted the lib/ and sr/ changes from that commit but it's crashing. I'll see if it can be easily fixed. /cc @jasnell

@addaleax
Copy link
Member

addaleax commented Mar 20, 2017

Curious, I tried it too but it worked fine (leaving out the one in IsAlive() – maybe that’s it?)

edit:

diff:
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index e1767c5e6723..4c9294fb4e69 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -399,12 +399,6 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
     res = null;
   });
 
-  if (wrap) {
-    wrap.on('close', function() {
-      res.onStreamClose();
-    });
-  }
-
   return res;
 };
 
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index 3dad65011ff8..b4b1e9c9b10d 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -815,13 +815,6 @@ void TLSWrap::EnableSessionCallbacks(
 }
 
 
-void TLSWrap::OnStreamClose(const FunctionCallbackInfo<Value>& args) {
-  TLSWrap* wrap;
-  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
-
-  wrap->stream_ = nullptr;
-}
-
 
 void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
   TLSWrap* wrap;
@@ -953,7 +946,6 @@ void TLSWrap::Initialize(Local<Object> target,
   env->SetProtoMethod(t, "enableSessionCallbacks", EnableSessionCallbacks);
   env->SetProtoMethod(t, "destroySSL", DestroySSL);
   env->SetProtoMethod(t, "enableCertCb", EnableCertCb);
-  env->SetProtoMethod(t, "onStreamClose", OnStreamClose);
 
   StreamBase::AddMethods<TLSWrap>(env, t, StreamBase::kFlagHasWritev);
   SSLWrap<TLSWrap>::AddMethods(env, t);
diff --git a/src/tls_wrap.h b/src/tls_wrap.h
index f1d53f3e2f5d..19633ea8667f 100644
--- a/src/tls_wrap.h
+++ b/src/tls_wrap.h
@@ -161,7 +161,6 @@ class TLSWrap : public AsyncWrap,
   static void EnableCertCb(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void DestroySSL(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void OnStreamClose(const v8::FunctionCallbackInfo<v8::Value>& args);
 
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
   static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);

@trevnorris
Copy link
Contributor Author

@addaleax ah yup. it should be ssl_ != nullptr && stream_ != nullptr && stream_->IsAlive(). I'll add that in.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks! CI: https://ci.nodejs.org/job/node-test-commit/8576/ (edit: green up to a single Jenkins failure)

virtual ~StreamResource() = default;
virtual ~StreamResource() {
if (!destruct_cb_.is_empty())
destruct_cb_.fn(destruct_cb_.ctx);
Copy link
Member

Choose a reason for hiding this comment

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

If TLSWrap is the only user of this, wouldn't it make more sense to set stream_ = nullptr in ~TLSWrap()?

(Also, that inheritance chain... TLSWrap -> StreamWrap -> StreamBase -> StreamResource, with multiple inheritance, CRTP and ad hoc function pointers thrown in for good measure. Barf!)

Copy link
Contributor Author

@trevnorris trevnorris Mar 22, 2017

Choose a reason for hiding this comment

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

@bnoordhuis setting stream_ = nullptr in ~TLSWrap() actually isn't enough. And actually, just managed to create a new test that still segfaults. So I'm revisiting the patch.

EDIT: The failure I mentioned just happened to occur under similar circumstances, but unrelated to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis okay. so this took me far longer than I'd have liked but I now remember why I made the change this way to begin with.

TLSWrap::stream_ points to the StreamBase* instance that's passed to TLSWrap::TLSWrap(). Each of these pointers are assigned to their own FunctionTemplate instance as an internal aligned pointer, and the lifetime of each is independent from the other. Meaning stream_ may be deleted even though the TLSWrap instance remains alive. In JS they're just attached as properties to each other.

So if the StreamBase instance is closed and released then stream_ will point to invalid memory, even though the TLSWrap instance is still alive. Which means the necessary step is to zero out the stream_ field in the TLSWrap instance, and also add stream != nullptr checks to the native methods like TLSWrap::IsAlive(), so it is no longer pointing to an invalid memory address.

To clarify (if that's possible) TLSWrap inherits from StreamBase and it holds a pointer to another StreamBase instance.

I'll update the commit message with all this information.

FYI all that class template multiple inheritance stuff going on is what lead me to this solution in c0e6c66:

#define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...)                                \
  do {                                                                        \
    *ptr =                                                                    \
        Unwrap<typename node::remove_reference<decltype(**ptr)>::type>(obj);  \
    if (*ptr == nullptr)                                                      \
      return __VA_ARGS__;                                                     \
  } while (0)

In order to handle unwrapping in methods like template<class Base, ...> StreamBase::JSMethod() that need to convert a void* of a class instance in a template method to its parent class, that also has multiple inheritance.

The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: nodejs#11947
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: nodejs#11947
@trevnorris
Copy link
Contributor Author

@bnoordhuis Updated the commit message with some of the specifics I outlined above.

Rebased and new CI: https://ci.nodejs.org/job/node-test-commit/8650/

@trevnorris
Copy link
Contributor Author

All tests are passing. Will land this tomorrow if there are no objections.

@addaleax
Copy link
Member

Landed in 0db49fe...595efd8

@addaleax addaleax closed this Mar 27, 2017
addaleax pushed a commit that referenced this pull request Mar 27, 2017
The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Mar 27, 2017
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@trevnorris
Copy link
Contributor Author

@addaleax thanks much!

@trevnorris trevnorris deleted the notify-on-destruct branch March 27, 2017 22:42
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
This was referenced Apr 19, 2017
MylesBorins added a commit that referenced this pull request May 2, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12497
MylesBorins added a commit that referenced this pull request May 2, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12497
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12497

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12497

Signed-off-by: Ilkka Myller <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12499
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12497
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: nodejs/node#11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: nodejs/node#11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs/node#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs/node#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs/node#11947
   * (Ben Noordhuis) nodejs/node#11898
   * (jBarz) nodejs/node#11776

PR-URL: nodejs/node#12497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants