FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v8.x backport] crypto: do not reach into OpenSSL internals for ThrowCryptoError by yhwang · Pull Request #18327 · nodejs/node · GitHub

/ node Public

[v8.x backport] crypto: do not reach into OpenSSL internals for ThrowCryptoError - #18327

Closed
yhwang wants to merge 1 commit into
nodejs:v8.x-stagingfrom
yhwang:backport-16701-to-v8.x-staging
ConversationCommits (1)ChecksFiles changed
Closed

[v8.x backport] crypto: do not reach into OpenSSL internals for ThrowCryptoError#18327
yhwang wants to merge 1 commit into
nodejs:v8.x-stagingfrom
yhwang:backport-16701-to-v8.x-staging

Conversation

yhwang commented Jan 23, 2018

Copy link
Copy Markdown
Member

There is a perfectly serviceable ERR_get_error function which avoids
having to sniff through the OpenSSL ring buffer like that. It does
return the errors in the opposite order, but that's easily fixed with
std::reverse.

Note this behavior is slightly different in that an ERR_get_error loop
will ultimately clear the error queue, but this is desirable. Leaving
the error queue uncleared means errors in subsequent operations may get
mixed up and cause issues.

Original PR-URL: #16701
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. v8.x labels Jan 23, 2018
yhwang commented Jan 23, 2018
View reviewed changes
Comment thread src/node_crypto.cc Outdated

yhwang Jan 23, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since algorithm header is added above and cmath is included in algorithm, the standard isnan and isinf macros are undefined and new functions are introduced under std namespace. Therefore, need to use std namespace for these 2 functions.
You can see more detailed information in cmath header.

gibfahn requested a review from bnoordhuis January 23, 2018 22:57

gibfahn commented Jan 23, 2018

Copy link
Copy Markdown
Member

@bnoordhuis could you confirm that this makes sense?

gibfahn commented Jan 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/15625/

EDIT: Wrong job, trying again: https://ci.nodejs.org/job/node-test-commit/15632/

gibfahn force-pushed the v8.x-staging branch 2 times, most recently from 68a631f to aff27a1 Compare January 24, 2018 02:46
yhwang force-pushed the backport-16701-to-v8.x-staging branch from ad97c44 to 357ea02 Compare January 24, 2018 18:00

yhwang commented Jan 24, 2018

Copy link
Copy Markdown
Member Author

rebased my branch to upstream/v8.x-staging.

and 2 test cases failed in windows:

not ok 407 parallel/test-tls-server-verify
not ok 506 sequential/test-inspector-bindings # TODO : Fix flaky test

I saw another v8.x backport PR also has the same failures.

gibfahn commented Jan 24, 2018

Copy link
Copy Markdown
Member

CI 2: https://ci.nodejs.org/job/node-test-commit/15662/

yhwang commented Jan 24, 2018

Copy link
Copy Markdown
Member Author

CI 2: https://ci.nodejs.org/job/node-test-commit/15662/

The results are the same as the first one. These 2 test cases still failed in Windows:

not ok 407 parallel/test-tls-server-verify
not ok 505 sequential/test-inspector-async-call-stack # TODO : Fix flaky test

bnoordhuis commented Jan 25, 2018

Copy link
Copy Markdown
Member

@yhwang Can you rebase? The test failures are known flakes.

yhwang force-pushed the backport-16701-to-v8.x-staging branch from 357ea02 to 08bad94 Compare January 25, 2018 19:03

yhwang commented Jan 25, 2018

Copy link
Copy Markdown
Member Author

@bnoordhuis I just rebased. Please kick off another CI to verify it. Thanks.

gibfahn commented Jan 26, 2018

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/15701/

bnoordhuis commented Feb 2, 2018

Copy link
Copy Markdown
Member

@yhwang I'm afraid this needs another rebase.

yhwang force-pushed the backport-16701-to-v8.x-staging branch from 08bad94 to de47005 Compare February 2, 2018 17:24

yhwang commented Feb 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@bnoordhuis sure thing and it's done.

[Edit] Please kick off a CI to verify it it

yhwang force-pushed the backport-16701-to-v8.x-staging branch from de47005 to d13ac52 Compare February 7, 2018 18:49

yhwang commented Feb 7, 2018

Copy link
Copy Markdown
Member Author

Rebase again because of the conflict and please kick off a CI. Thanks.

mhdawson commented Feb 9, 2018

Copy link
Copy Markdown
Member

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

yhwang commented Feb 12, 2018

Copy link
Copy Markdown
Member Author

failed on windows-test. it complains about cctest.exe not found. However, the windows build task passed, I downloaded the binary.tar.gz and the cctest.exe and node.exe inside the tarball work well.

gibfahn commented Feb 18, 2018

Copy link
Copy Markdown
Member

Sorry @yhwang , looks like this needs (yet another) rebase after the V8 update.

yhwang commented Feb 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

rebase failed and need to solve lots of conflict in v8. Let me use a new branch which is from the latest v8.x-staging. that's will be easier.

crypto: do not reach into OpenSSL internals for ThrowCryptoError
c96e3c9 
There is a perfectly serviceable ERR_get_error function which avoids
having to sniff through the OpenSSL ring buffer like that. It does
return the errors in the opposite order, but that's easily fixed with
std::reverse.

Note this behavior is slightly different in that an ERR_get_error loop
will ultimately clear the error queue, but this is desirable. Leaving
the error queue uncleared means errors in subsequent operations may get
mixed up and cause issues.

PR-URL: nodejs#16701
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
yhwang force-pushed the backport-16701-to-v8.x-staging branch from d13ac52 to c96e3c9 Compare February 19, 2018 18:04

yhwang commented Feb 19, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/16377/

gibfahn commented Feb 20, 2018

Copy link
Copy Markdown
Member

rebase failed and need to solve lots of conflict in v8. Let me use a new branch which is from the latest v8.x-staging. that's will be easier.

Yeah I frequently just do reset --hard up/v8.x-staging; git cherry-pick mybranch instead of actually rebasing.

gibfahn commented Feb 20, 2018

Copy link
Copy Markdown
Member

@bnoordhuis would you mind taking a quick look?

gibfahn pushed a commit that referenced this pull request Feb 20, 2018
crypto: do not reach into OpenSSL internals for ThrowCryptoError
8cc0ea7 
There is a perfectly serviceable ERR_get_error function which avoids
having to sniff through the OpenSSL ring buffer like that. It does
return the errors in the opposite order, but that's easily fixed with
std::reverse.

Note this behavior is slightly different in that an ERR_get_error loop
will ultimately clear the error queue, but this is desirable. Leaving
the error queue uncleared means errors in subsequent operations may get
mixed up and cause issues.

PR-URL: #16701
Backport-PR-URL: #18327
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

gibfahn commented Feb 20, 2018

Copy link
Copy Markdown
Member

Landed in 8cc0ea7

gibfahn closed this Feb 20, 2018
yhwang deleted the backport-16701-to-v8.x-staging branch February 20, 2018 19:42
richardlau mentioned this pull request Mar 7, 2018
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis Awaiting requested review from bnoordhuis
1 more reviewer
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL