Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

tls_wrap: clear errors on return #4515

Closed
wants to merge 1 commit into from
Closed

tls_wrap: clear errors on return #4515

wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 3, 2016

Adopt ClearErrorOnReturn from node_crypto.cc, and use it to clear
errors after SSL_read/SSL_write/SSL_shutdown functions.

See: #4485

@indutny
Copy link
Member Author

indutny commented Jan 3, 2016

I'm not sure how the test case should look like, but this PR fixes #4485 on v0.12 branch.

@indutny indutny added tls Issues and PRs related to the tls subsystem. lts-watch-v4.x c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 3, 2016
This was referenced Jan 3, 2016
Closed
Closed
bnoordhuis
bnoordhuis reviewed Jan 4, 2016
View reviewed changes
src/tls_wrap.cc
@@ -401,6 +400,9 @@ void TLSWrap::ClearOut() {
if (ssl_ == nullptr)
return;

crypto::ClearErrorOnReturn clear_error_on_return;
(void) &clear_error_on_return; // Silence compiler warning.
Copy link
Member

Choose a reason for hiding this comment

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

I think you can drop this line now. IIRC, that was a workaround for a false positive in gcc 4.2 or 4.4.

@bnoordhuis
Copy link
Member

LGTM with a suggestion. Maybe it's better to use MarkPopErrorOnReturn instead though?

@indutny
tls_wrap: clear errors on return
9e1d151 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@indutny indutny force-pushed the fix/tls-leftover-errors branch from c3add8b to 9e1d151 Compare January 4, 2016 13:28
@indutny
Copy link
Member Author

indutny commented Jan 4, 2016

Thank you, force pushed. CI: https://ci.nodejs.org/job/node-test-pull-request/1141/

@indutny
Copy link
Member Author

indutny commented Jan 4, 2016

Failures seems to be unrelated, but just in case: https://ci.nodejs.org/job/node-test-pull-request/1142/

@bnoordhuis
Copy link
Member

LGTM

@indutny
Copy link
Member Author

indutny commented Jan 4, 2016

Landed in 4f87574, thank you!

@indutny indutny closed this Jan 4, 2016
@indutny indutny deleted the fix/tls-leftover-errors branch January 4, 2016 14:30
indutny added a commit that referenced this pull request Jan 4, 2016
@indutny
tls_wrap: clear errors on return
4f87574 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: #4485
PR-URL: #4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@indutny @Fishrock123
tls_wrap: clear errors on return
69343d6 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

@indutny it appears this change is relying on changes to crypto that haven't landed on v4.x yet.

I think it might be related to #4127 but I have not fully dug in. I will revisit this when we do our next release

@indutny
Copy link
Member Author

indutny commented Jan 13, 2016

@thealphanerd Argh, is it because of the set_mark thing? What exactly happens?

@MylesBorins
Copy link
Contributor 

++<<<<<<< 2192fc8e890be53ee2213740c3cba2656affefb3
 +// Forcibly clear OpenSSL's error stack on return. This stops stale errors
 +// from popping up later in the lifecycle of crypto operations where they
 +// would cause spurious failures. It's a rather blunt method, though.
 +// ERR_clear_error() isn't necessarily cheap either.
 +struct ClearErrorOnReturn {
 +  ~ClearErrorOnReturn() { ERR_clear_error(); }
 +};
 +
++=======
++>>>>>>> tls_wrap: clear errors on return

I didn't dig in too deep, but usually those kind of conflicts are due to missing patch's.

@indutny
Copy link
Member Author

indutny commented Jan 13, 2016

@thealphanerd this patch should do it: https://gist.github.com/indutny/41e1992a45b1cb0b062e

@MylesBorins
Copy link
Contributor

making sure we are on the same is the above patch a replacement for this commit or something to put in first? If you don't mind would you be willing to wrap that all up in a single PR and submit against v4.x-staging?

indutny added a commit to indutny/io.js that referenced this pull request Jan 14, 2016
@indutny
tls_wrap: clear errors on return
5f58cff 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@indutny indutny mentioned this pull request Jan 14, 2016
Closed
@indutny
Copy link
Member Author

indutny commented Jan 14, 2016

@thealphanerd it is just a resolved conflicts. See #4687

@indutny indutny mentioned this pull request Jan 15, 2016
Closed
jasnell pushed a commit that referenced this pull request Jan 15, 2016
@indutny @jasnell
tls_wrap: clear errors on return
cfb0945 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: #4485
PR-URL: #4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

v4.x-staging Commit Metadata:
PR-URL: #4709
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@indutny
tls_wrap: clear errors on return
4d37472 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: #4485
PR-URL: #4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

v4.x-staging Commit Metadata:
PR-URL: #4709
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@indutny
tls_wrap: clear errors on return
d60b1fb 
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@indutny @bnoordhuis @MylesBorins