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

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

/ node Public

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

Closed
davidben wants to merge 1 commit into
nodejs:masterfrom
davidben:err-get-sanity
ConversationCommits (1)ChecksFiles changed
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 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 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 commented Nov 3, 2017

Copy link
Copy Markdown
Member

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

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 commented Nov 3, 2017

Copy link
Copy Markdown
Contributor

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

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

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.

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

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

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.

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

Done.

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 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 commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

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

apapirovski commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

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

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 commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

Landed in 7db5370

apapirovski closed this Dec 9, 2017
apapirovski pushed a commit that referenced this pull request Dec 9, 2017
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
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
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 mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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 mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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 mentioned this pull request Dec 20, 2017
Merged

gibfahn commented Dec 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

I backported this to v8.x-staging, and I'm getting this error on Ubuntu 16.04:

../src/node_crypto.cc: In function ‘void node::crypto::PBKDF2(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_crypto.cc:5416:43: error: ‘isnan’ was not declared in this scope
   if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen) ||
                                           ^
../src/node_crypto.cc:5416:43: note: suggested alternative:
In file included from /usr/include/c++/5/random:38:0,
                 from /usr/include/c++/5/bits/stl_algo.h:66,
                 from /usr/include/c++/5/algorithm:62,
                 from ../src/node_crypto.cc:46:
/usr/include/c++/5/cmath:641:5: note:   ‘std::isnan’
     isnan(_Tp __x)
     ^
../src/node_crypto.cc:5416:64: error: ‘isinf’ was not declared in this scope
   if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen) ||
                                                                ^
../src/node_crypto.cc:5416:64: note: suggested alternative:
In file included from /usr/include/c++/5/random:38:0,
                 from /usr/include/c++/5/bits/stl_algo.h:66,
                 from /usr/include/c++/5/algorithm:62,
                 from ../src/node_crypto.cc:46:
/usr/include/c++/5/cmath:621:5: note:   ‘std::isinf’
     isinf(_Tp __x)
     ^
  g++ '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DHAVE_INSPECTOR=1' '-D__POSIX__' '-DHAVE_OPENSSL=1' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' -I../src -I../tools/msvs/genfiles -I/home/gib/node/out/Release/obj/gen -I/home/gib/node/out/Release/obj/gen/include -I../deps/openssl/openssl/include -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/gib/node/out/Release/.deps//home/gib/node/out/Release/obj.target/node/src/node_crypto_clienthello.o.d.raw   -c -o /home/gib/node/out/Release/obj.target/node/src/node_crypto_clienthello.o ../src/node_crypto_clienthello.cc
  g++ '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DHAVE_INSPECTOR=1' '-D__POSIX__' '-DHAVE_OPENSSL=1' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' -I../src -I../tools/msvs/genfiles -I/home/gib/node/out/Release/obj/gen -I/home/gib/node/out/Release/obj/gen/include -I../deps/openssl/openssl/include -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/gib/node/out/Release/.deps//home/gib/node/out/Release/obj.target/node/src/tls_wrap.o.d.raw   -c -o /home/gib/node/out/Release/obj.target/node/src/tls_wrap.o ../src/tls_wrap.cc
node.target.mk:199: recipe for target '/home/gib/node/out/Release/obj.target/node/src/node_crypto.o' failed
make[1]: *** [/home/gib/node/out/Release/obj.target/node/src/node_crypto.o] Error 1

See the CI run for more info https://ci.nodejs.org/job/node-test-commit/14987/

@yhwang took a look, and said:

it should be stdc++ version issue
I added -D_GLIBCXX_USE_C99_FP_MACROS_DYNAMIC=1 and the compile passed

I've removed it from staging for now. Assuming this should be backported, could someone raise a PR to backport to v8.x-staging? The guide is here.

yhwang mentioned this pull request Jan 23, 2018
Closed
yhwang pushed a commit to yhwang/node that referenced this pull request Feb 19, 2018
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
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 mentioned this pull request Mar 7, 2018
Closed
4 tasks
This was referenced Mar 9, 2020
Open
Open
snyk-bot mentioned this pull request Jul 15, 2022
Open
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 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

Back | FazBrowse Home | New Git URL