crypto: don't reach into OpenSSL internals for ThrowCryptoError by davidben · Pull Request #16701 · nodejs/node · GitHub
Skip to content

crypto: don't reach into OpenSSL internals for ThrowCryptoError#16701

Closed
davidben wants to merge 1 commit into
nodejs:masterfrom
davidben:err-get-sanity
Closed

crypto: don't reach into OpenSSL internals for ThrowCryptoError#16701
davidben wants to merge 1 commit into
nodejs:masterfrom
davidben:err-get-sanity

Conversation

@davidben

@davidben davidben commented Nov 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 desireable. Leaving
the error queue uncleared means errors in subsequent operations may get
mixed up and cause issues.

PS: In future, I'm happy to look over changes to crypto stuff for you all, especially when they reach into internals like that. That's rarely actually necessary. :-)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@nodejs-github-bot 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. labels Nov 3, 2017
@Trott

Trott commented Nov 3, 2017

Copy link
Copy Markdown
Member

@MylesBorins

MylesBorins commented Nov 3, 2017

Copy link
Copy Markdown
Contributor

Consistent error on windows

not ok 71 parallel/test-cli-node-options
  ---
  duration_ms: 2.245
  severity: fail
  stack: |-
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: For "--stack-trace-limit=100", failed to find /(\s*at f \(\[eval\]:1:\d*\)\n){100}/ in: <
    [eval]:1
    (function f() { f(); })();
               ^
    
    RangeError: Maximum call stack size exceeded
        at f ([eval]:1:12)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
    
    >
        at common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-cli-node-options.js:58:12)
        at c:\workspace\node-test-binary-windows\test\common\index.js:522:15
        at ChildProcess.exithandler (child_process.js:279:5)
        at emitTwo (events.js:135:13)
        at ChildProcess.emit (events.js:224:7)
        at maybeClose (internal/child_process.js:943:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:220:5)
  ...

other failures are known flakes (fixed in master)

@apapirovski

apapirovski commented Nov 3, 2017

Copy link
Copy Markdown
Contributor

I don't think that's this PR, it's failing on others too

bnoordhuis
bnoordhuis approved these changes Nov 3, 2017
View reviewed changes
Comment thread src/node_crypto.cc Outdated

@bnoordhuis bnoordhuis Nov 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most minor of nits but could you use for (;;) { here? That's what we most commonly use for indefinite loops.

@davidben davidben Nov 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@davidben
crypto: don't reach into OpenSSL internals for ThrowCryptoError
7844116 
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 desireable. Leaving
the error queue uncleared means errors in subsequent operations may get
mixed up and cause issues.
@cjihrig

cjihrig commented Nov 4, 2017

Copy link
Copy Markdown
Contributor

A new Coverity defect report came out, listing this:

** CID 178579:  Null pointer dereferences  (NULL_RETURNS)
/src/node_crypto.cc: 267 in node::crypto::ThrowCryptoError(node::Environment *, unsigned long, const char *)()

>>>     CID 178579:  Null pointer dereferences  (NULL_RETURNS)
>>>     Dereferencing a null pointer "es".
267       if (es->bottom != es->top) {

I was going to PR a fix, but it looks like this PR will take care of it by removing that code.

@apapirovski

apapirovski commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

ping @nodejs/crypto & @bnoordhuis — can/should this land?

@apapirovski

apapirovski commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

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

@bnoordhuis

bnoordhuis commented Dec 9, 2017

Copy link
Copy Markdown
Member

@apapirovski Yes, it can land, unless more people want/should sign off on it. The freebsd failure seems to be an unrelated flake.

@apapirovski

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

Landed in 7db5370

@apapirovski apapirovski closed this Dec 9, 2017
apapirovski pushed a commit that referenced this pull request Dec 9, 2017
@davidben @apapirovski
crypto: do not reach into OpenSSL internals for ThrowCryptoError
7db5370 
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
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davidben @MylesBorins
crypto: do not reach into OpenSSL internals for ThrowCryptoError
059deef 
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
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davidben @MylesBorins
crypto: do not reach into OpenSSL internals for ThrowCryptoError
8c21430 
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
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@davidben @gibfahn
crypto: do not reach into OpenSSL internals for ThrowCryptoError
88a4d04 
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
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@davidben @gibfahn
crypto: do not reach into OpenSSL internals for ThrowCryptoError
92fbf38 
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
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
@gibfahn

gibfahn commented Dec 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@yhwang yhwang mentioned this pull request Jan 23, 2018
Closed
yhwang pushed a commit to yhwang/node that referenced this pull request Feb 19, 2018
@davidben @yhwang
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>
gibfahn pushed a commit that referenced this pull request Feb 20, 2018
@davidben @gibfahn
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>
@richardlau richardlau mentioned this pull request Mar 7, 2018
Closed
4 tasks
This was referenced Mar 9, 2020
Open
Open
@snyk-bot snyk-bot mentioned this pull request Jul 15, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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++. 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.

9 participants

@davidben @Trott @MylesBorins @apapirovski @cjihrig @bnoordhuis @gibfahn @BethGriggs @nodejs-github-bot