crypto: don't reach into OpenSSL internals for ThrowCryptoError #16701
Conversation
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
nodejs-github-bot added c++ 
Member
Contributor
| Consistent error on windows other failures are known flakes (fixed in master) |
Contributor
| I don't think that's this PR, it's failing on others too |
bnoordhuis approved these changes Nov 3, 2017
src/node_crypto.cc Outdated
Member
There was a problem hiding this comment.
Most minor of nits but could you use for (;;) { here? That's what we most commonly use for indefinite loops.
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.
davidben force-pushed the err-get-sanity branch from 8aba98b to 7844116 Compare 
Contributor
| A new Coverity defect report came out, listing this: I was going to PR a fix, but it looks like this PR will take care of it by removing that code. |
Contributor
| ping @nodejs/crypto & @bnoordhuis — can/should this land? |
Contributor
Member
| @apapirovski Yes, it can land, unless more people want/should sign off on it. The freebsd failure seems to be an unrelated flake. |
Contributor
| Landed in 7db5370 |
apapirovski pushed a commit that referenced this pull request Dec 9, 2017
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
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
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>
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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>
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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>
Merged
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 1See the CI run for more info https://ci.nodejs.org/job/node-test-commit/14987/ @yhwang took a look, and said:
I've removed it from staging for now. Assuming this should be backported, could someone raise a PR to backport to |
yhwang pushed a commit to yhwang/node that referenced this pull request Feb 19, 2018
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
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>
4 tasks
This was referenced Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
There is a perfectly serviceable
ERR_get_errorfunction which avoidshaving 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_errorloopwill 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), orvcbuild test(Windows) passesAffected core subsystem(s)
crypto