GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||
GeezerVison is showing github.com — stream_base,tls_wrap: notify on destruct by trevnorris · Pull Request #11947 · nodejs/node Conversation
|
|
Curious, I tried it too but it worked fine (leaving out the one in 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); |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@addaleax ah yup. it should be |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
addaleax
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this 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)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
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!)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
trevnorris
Mar 22, 2017
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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
fd61420 to
ab67615
Compare
|
@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/ |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
All tests are passing. Will land this tomorrow if there are no objections. |
All reactions
-
👍 1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Landed in 0db49fe...595efd8 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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]>
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]>
|
@addaleax thanks much! |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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]>
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 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]>
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]>
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]>
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]>
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]>
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
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
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
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
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]>
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]>
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]>
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]>
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
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
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]>
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]>
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
Reviewers
+1 more reviewer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
7 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.