Skip to content

Conversation

@indutny
Copy link
Member

@indutnyindutny 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-botnodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 20, 2016
@indutnyindutny 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
MemberAuthor

};

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
MemberAuthor

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
MemberAuthor

@bnoordhuis everything fixed, PTAL

@indutny
Copy link
MemberAuthor

@bnoordhuis
Copy link
Member

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

@indutny
Copy link
MemberAuthor

Likely unrelated, landing.

@indutny
Copy link
MemberAuthor

Landed in 9cac8c8, thank you!

@indutnyindutny closed this May 25, 2016
@indutnyindutny 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
MemberAuthor

How does the backport look like?

@MylesBorins
Copy link
Contributor

@indutny it landed cleanly

@indutny
Copy link
MemberAuthor

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]>
@MylesBorinsMylesBorins 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tlsIssues 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

@indutny@bnoordhuis@MylesBorins@nodejs-github-bot