crypto: set env values in Deserialize method by calling CreateNativeK… #35416

ThakurKarthik · 2020-09-29T15:34:45Z

Hi @addaleax @jasnell I added some changes for the issue, I verified in my system the example jasnell has posted earlier, it is not throwing any segmentation fault. Please review the changes.Thanks !

Fixes: #35263

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-09-29T15:34:49Z

Review requested:

@nodejs/crypto

addaleax

This is definitely a five-star first contribution 👍

I think ideally we’d also add a test here (for example so that nobody attempts to remove the extra require() call here), which you could probably base on the test case that @jasnell provided in the original issue :)

addaleax · 2020-09-29T16:35:23Z

src/node_crypto.cc

@@ -3466,6 +3465,9 @@ BaseObjectPtr<BaseObject> NativeKeyObject::KeyObjectTransferData::Deserialize(

  Local<Value> handle = KeyObjectHandle::Create(env, data_).ToLocalChecked();
  Local<Function> key_ctor;
+  Local<Value> arg = FIXED_ONE_BYTE_STRING(env->isolate(),
+                                              "internal/crypto/keys");
+  env->native_module_require()->Call(context, Null(env->isolate()), 1, &arg);


This call here could fail to populate env->crypto_key_object_secret_constructor() etc., mostly because the existence of worker.terminate() means that any call into JS can fail with an exception, regardless of whether the code itself throws something or not.

So I would recommend checking for an exception here, and returning if there is one:

Suggested change

env->native_module_require()->Call(context, Null(env->isolate()), 1, &arg);

if (env->native_module_require()->Call(context, Null(env->isolate()), 1, &arg).IsEmpty())

return {};

Yes i will incorporate this check, how should i proceed with writing a test for these changes? jasnell said he added a check in his open draft pull request. Looks like as part of refactoring, node_crypto.cc file is removed over there.

I think you could basically take the snipped from the issue, add a require('../common'); as the first statement, and maybe a comment that links to the issue, and then just save that as a file in test/parallel/.

Looks like as part of refactoring, node_crypto.cc file is removed over there.

I think that PR might require a rebase or two anyway. Also, this PR should probably land at least on v14.x and v12.x as well, unlike the other one, so the backporting process is easier overall if this PR lands on its own and we don’t worry as much about the other one. :)

…eyObjectClass Fixes: nodejs#35263

…the fix

ThakurKarthik · 2020-09-30T17:36:28Z

Hi @addaleax I have added the requested changes. Please verify. Thanks !

addaleax

Nice, thank you :)

nodejs-github-bot · 2020-09-30T18:23:16Z

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

github-actions · 2020-10-03T14:42:32Z

Commit Queue failed

- Loading data for nodejs/node/pull/35416
✔  Done loading data for nodejs/node/pull/35416
----------------------------------- PR info ------------------------------------
Title      crypto: set env values in Deserialize method by calling CreateNativeK… (#35416)
Author     Thakur Karthik  (@ThakurKarthik, first-time contributor)
Branch     ThakurKarthik:karthik-dev -> nodejs:master
Labels     C++, author ready, crypto
Commits    2
 - crypto: set env values in Deserialize method by calling CreateNativeK…
 - crypto: add check for funciton call fail scenario, add test case for …
Committers 1
 - ThakurKarthik 
PR-URL: https://github.com/nodejs/node/pull/35416
Fixes: https://github.com/nodejs/node/issues/35263
Reviewed-By: Anna Henningsen 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35416
Fixes: https://github.com/nodejs/node/issues/35263
Reviewed-By: Anna Henningsen 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-09-30T18:23:16Z: https://ci.nodejs.org/job/node-test-pull-request/33305/
- Querying data for job/node-test-pull-request/33305/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Tue, 29 Sep 2020 15:34:45 GMT
   ✔  Approvals: 2
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35416#pullrequestreview-499689022
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35416#pullrequestreview-501546639
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

addaleax · 2020-10-03T14:55:35Z

Landed in a8556da, thanks for the PR! 🎉

Fixes: #35263 PR-URL: #35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

jasnell · 2020-10-04T00:57:33Z

Woo! Thank you @ThakurKarthik !

Fixes: #35263 PR-URL: #35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fixes: nodejs#35263 PR-URL: nodejs#35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

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 Sep 29, 2020

addaleax approved these changes Sep 29, 2020

View reviewed changes

ThakurKarthik added 2 commits September 30, 2020 23:02

crypto: set env values in Deserialize method by calling CreateNativeK…

e138130

…eyObjectClass Fixes: nodejs#35263

crypto: add check for funciton call fail scenario, add test case for …

e8edcb5

…the fix

ThakurKarthik force-pushed the karthik-dev branch from 71ae739 to e8edcb5 Compare September 30, 2020 17:33

addaleax approved these changes Sep 30, 2020

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. review wanted PRs that need reviews. labels Sep 30, 2020

addaleax requested a review from jasnell September 30, 2020 18:08

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2020

Trott approved these changes Oct 3, 2020

View reviewed changes

addaleax added commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Oct 3, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 3, 2020

addaleax closed this Oct 3, 2020

addaleax pushed a commit that referenced this pull request Oct 3, 2020

crypto: set env values in KeyObject Deserialize method

a8556da

Fixes: #35263 PR-URL: #35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams pushed a commit that referenced this pull request Oct 6, 2020

crypto: set env values in KeyObject Deserialize method

1f15e34

Fixes: #35263 PR-URL: #35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams mentioned this pull request Oct 6, 2020

V14.13.1 proposal #35525

Merged

MylesBorins pushed a commit that referenced this pull request Nov 3, 2020

crypto: set env values in KeyObject Deserialize method

1ce1206

Fixes: #35263 PR-URL: #35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins mentioned this pull request Nov 3, 2020

v12.20.0 proposal #35950

Merged

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020

crypto: set env values in KeyObject Deserialize method

a6f58c0

Fixes: #35263 PR-URL: #35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021

crypto: set env values in KeyObject Deserialize method

6a6e7af

Fixes: nodejs#35263 PR-URL: nodejs#35416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: set env values in Deserialize method by calling CreateNativeK… #35416

crypto: set env values in Deserialize method by calling CreateNativeK… #35416

ThakurKarthik commented Sep 29, 2020

nodejs-github-bot commented Sep 29, 2020

addaleax left a comment

addaleax Sep 29, 2020

ThakurKarthik Sep 29, 2020

addaleax Sep 29, 2020

ThakurKarthik commented Sep 30, 2020

addaleax left a comment

nodejs-github-bot commented Sep 30, 2020

github-actions bot commented Oct 3, 2020

addaleax commented Oct 3, 2020

jasnell commented Oct 4, 2020

	env->native_module_require()->Call(context, Null(env->isolate()), 1, &arg);
	if (env->native_module_require()->Call(context, Null(env->isolate()), 1, &arg).IsEmpty())
	return {};

crypto: set env values in Deserialize method by calling CreateNativeK… #35416

crypto: set env values in Deserialize method by calling CreateNativeK… #35416

Conversation

ThakurKarthik commented Sep 29, 2020

Checklist

nodejs-github-bot commented Sep 29, 2020

addaleax left a comment

Choose a reason for hiding this comment

addaleax Sep 29, 2020

Choose a reason for hiding this comment

ThakurKarthik Sep 29, 2020

Choose a reason for hiding this comment

addaleax Sep 29, 2020

Choose a reason for hiding this comment

ThakurKarthik commented Sep 30, 2020

addaleax left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 30, 2020

github-actions bot commented Oct 3, 2020

addaleax commented Oct 3, 2020

jasnell commented Oct 4, 2020