[v8.x backport] crypto: do not reach into OpenSSL internals for ThrowCryptoError #18327
Conversation
nodejs-github-bot
added
c++
| @@ -5422,7 +5413,7 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) { | |||
| } | |||
|
|
|||
| raw_keylen = args[3]->NumberValue(); | |||
| if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen) || | |||
| if (raw_keylen < 0.0 || std::isnan(raw_keylen) || std::isinf(raw_keylen) || | |||
There was a problem hiding this comment.
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.
|
@bnoordhuis could you confirm that this makes sense? |
|
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
yhwang
force-pushed
the
backport-16701-to-v8.x-staging
branch
from
ad97c44 to
357ea02
Compare
|
rebased my branch to upstream/v8.x-staging. and 2 test cases failed in windows: I saw another v8.x backport PR also has the same failures. |
|
The results are the same as the first one. These 2 test cases still failed in Windows: |
|
@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
|
@bnoordhuis I just rebased. Please kick off another CI to verify it. Thanks. |
|
@yhwang I'm afraid this needs another rebase. |
yhwang
force-pushed
the
backport-16701-to-v8.x-staging
branch
from
08bad94 to
de47005
Compare
|
@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
|
Rebase again because of the conflict and please kick off a CI. Thanks. |
|
failed on windows-test. it complains about |
|
Sorry @yhwang , looks like this needs (yet another) rebase after the V8 update. |
|
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. |
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
Yeah I frequently just do |
|
@bnoordhuis would you mind taking a quick look? |
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>
|
Landed in 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.
Original PR-URL: #16701
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl