From 94935b25dc31e690e1961bc50afcc475a63f2ea3 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 31 Jan 2020 10:24:10 +0100 Subject: [PATCH] src: remove duplicate env field from CryptoJob Add a `ThreadPoolWork::env()` getter and use that. To fix the diamond problem in class CompressionStream that inherits from both AsyncWrap and ThreadPoolWork, we add a `CompressionStream::env()` getter that delegates to `AsyncWrap::env()`. Refs: https://github.com/nodejs/node/pull/31554#discussion_r372319787 --- src/node_crypto.cc | 41 ++++++++++++++++++++--------------------- src/node_internals.h | 2 ++ src/node_zlib.cc | 2 ++ 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b589eac6947b13..959232920e358c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -6194,9 +6194,8 @@ bool ECDH::IsKeyPairValid() { // TODO(addaleax): If there is an `AsyncWrap`, it currently has no access to // this object. This makes proper reporting of memory usage impossible. struct CryptoJob : public ThreadPoolWork { - Environment* const env; std::unique_ptr async_wrap; - inline explicit CryptoJob(Environment* env) : ThreadPoolWork(env), env(env) {} + inline explicit CryptoJob(Environment* env) : ThreadPoolWork(env) {} inline void AfterThreadPoolWork(int status) final; virtual void AfterThreadPoolWork() = 0; static inline void Run(std::unique_ptr job, Local wrap); @@ -6207,8 +6206,8 @@ void CryptoJob::AfterThreadPoolWork(int status) { CHECK(status == 0 || status == UV_ECANCELED); std::unique_ptr job(this); if (status == UV_ECANCELED) return; - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); CHECK_EQ(false, async_wrap->persistent().IsWeak()); AfterThreadPoolWork(); } @@ -6249,12 +6248,12 @@ struct RandomBytesJob : public CryptoJob { inline void AfterThreadPoolWork() override { Local arg = ToResult(); - async_wrap->MakeCallback(env->ondone_string(), 1, &arg); + async_wrap->MakeCallback(env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - if (errors.empty()) return Undefined(env->isolate()); - return errors.ToException(env).ToLocalChecked(); + if (errors.empty()) return Undefined(env()->isolate()); + return errors.ToException(env()).ToLocalChecked(); } }; @@ -6306,11 +6305,11 @@ struct PBKDF2Job : public CryptoJob { inline void AfterThreadPoolWork() override { Local arg = ToResult(); - async_wrap->MakeCallback(env->ondone_string(), 1, &arg); + async_wrap->MakeCallback(env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - return Boolean::New(env->isolate(), success.FromJust()); + return Boolean::New(env()->isolate(), success.FromJust()); } inline void Cleanse() { @@ -6386,12 +6385,12 @@ struct ScryptJob : public CryptoJob { inline void AfterThreadPoolWork() override { Local arg = ToResult(); - async_wrap->MakeCallback(env->ondone_string(), 1, &arg); + async_wrap->MakeCallback(env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - if (errors.empty()) return Undefined(env->isolate()); - return errors.ToException(env).ToLocalChecked(); + if (errors.empty()) return Undefined(env()->isolate()); + return errors.ToException(env()).ToLocalChecked(); } inline void Cleanse() { @@ -6720,7 +6719,7 @@ class GenerateKeyPairJob : public CryptoJob { inline void AfterThreadPoolWork() override { Local args[3]; ToResult(&args[0], &args[1], &args[2]); - async_wrap->MakeCallback(env->ondone_string(), 3, args); + async_wrap->MakeCallback(env()->ondone_string(), 3, args); } inline void ToResult(Local* err, @@ -6728,14 +6727,14 @@ class GenerateKeyPairJob : public CryptoJob { Local* privkey) { if (pkey_ && EncodeKeys(pubkey, privkey)) { CHECK(errors_.empty()); - *err = Undefined(env->isolate()); + *err = Undefined(env()->isolate()); } else { if (errors_.empty()) errors_.Capture(); CHECK(!errors_.empty()); - *err = errors_.ToException(env).ToLocalChecked(); - *pubkey = Undefined(env->isolate()); - *privkey = Undefined(env->isolate()); + *err = errors_.ToException(env()).ToLocalChecked(); + *pubkey = Undefined(env()->isolate()); + *privkey = Undefined(env()->isolate()); } } @@ -6744,20 +6743,20 @@ class GenerateKeyPairJob : public CryptoJob { if (public_key_encoding_.output_key_object_) { // Note that this has the downside of containing sensitive data of the // private key. - if (!KeyObject::Create(env, kKeyTypePublic, pkey_).ToLocal(pubkey)) + if (!KeyObject::Create(env(), kKeyTypePublic, pkey_).ToLocal(pubkey)) return false; } else { - if (!WritePublicKey(env, pkey_.get(), public_key_encoding_) + if (!WritePublicKey(env(), pkey_.get(), public_key_encoding_) .ToLocal(pubkey)) return false; } // Now do the same for the private key. if (private_key_encoding_.output_key_object_) { - if (!KeyObject::Create(env, kKeyTypePrivate, pkey_).ToLocal(privkey)) + if (!KeyObject::Create(env(), kKeyTypePrivate, pkey_).ToLocal(privkey)) return false; } else { - if (!WritePrivateKey(env, pkey_.get(), private_key_encoding_) + if (!WritePrivateKey(env(), pkey_.get(), private_key_encoding_) .ToLocal(privkey)) return false; } diff --git a/src/node_internals.h b/src/node_internals.h index 6e50b22af2f915..64a740add2aaa9 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -268,6 +268,8 @@ class ThreadPoolWork { virtual void DoThreadPoolWork() = 0; virtual void AfterThreadPoolWork(int status) = 0; + inline Environment* env() const { return env_; } + private: Environment* env_; uv_work_t work_req_; diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 739e36d69911b2..c82b5da8d6137f 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -256,6 +256,8 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { CHECK_EQ(unreported_allocations_, 0); } + Environment* env() const { return AsyncWrap::env(); } + void Close() { if (write_in_progress_) { pending_close_ = true;