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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

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

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3608

@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
@sam-github sam-github mentioned this pull request May 23, 2019
Closed
bnoordhuis
bnoordhuis approved these changes May 23, 2019
View reviewed changes
src/tls_wrap.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23302/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23304/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23313/

Trott
Trott approved these changes May 25, 2019
View reviewed changes
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?

refack
refack suggested changes May 26, 2019
View reviewed changes
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

CI: https://ci.nodejs.org/job/node-test-pull-request/23415/

@sam-github
tls: trace errors can show up as SSL errors
baff45b 
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

CI: https://ci.nodejs.org/job/node-test-pull-request/23424/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/23443/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23449/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/23451/

@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
@sam-github
tls: trace errors can show up as SSL errors
b1bd9e3 
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 <info@bnoordhuis.nl>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@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
@sam-github @targos
tls: trace errors can show up as SSL errors
6548b91 
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 <info@bnoordhuis.nl>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos mentioned this pull request Jun 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

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.

investigate flaky test-tls-enable-trace-cli
8 participants
@sam-github @nodejs-github-bot @Trott @refack @bnoordhuis @addaleax @cjihrig @BethGriggs