From 8ccb0aa14400f20bb64cbac0635a92eeb091288c Mon Sep 17 00:00:00 2001 From: ConorDavenport Date: Tue, 28 Jan 2020 16:17:26 +0000 Subject: [PATCH 1/4] src: remove duplicate field env in CryptoJob class Removed field env from cryptojob class, replaced with function get_env() inherited from ThreadPoolWork --- src/node_crypto.cc | 42 +++++++++++++++++++++--------------------- src/node_internals.h | 4 ++++ 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b589eac6947b13..f422da03ce9982 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(get_env()->isolate()); + Context::Scope context_scope(get_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(get_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(get_env()->isolate()); + return errors.ToException(get_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(get_env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - return Boolean::New(env->isolate(), success.FromJust()); + return Boolean::New(get_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(get_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(get_env()->isolate()); + return errors.ToException(get_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(get_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(get_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(get_env()).ToLocalChecked(); + *pubkey = Undefined(get_env()->isolate()); + *privkey = Undefined(get_env()->isolate()); } } @@ -6744,20 +6743,21 @@ 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(get_env(), kKeyTypePublic, pkey_).ToLocal(pubkey)) return false; } else { - if (!WritePublicKey(env, pkey_.get(), public_key_encoding_) + if (!WritePublicKey(get_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(get_env(), kKeyTypePrivate, pkey_) + .ToLocal(privkey)) return false; } else { - if (!WritePrivateKey(env, pkey_.get(), private_key_encoding_) + if (!WritePrivateKey(get_env(), pkey_.get(), private_key_encoding_) .ToLocal(privkey)) return false; } diff --git a/src/node_internals.h b/src/node_internals.h index 2bd0ff78da7c33..2301700712f8e4 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -264,6 +264,10 @@ class ThreadPoolWork { virtual void DoThreadPoolWork() = 0; virtual void AfterThreadPoolWork(int status) = 0; + Environment* get_env() const { + return env_; + } + private: Environment* env_; uv_work_t work_req_; From 241f09dab681aa336aef2268fcfd5875e2612afe Mon Sep 17 00:00:00 2001 From: Conor <43864112+ConorDavenport@users.noreply.github.com> Date: Thu, 30 Jan 2020 09:07:44 +0000 Subject: [PATCH 2/4] Update src/node_internals.h Co-Authored-By: Ben Noordhuis --- src/node_internals.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_internals.h b/src/node_internals.h index 2301700712f8e4..9f0cc7a50c6c67 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -264,7 +264,7 @@ class ThreadPoolWork { virtual void DoThreadPoolWork() = 0; virtual void AfterThreadPoolWork(int status) = 0; - Environment* get_env() const { + Environment* env() const { return env_; } From a0021fda98f352461d121dc9d106fb5a4858a181 Mon Sep 17 00:00:00 2001 From: ConorDavenport Date: Thu, 30 Jan 2020 10:56:02 +0000 Subject: [PATCH 3/4] fixup! Update src/node_internals.h --- src/node_internals.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_internals.h b/src/node_internals.h index 9f0cc7a50c6c67..2301700712f8e4 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -264,7 +264,7 @@ class ThreadPoolWork { virtual void DoThreadPoolWork() = 0; virtual void AfterThreadPoolWork(int status) = 0; - Environment* env() const { + Environment* get_env() const { return env_; } From baed4e7d724b71088ec4e92eddc81eb9de79372c Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2020 12:46:49 -0800 Subject: [PATCH 4/4] fixup! fixup! Update src/node_internals.h --- src/node_crypto.cc | 38 +++++++++++++++++++------------------- src/node_internals.h | 4 +--- src/node_zlib.cc | 26 ++++++++++++++------------ 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f422da03ce9982..459f5479ac422a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -6206,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(get_env()->isolate()); - Context::Scope context_scope(get_env()->context()); + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); CHECK_EQ(false, async_wrap->persistent().IsWeak()); AfterThreadPoolWork(); } @@ -6248,12 +6248,12 @@ struct RandomBytesJob : public CryptoJob { inline void AfterThreadPoolWork() override { Local arg = ToResult(); - async_wrap->MakeCallback(get_env()->ondone_string(), 1, &arg); + async_wrap->MakeCallback(env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - if (errors.empty()) return Undefined(get_env()->isolate()); - return errors.ToException(get_env()).ToLocalChecked(); + if (errors.empty()) return Undefined(env()->isolate()); + return errors.ToException(env()).ToLocalChecked(); } }; @@ -6305,11 +6305,11 @@ struct PBKDF2Job : public CryptoJob { inline void AfterThreadPoolWork() override { Local arg = ToResult(); - async_wrap->MakeCallback(get_env()->ondone_string(), 1, &arg); + async_wrap->MakeCallback(env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - return Boolean::New(get_env()->isolate(), success.FromJust()); + return Boolean::New(env()->isolate(), success.FromJust()); } inline void Cleanse() { @@ -6385,12 +6385,12 @@ struct ScryptJob : public CryptoJob { inline void AfterThreadPoolWork() override { Local arg = ToResult(); - async_wrap->MakeCallback(get_env()->ondone_string(), 1, &arg); + async_wrap->MakeCallback(env()->ondone_string(), 1, &arg); } inline Local ToResult() const { - if (errors.empty()) return Undefined(get_env()->isolate()); - return errors.ToException(get_env()).ToLocalChecked(); + if (errors.empty()) return Undefined(env()->isolate()); + return errors.ToException(env()).ToLocalChecked(); } inline void Cleanse() { @@ -6719,7 +6719,7 @@ class GenerateKeyPairJob : public CryptoJob { inline void AfterThreadPoolWork() override { Local args[3]; ToResult(&args[0], &args[1], &args[2]); - async_wrap->MakeCallback(get_env()->ondone_string(), 3, args); + async_wrap->MakeCallback(env()->ondone_string(), 3, args); } inline void ToResult(Local* err, @@ -6727,14 +6727,14 @@ class GenerateKeyPairJob : public CryptoJob { Local* privkey) { if (pkey_ && EncodeKeys(pubkey, privkey)) { CHECK(errors_.empty()); - *err = Undefined(get_env()->isolate()); + *err = Undefined(env()->isolate()); } else { if (errors_.empty()) errors_.Capture(); CHECK(!errors_.empty()); - *err = errors_.ToException(get_env()).ToLocalChecked(); - *pubkey = Undefined(get_env()->isolate()); - *privkey = Undefined(get_env()->isolate()); + *err = errors_.ToException(env()).ToLocalChecked(); + *pubkey = Undefined(env()->isolate()); + *privkey = Undefined(env()->isolate()); } } @@ -6743,21 +6743,21 @@ 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(get_env(), kKeyTypePublic, pkey_).ToLocal(pubkey)) + if (!KeyObject::Create(env(), kKeyTypePublic, pkey_).ToLocal(pubkey)) return false; } else { - if (!WritePublicKey(get_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(get_env(), kKeyTypePrivate, pkey_) + if (!KeyObject::Create(env(), kKeyTypePrivate, pkey_) .ToLocal(privkey)) return false; } else { - if (!WritePrivateKey(get_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 2301700712f8e4..c6b833393d7dff 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -264,9 +264,7 @@ class ThreadPoolWork { virtual void DoThreadPoolWork() = 0; virtual void AfterThreadPoolWork(int status) = 0; - Environment* get_env() const { - return env_; - } + Environment* env() const { return env_; } private: Environment* env_; diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 739e36d69911b2..1b7fd788b959f9 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -348,7 +348,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { if (!async) { // sync version - env()->PrintSyncTrace(); + AsyncWrap::env()->PrintSyncTrace(); DoThreadPoolWork(); if (CheckError()) { UpdateWriteResult(); @@ -397,8 +397,9 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { CHECK_EQ(status, 0); - HandleScope handle_scope(env()->isolate()); - Context::Scope context_scope(env()->context()); + Environment* env = AsyncWrap::env(); + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); if (!CheckError()) return; @@ -406,7 +407,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { UpdateWriteResult(); // call the write() cb - Local cb = PersistentToLocal::Default(env()->isolate(), + Local cb = PersistentToLocal::Default(env->isolate(), write_js_callback_); MakeCallback(cb, 0, nullptr); @@ -416,16 +417,17 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { // TODO(addaleax): Switch to modern error system (node_errors.h). void EmitError(const CompressionError& err) { + Environment* env = AsyncWrap::env(); // If you hit this assertion, you forgot to enter the v8::Context first. - CHECK_EQ(env()->context(), env()->isolate()->GetCurrentContext()); + CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - HandleScope scope(env()->isolate()); + HandleScope scope(env->isolate()); Local args[3] = { - OneByteString(env()->isolate(), err.message), - Integer::New(env()->isolate(), err.err), - OneByteString(env()->isolate(), err.code) + OneByteString(env->isolate(), err.message), + Integer::New(env->isolate(), err.err), + OneByteString(env->isolate(), err.code) }; - MakeCallback(env()->onerror_string(), arraysize(args), args); + MakeCallback(env->onerror_string(), arraysize(args), args); // no hope of rescue. write_in_progress_ = false; @@ -454,7 +456,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { void InitStream(uint32_t* write_result, Local write_js_callback) { write_result_ = write_result; - write_js_callback_.Reset(env()->isolate(), write_js_callback); + write_js_callback_.Reset(AsyncWrap::env()->isolate(), write_js_callback); init_done_ = true; } @@ -500,7 +502,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { if (report == 0) return; CHECK_IMPLIES(report < 0, zlib_memory_ >= static_cast(-report)); zlib_memory_ += report; - env()->isolate()->AdjustAmountOfExternalAllocatedMemory(report); + AsyncWrap::env()->isolate()->AdjustAmountOfExternalAllocatedMemory(report); } struct AllocScope {