src: use unique_ptr for requests in crypto #17000

fhinkel · 2017-11-13T21:53:36Z

Instead of raw pointerns, use std::unique_ptr for PBKDF2Request and
RandomBytesRequest. This makes ownership more clear.

Checklist

make -j4 test (UNIX) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

bnoordhuis · 2017-11-14T19:33:01Z

src/node_crypto.cc

@@ -5533,9 +5533,9 @@ void PBKDF2Request::After() {

 void PBKDF2Request::After(uv_work_t* work_req, int status) {
  CHECK_EQ(status, 0);
-  PBKDF2Request* req = ContainerOf(&PBKDF2Request::work_req_, work_req);
+  std::unique_ptr<PBKDF2Request> req(
+    ContainerOf(&PBKDF2Request::work_req_, work_req));


4 space indent. (Line continuation.)

Thanks! I think I have a few of these wrong. Let me check my other PRs.

bnoordhuis · 2017-11-14T19:34:45Z

src/node_crypto.cc

+                                                       saltlen,
+                                                       salt,
+                                                       iter,
+                                                       keylen));


Since you're here, I'd be okay with writing this as:

std::unique_ptr<PBKDF2Request> req( new PBKDF2Request(env, obj, digest, passlen, pass, saltlen, iter, keylen));

Instead of raw pointerns, use std::unique_ptr for PBKDF2Request and RandomBytesRequest. This makes ownership more clear.

fhinkel · 2017-11-15T18:05:42Z

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

fhinkel · 2017-11-15T19:51:47Z

CI failures are unrelated. Landed in 1b093cb.

Instead of raw pointerns, use std::unique_ptr for PBKDF2Request and RandomBytesRequest. This makes ownership more clear. PR-URL: #17000 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr, to the Cpp style guide. Mostly copied from the Google style guide. PR-URL: #17055 Ref: #16970 Ref: #16974 Ref: #17000 Ref: #17012 Ref: #17020 Ref: #17030 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Instead of raw pointerns, use std::unique_ptr for PBKDF2Request and RandomBytesRequest. This makes ownership more clear. PR-URL: #17000 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr, to the Cpp style guide. Mostly copied from the Google style guide. PR-URL: #17055 Ref: #16970 Ref: #16974 Ref: #17000 Ref: #17012 Ref: #17020 Ref: #17030 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins · 2017-12-19T19:42:46Z

This needs to be manually backported to v6.x and v8.x.

6.x does not have pbkdf2, but we might want to backport it in the next minor. Please leave the label on right now and we will revisit the 6.x backport in January

gibfahn · 2017-12-19T19:43:27Z

