From 4c725f4e915ad1d3b98fd202ceda0209f18019e1 Mon Sep 17 00:00:00 2001
From: Keyhan Vakil <kvakil@sylph.kvakil.me>
Date: Thu, 4 Aug 2022 15:41:38 -0700
Subject: [PATCH] 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
PR-URL: https://github.com/nodejs/node/pull/44079
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 src/crypto/README.md        |  5 ++++-
 src/crypto/crypto_cipher.cc | 15 ++++++---------
 src/crypto/crypto_util.h    | 25 +++++++++++++++++++------
 3 files changed, 29 insertions(+), 16 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_cipher.cc b/src/crypto/crypto_cipher.cc
index 0ce2708d12ceb8..44b54363dd1ec2 100644
--- a/src/crypto/crypto_cipher.cc
+++ b/src/crypto/crypto_cipher.cc
@@ -536,9 +536,8 @@ void CipherBase::InitIv(const FunctionCallbackInfo<Value>& args) {
   if (UNLIKELY(key_buf.size() > INT_MAX))
     return THROW_ERR_OUT_OF_RANGE(env, "key is too big");
 
-  ArrayBufferOrViewContents<unsigned char> iv_buf;
-  if (!args[2]->IsNull())
-    iv_buf = ArrayBufferOrViewContents<unsigned char>(args[2]);
+  ArrayBufferOrViewContents<unsigned char> iv_buf(
+      !args[2]->IsNull() ? args[2] : Local<Value>());
 
   if (UNLIKELY(!iv_buf.CheckSizeInt32()))
     return THROW_ERR_OUT_OF_RANGE(env, "iv is too big");
@@ -1061,12 +1060,10 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
       return THROW_ERR_OSSL_EVP_INVALID_DIGEST(env);
   }
 
-  ArrayBufferOrViewContents<unsigned char> oaep_label;
-  if (!args[offset + 3]->IsUndefined()) {
-    oaep_label = ArrayBufferOrViewContents<unsigned char>(args[offset + 3]);
-    if (UNLIKELY(!oaep_label.CheckSizeInt32()))
-      return THROW_ERR_OUT_OF_RANGE(env, "oaep_label is too big");
-  }
+  ArrayBufferOrViewContents<unsigned char> oaep_label(
+      !args[offset + 3]->IsUndefined() ? args[offset + 3] : Local<Value>());
+  if (UNLIKELY(!oaep_label.CheckSizeInt32()))
+    return THROW_ERR_OUT_OF_RANGE(env, "oaep_label is too big");
 
   std::unique_ptr<BackingStore> out;
   if (!Cipher<operation, EVP_PKEY_cipher_init, EVP_PKEY_cipher>(
diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
index 69ada7995bd226..4afae1884fe40e 100644
--- a/src/crypto/crypto_util.h
+++ b/src/crypto/crypto_util.h
@@ -699,24 +699,30 @@ template <typename T>
 class ArrayBufferOrViewContents {
  public:
   ArrayBufferOrViewContents() = default;
+  ArrayBufferOrViewContents(const ArrayBufferOrViewContents&) = delete;
+  void operator=(const ArrayBufferOrViewContents&) = delete;
 
   inline explicit ArrayBufferOrViewContents(v8::Local<v8::Value> buf) {
+    if (buf.IsEmpty()) {
+      return;
+    }
+
     CHECK(IsAnyByteSource(buf));
     if (buf->IsArrayBufferView()) {
       auto view = buf.As<v8::ArrayBufferView>();
       offset_ = view->ByteOffset();
       length_ = view->ByteLength();
-      store_ = view->Buffer()->GetBackingStore();
+      data_ = view->Buffer()->Data();
     } else if (buf->IsArrayBuffer()) {
       auto ab = buf.As<v8::ArrayBuffer>();
       offset_ = 0;
       length_ = ab->ByteLength();
-      store_ = ab->GetBackingStore();
+      data_ = ab->Data();
     } else {
       auto sab = buf.As<v8::SharedArrayBuffer>();
       offset_ = 0;
       length_ = sab->ByteLength();
-      store_ = sab->GetBackingStore();
+      data_ = sab->Data();
     }
   }
 
@@ -726,7 +732,7 @@ class ArrayBufferOrViewContents {
     // length is zero, so we have to return something.
     if (size() == 0)
       return &buf;
-    return reinterpret_cast<T*>(store_->Data()) + offset_;
+    return reinterpret_cast<T*>(data_) + offset_;
   }
 
   inline T* data() {
@@ -735,7 +741,7 @@ class ArrayBufferOrViewContents {
     // length is zero, so we have to return something.
     if (size() == 0)
       return &buf;
-    return reinterpret_cast<T*>(store_->Data()) + offset_;
+    return reinterpret_cast<T*>(data_) + offset_;
   }
 
   inline size_t size() const { return length_; }
@@ -775,7 +781,14 @@ class ArrayBufferOrViewContents {
   T buf = 0;
   size_t offset_ = 0;
   size_t length_ = 0;
-  std::shared_ptr<v8::BackingStore> store_;
+  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<v8::Value> EncodeBignum(