Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

stream_base,tls_wrap: notify on destruct#11947

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

trevnorris
Copy link
Contributor

@trevnorristrevnorris commentedMar 20, 2017•
edited
Loading

Classes likeTLSWrap are given a pointer to aStreamBase instance;
stored on the private memberTLSWrap::stream_. Unfortunately the
lifetime ofstream_ is not reported to theTLSWrap 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.

GiveStreamBase a destruct callback to notify when the class is being
cleaned up. Use this inTLSWrap to setstream_ = nullptr.

Checklist
  • make -j4 test (UNIX), orvcbuild 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-botnodejs-github-bot added c++Issues and PRs that require attention from people who are familiar with C++. tlsIssues and PRs related to the tls subsystem. labelsMar 20, 2017
@trevnorristrevnorris mentioned this pull requestMar 20, 2017
4 tasks
Copy link
Member

@addaleaxaddaleax 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
ContributorAuthor

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

@addaleax
Copy link
Member

addaleax commentedMar 20, 2017•
edited
Loading

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

edit:

diff:
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.jsindex 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.ccindex 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.hindex 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
ContributorAuthor

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

Copy link
Member

@addaleaxaddaleax left a comment•
edited
Loading

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 setstream_ = 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
ContributorAuthor

@trevnorristrevnorrisMar 22, 2017•
edited
Loading

Choose a reason for hiding this comment

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

@bnoordhuissettingstream_ = 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
ContributorAuthor

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 theStreamBase* instance that's passed toTLSWrap::TLSWrap(). Each of these pointers are assigned to their ownFunctionTemplate instance as an internal aligned pointer, and the lifetime of each is independent from the other. Meaningstream_ may be deleted even though theTLSWrap instance remains alive. In JS they're just attached as properties to each other.

So if theStreamBase instance is closed and released thenstream_ will point to invalid memory, even though theTLSWrap instance is still alive. Which means the necessary step is to zero out thestream_ field in theTLSWrap instance, and also addstream != nullptr checks to the native methods likeTLSWrap::IsAlive(), so it is no longer pointing to an invalid memory address.

To clarify (if that's possible)TLSWrap inherits fromStreamBase and it holds a pointer to anotherStreamBase 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 inc0e6c66:

#defineASSIGN_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 liketemplate<class Base, ...> StreamBase::JSMethod() that need to convert avoid* 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 asTLSWrap::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. Soit's possible for stream_ to be delete'd while the TLSWrap instance isstill 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 seeif it's available.While the test provided is fixed by this commit, it was also previouslyfixed by478fabf. Regardless, leave the test in for better testing.PR-URL:nodejs#11947
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:nodejs#11947
@trevnorris
Copy link
ContributorAuthor

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

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

addaleax reacted with thumbs up emoji

@addaleax
Copy link
Member

Landed in0db49fe...595efd8

addaleax pushed a commit that referenced this pull requestMar 27, 2017
The TLSWrap constructor is passed a StreamBase* which it stores asTLSWrap::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. Soit's possible for stream_ to be delete'd while the TLSWrap instance isstill 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 seeif it's available.While the test provided is fixed by this commit, it was also previouslyfixed by478fabf. Regardless, leave the test in for better testing.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull requestMar 27, 2017
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
@trevnorris
Copy link
ContributorAuthor

@addaleax thanks much!

@trevnorristrevnorris deleted the notify-on-destruct branchMarch 27, 2017 22:42
MylesBorins pushed a commit that referenced this pull requestMar 28, 2017
The TLSWrap constructor is passed a StreamBase* which it stores asTLSWrap::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. Soit's possible for stream_ to be delete'd while the TLSWrap instance isstill 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 seeif it's available.While the test provided is fixed by this commit, it was also previouslyfixed by478fabf. Regardless, leave the test in for better testing.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull requestMar 28, 2017
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorinsMylesBorins mentioned this pull requestMar 28, 2017
@italoacasasitaloacasas mentioned this pull requestApr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull requestApr 18, 2017
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull requestApr 19, 2017
The TLSWrap constructor is passed a StreamBase* which it stores asTLSWrap::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. Soit's possible for stream_ to be delete'd while the TLSWrap instance isstill 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 seeif it's available.While the test provided is fixed by this commit, it was also previouslyfixed by478fabf. Regardless, leave the test in for better testing.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull requestApr 19, 2017
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull requestApr 19, 2017
The TLSWrap constructor is passed a StreamBase* which it stores asTLSWrap::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. Soit's possible for stream_ to be delete'd while the TLSWrap instance isstill 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 seeif it's available.While the test provided is fixed by this commit, it was also previouslyfixed by478fabf. Regardless, leave the test in for better testing.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull requestApr 19, 2017
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referencedApr 19, 2017
MylesBorins added a commit that referenced this pull requestMay 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)#11776PR-URL:#12499
MylesBorins added a commit that referenced this pull requestMay 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)#11776PR-URL:#12497
MylesBorins added a commit that referenced this pull requestMay 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)#11776PR-URL:#12499
MylesBorins added a commit that referenced this pull requestMay 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)#11776PR-URL:#12497
imyller added a commit to imyller/meta-nodejs that referenced this pull requestMay 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#12499Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull requestMay 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#12497Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull requestMay 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#12499Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull requestMay 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#12497Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
anchnk pushed a commit to anchnk/node that referenced this pull requestMay 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#11776PR-URL:nodejs#12499
anchnk pushed a commit to anchnk/node that referenced this pull requestMay 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#11776PR-URL:nodejs#12497
andrew749 pushed a commit to michielbaird/node that referenced this pull requestJul 19, 2017
The TLSWrap constructor is passed a StreamBase* which it stores asTLSWrap::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. Soit's possible for stream_ to be delete'd while the TLSWrap instance isstill 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 seeif it's available.While the test provided is fixed by this commit, it was also previouslyfixed by478fabf. Regardless, leave the test in for better testing.PR-URL:nodejs/node#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
andrew749 pushed a commit to michielbaird/node that referenced this pull requestJul 19, 2017
This partually reverts commit4cdb0e8.A nullptr check in TSLWrap::IsAlive() and the added test were left.PR-URL:nodejs/node#11947Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Anna Henningsen <anna@addaleax.net>
andrew749 pushed a commit to michielbaird/node that referenced this pull requestJul 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#11776PR-URL:nodejs/node#12497
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@bnoordhuisbnoordhuisbnoordhuis left review comments

@fhinkelfhinkelfhinkel approved these changes

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

Assignees
No one assigned
Labels
c++Issues and PRs that require attention from people who are familiar with C++.tlsIssues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@trevnorris@addaleax@MylesBorins@fhinkel@bnoordhuis@jasnell@nodejs-github-bot
[8]ページ先頭
©2009-2025 Movatter.jp