Compile failure on 8.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' '-DNGHTTP2_STATICLIB' '-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' -I../src -I../tools/msvs/genfiles -I/build/gib/node/out/Release/obj/gen -I../deps/nghttp2/lib/includes -I/build/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  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /build/gib/node/out/Release/.deps//build/gib/node/out/Release/obj.target/node/src/tls_wrap.o.d.raw   -c -o /build/gib/node/out/Release/obj.target/node/src/tls_wrap.o ../src/tls_wrap.cc
../src/node_contextify.cc: In static member function ‘static void node::{anonymous}::ContextifyContext::RunInDebugContext(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_contextify.cc:278:76: warning: ‘static v8::Local<v8::Context> v8::Debug::GetDebugContext(v8::Isolate*)’ is deprecated (declared at ../deps/v8/include/v8-debug.h:208): Use v8-inspector [-Wdeprecated-declarations]
     Local<Context> debug_context = Debug::GetDebugContext(args.GetIsolate());
                                                                            ^
../src/node_contextify.cc:283:75: warning: ‘static bool v8::Debug::SetDebugEventListener(v8::Isolate*, v8::Debug::EventCallback, v8::Local<v8::Value>)’ is deprecated (declared at ../deps/v8/include/v8-debug.h:145): No longer supported [-Wdeprecated-declarations]
       Debug::SetDebugEventListener(args.GetIsolate(), dummy_event_listener);
                                                                           ^
../src/node_contextify.cc:284:63: warning: ‘static v8::Local<v8::Context> v8::Debug::GetDebugContext(v8::Isolate*)’ is deprecated (declared at ../deps/v8/include/v8-debug.h:208): Use v8-inspector [-Wdeprecated-declarations]
       debug_context = Debug::GetDebugContext(args.GetIsolate());
                                                               ^
../src/node_crypto.cc: In function ‘void node::crypto::PBKDF2(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5437:12: error:   from here [-fpermissive]
       goto err;
            ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5427:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5420:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5415:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5409:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5401:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5390:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node_crypto.cc:5478:2: error: jump to label ‘err’ [-fpermissive]
  err:
  ^
../src/node_crypto.cc:5383:10: error:   from here [-fpermissive]
     goto err;
          ^
../src/node_crypto.cc:5447:34: error:   crosses initialization of ‘std::unique_ptr<node::crypto::PBKDF2Request> req’
   std::unique_ptr<PBKDF2Request> req(
                                  ^
../src/node.cc: In function ‘void node::DebugPause(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node.cc:4422:42: warning: ‘static void v8::Debug::DebugBreak(v8::Isolate*)’ is deprecated (declared at ../deps/v8/include/v8-debug.h:151): No longer supported [-Wdeprecated-declarations]
   v8::Debug::DebugBreak(args.GetIsolate());
                                          ^

Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr, to the Cpp style guide. Mostly copied from the Google style guide. PR-URL: #17055 Ref: #16970 Ref: #16974 Ref: #17000 Ref: #17012 Ref: #17020 Ref: #17030 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell · 2018-08-17T19:44:37Z

i'm not convinced this is worth backporting to 8.x

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 13, 2017

jasnell approved these changes Nov 14, 2017

View reviewed changes

bnoordhuis approved these changes Nov 14, 2017

View reviewed changes

cjihrig approved these changes Nov 14, 2017

View reviewed changes

src: use unique_ptr for requests in crypto

e86545e

Instead of raw pointerns, use std::unique_ptr for PBKDF2Request and RandomBytesRequest. This makes ownership more clear.

fhinkel force-pushed the nov/unique_ptr_crypto branch from 701d89c to e86545e Compare November 14, 2017 21:41

fhinkel mentioned this pull request Nov 15, 2017

doc: mention smart pointers in Cpp style guide #17055

Closed

2 tasks

fhinkel closed this Nov 15, 2017

fhinkel added dont-land-on-v6.x and removed dont-land-on-v6.x labels Nov 17, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added backport-requested-v6.x labels Dec 19, 2017

MylesBorins added dont-land-on-v8.x and removed backport-requested-v8.x labels Aug 17, 2018

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use unique_ptr for requests in crypto #17000

src: use unique_ptr for requests in crypto #17000

fhinkel commented Nov 13, 2017

bnoordhuis Nov 14, 2017

fhinkel Nov 14, 2017

bnoordhuis Nov 14, 2017

fhinkel commented Nov 15, 2017

fhinkel commented Nov 15, 2017

MylesBorins commented Dec 19, 2017

gibfahn commented Dec 19, 2017

jasnell commented Aug 17, 2018

src: use unique_ptr for requests in crypto #17000

src: use unique_ptr for requests in crypto #17000

Conversation

fhinkel commented Nov 13, 2017

Checklist

Affected core subsystem(s)

bnoordhuis Nov 14, 2017

Choose a reason for hiding this comment

fhinkel Nov 14, 2017

Choose a reason for hiding this comment

bnoordhuis Nov 14, 2017

Choose a reason for hiding this comment

fhinkel commented Nov 15, 2017

fhinkel commented Nov 15, 2017

MylesBorins commented Dec 19, 2017

gibfahn commented Dec 19, 2017

jasnell commented Aug 17, 2018