From 55bcf9a9636698d1cb0115597a119b889a9a2af2 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Thu, 21 Jul 2022 04:39:54 +0000 Subject: [PATCH 1/2] src: remove usages of GetBackingStore in crypto This removes all usages of GetBackingStore in `crypto`. See the linked issue for an explanation. Note: I am not sure of the lifetime semantics intended by `ArrayBufferOrViewContents` -- I am pretty sure it is correct based on a manual audit of the callsites, but please ensure that it is correct. Refs: https://github.com/nodejs/node/issues/32226 Refs: https://github.com/nodejs/node/pull/43921 --- src/crypto/README.md | 5 ++++- src/crypto/crypto_util.h | 12 ++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/crypto/README.md b/src/crypto/README.md index 1f5e5b20448499..c58f3cb118089f 100644 --- a/src/crypto/README.md +++ b/src/crypto/README.md @@ -112,12 +112,15 @@ the `ByteSource::Builder` without releasing it as a `ByteSource`. ### `ArrayBufferOrViewContents` -The `ArrayBufferOfViewContents` class is a helper utility that abstracts +The `ArrayBufferOrViewContents` class is a helper utility that abstracts `ArrayBuffer`, `TypedArray`, or `DataView` inputs and provides access to their underlying data pointers. It is used extensively through `src/crypto` to make it easier to deal with inputs that allow any `ArrayBuffer`-backed object. +The lifetime of `ArrayBufferOrViewContents` should not exceed the +lifetime of its input. + ### Key objects Most crypto operations involve the use of keys -- cryptographic inputs diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h index 9e33b3777b00d7..a7ea69cb27ea6d 100644 --- a/src/crypto/crypto_util.h +++ b/src/crypto/crypto_util.h @@ -703,17 +703,17 @@ class ArrayBufferOrViewContents { auto view = buf.As(); offset_ = view->ByteOffset(); length_ = view->ByteLength(); - store_ = view->Buffer()->GetBackingStore(); + data_ = view->Buffer()->Data(); } else if (buf->IsArrayBuffer()) { auto ab = buf.As(); offset_ = 0; length_ = ab->ByteLength(); - store_ = ab->GetBackingStore(); + data_ = ab->Data(); } else { auto sab = buf.As(); offset_ = 0; length_ = sab->ByteLength(); - store_ = sab->GetBackingStore(); + data_ = sab->Data(); } } @@ -723,7 +723,7 @@ class ArrayBufferOrViewContents { // length is zero, so we have to return something. if (size() == 0) return &buf; - return reinterpret_cast(store_->Data()) + offset_; + return reinterpret_cast(data_) + offset_; } inline T* data() { @@ -732,7 +732,7 @@ class ArrayBufferOrViewContents { // length is zero, so we have to return something. if (size() == 0) return &buf; - return reinterpret_cast(store_->Data()) + offset_; + return reinterpret_cast(data_) + offset_; } inline size_t size() const { return length_; } @@ -772,7 +772,7 @@ class ArrayBufferOrViewContents { T buf = 0; size_t offset_ = 0; size_t length_ = 0; - std::shared_ptr store_; + void* data_ = 0; }; v8::MaybeLocal EncodeBignum( From bba5c5ec5c0bd0f264b2befaef819c25d4dd249f Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Mon, 1 Aug 2022 20:47:21 +0000 Subject: [PATCH 2/2] fixup! src: remove usages of GetBackingStore in crypto --- src/crypto/crypto_cipher.cc | 15 ++++++--------- src/crypto/crypto_util.h | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/crypto/crypto_cipher.cc b/src/crypto/crypto_cipher.cc index 6c663a2b21d0a2..341fe88b3f2404 100644 --- a/src/crypto/crypto_cipher.cc +++ b/src/crypto/crypto_cipher.cc @@ -523,9 +523,8 @@ void CipherBase::InitIv(const FunctionCallbackInfo& args) { if (UNLIKELY(key_buf.size() > INT_MAX)) return THROW_ERR_OUT_OF_RANGE(env, "key is too big"); - ArrayBufferOrViewContents iv_buf; - if (!args[2]->IsNull()) - iv_buf = ArrayBufferOrViewContents(args[2]); + ArrayBufferOrViewContents iv_buf( + !args[2]->IsNull() ? args[2] : Local()); if (UNLIKELY(!iv_buf.CheckSizeInt32())) return THROW_ERR_OUT_OF_RANGE(env, "iv is too big"); @@ -1048,12 +1047,10 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { return THROW_ERR_OSSL_EVP_INVALID_DIGEST(env); } - ArrayBufferOrViewContents oaep_label; - if (!args[offset + 3]->IsUndefined()) { - oaep_label = ArrayBufferOrViewContents(args[offset + 3]); - if (UNLIKELY(!oaep_label.CheckSizeInt32())) - return THROW_ERR_OUT_OF_RANGE(env, "oaep_label is too big"); - } + ArrayBufferOrViewContents oaep_label( + !args[offset + 3]->IsUndefined() ? args[offset + 3] : Local()); + if (UNLIKELY(!oaep_label.CheckSizeInt32())) + return THROW_ERR_OUT_OF_RANGE(env, "oaep_label is too big"); std::unique_ptr out; if (!Cipher( diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h index a7ea69cb27ea6d..5c58bb2754ad93 100644 --- a/src/crypto/crypto_util.h +++ b/src/crypto/crypto_util.h @@ -696,8 +696,14 @@ template class ArrayBufferOrViewContents { public: ArrayBufferOrViewContents() = default; + ArrayBufferOrViewContents(const ArrayBufferOrViewContents&) = delete; + void operator=(const ArrayBufferOrViewContents&) = delete; inline explicit ArrayBufferOrViewContents(v8::Local buf) { + if (buf.IsEmpty()) { + return; + } + CHECK(IsAnyByteSource(buf)); if (buf->IsArrayBufferView()) { auto view = buf.As(); @@ -772,7 +778,14 @@ class ArrayBufferOrViewContents { T buf = 0; size_t offset_ = 0; size_t length_ = 0; - void* data_ = 0; + void* data_ = nullptr; + + // Declaring operator new and delete as deleted is not spec compliant. + // Therefore declare them private instead to disable dynamic alloc + void* operator new(size_t); + void* operator new[](size_t); + void operator delete(void*); + void operator delete[](void*); }; v8::MaybeLocal EncodeBignum(