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: catch certCbDone exceptions #6887

Closed
wants to merge 3 commits into from
Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented May 20, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Catch and emit certCbDone exceptions instead of throwing them as
uncaughtException and crashing the whole process.

Fix: #6822

cc @nodejs/crypto

Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: nodejs#6822
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 20, 2016
@indutny indutny added c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels May 20, 2016
@indutny
Copy link
Member Author

indutny commented May 20, 2016

};

const server = tls.createServer(options, (c) => {
assert(false, 'Should not be called');
Copy link
Member

Choose a reason for hiding this comment

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

common.fail

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
Member Author

indutny commented May 20, 2016

@bnoordhuis everything fixed, PTAL

@indutny
Copy link
Member Author

indutny commented May 20, 2016

@bnoordhuis
Copy link
Member

LGTM but there are test failures on the Windows bots that may or may not be related:

@indutny
Copy link
Member Author

indutny commented May 25, 2016

Likely unrelated, landing.

@indutny
Copy link
Member Author

indutny commented May 25, 2016

Landed in 9cac8c8, thank you!

@indutny indutny closed this May 25, 2016
@indutny indutny deleted the fix/gh-6822 branch May 25, 2016 20:07
indutny added a commit that referenced this pull request May 25, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: nodejs#6822
PR-URL: nodejs#6887
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

@indutny we are getting failures backporting this one to lts

Path: parallel/test-tls-empty-sni-context
Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-tls-empty-sni-context.js
--- TIMEOUT ---

@indutny
Copy link
Member Author

indutny commented Jul 11, 2016

How does the backport look like?

@MylesBorins
Copy link
Contributor

@indutny it landed cleanly

@indutny
Copy link
Member Author

indutny commented Jul 12, 2016

tlsClientError => clientError ;)

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: #6822
PR-URL: #6887
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting null cert and null issuer in OCSPRequest which leads to crash
4 participants