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

tls: trace errors can show up as SSL errors #27841

Closed
wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

Calls to TLS_trace might leave errors on the SSL error stack, which then
get reported as SSL errors instead of being ignored. Wrap TLS_trace to
keep the error stack unchanged.

Fixes: #27636

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@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 May 23, 2019
src/tls_wrap.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Remove the entry for the flaky test from test/parallel/parallel.status?

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

please better names

src/tls_wrap.cc Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented May 27, 2019

Looks like it needs a rebase?

@nodejs-github-bot
Copy link
Collaborator

Calls to TLS_trace might leave errors on the SSL error stack, which then
get reported as SSL errors instead of being ignored. Wrap TLS_trace to
keep the error stack unchanged.

Fixes: nodejs#27636
@refack
Copy link
Contributor

refack commented May 27, 2019

Thank you for addressing my CR.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented May 28, 2019

The test is still failing in CI. Not sure if that's expected (as perhaps it has multiple issues) or if that's a sign that this isn't quite a complete fix?

@sam-github
Copy link
Contributor Author

@Trott latest ci was all green, but I'll kick off another.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor Author

test-cpu-prof failed on FreeBSD, but that doesn't seem related. Resuming.

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor Author

@Trott I am not seeing signs of instability. Can I land this?

@Trott
Copy link
Member

Trott commented May 29, 2019

@Trott I am not seeing signs of instability. Can I land this?

OMG yes, please! Sorry my erroneous observation caused a delay here.

@Trott
Copy link
Member

Trott commented May 29, 2019

(Oh, wait, it wasn't erroneous. At the time, the most recent CI was https://ci.nodejs.org/job/node-test-pull-request/23424/ which shows the test still failing. But whatever, if it's all green now, let's land. Maybe this was needed in combination with something else that landed recently.

@sam-github
Copy link
Contributor Author

Landed in b1bd9e3

@sam-github sam-github closed this May 30, 2019
pull bot pushed a commit to zys-contrib/node that referenced this pull request May 30, 2019
Calls to TLS_trace might leave errors on the SSL error stack, which then
get reported as SSL errors instead of being ignored. Wrap TLS_trace to
keep the error stack unchanged.

Fixes: nodejs#27636

PR-URL: nodejs#27841
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@sam-github sam-github deleted the fix-tls-trace-tests branch May 30, 2019 10:55
targos pushed a commit that referenced this pull request May 31, 2019
Calls to TLS_trace might leave errors on the SSL error stack, which then
get reported as SSL errors instead of being ignored. Wrap TLS_trace to
keep the error stack unchanged.

Fixes: #27636

PR-URL: #27841
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@targos targos mentioned this pull request Jun 3, 2019
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.

investigate flaky test-tls-enable-trace-cli
8 participants