Skip to content

tls: catch certCbDone exceptions#6887

Closed
indutny wants to merge 3 commits intonodejs:masterfrom
indutny:fix/gh-6822
Closed

tls: catch certCbDone exceptions#6887
indutny wants to merge 3 commits intonodejs:masterfrom
indutny:fix/gh-6822

Conversation

@indutny
Copy link
Member

@indutny indutny commented May 20, 2016 •
edited
Loading

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

@indutny
tls: catch certCbDone exceptions
5d91d5a 
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 indutny mentioned this pull request May 20, 2016
Closed
@indutny
Copy link
Member Author

indutny commented May 20, 2016

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

bnoordhuis
bnoordhuis reviewed May 20, 2016
View reviewed changes
test/parallel/test-tls-empty-sni-context.js Outdated
};

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

@bnoordhuis bnoordhuis May 20, 2016

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

@indutny indutny May 20, 2016

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

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

@bnoordhuis
Copy link
Member

bnoordhuis commented May 21, 2016

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
@indutny
tls: catch certCbDone exceptions
9cac8c8 
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 <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
@indutny @Fishrock123
tls: catch certCbDone exceptions
a6d9178 
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 <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@indutny @rvagg
tls: catch certCbDone exceptions
21e3135 
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 <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 11, 2016

@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

MylesBorins commented Jul 11, 2016

@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
@indutny @MylesBorins
tls: catch certCbDone exceptions
08f0120 
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 <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny @MylesBorins
tls: catch certCbDone exceptions
1e66668 
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 <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny
tls: catch certCbDone exceptions
f711650 
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 <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny
tls: catch certCbDone exceptions
c615896 
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 <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@indutny
tls: catch certCbDone exceptions
481b940 
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 <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@indutny
tls: catch certCbDone exceptions
b956635 
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 <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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.

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

4 participants

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