Skip to content

Commit

Permalink
src: remove usages of GetBackingStore in crypto
Browse files Browse the repository at this point in the history
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: #32226
Refs: #43921
PR-URL: #44079
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
kvakil authored and ruyadorno committed Aug 22, 2022
1 parent 2f904bc commit 4755ad5
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 16 deletions.
5 changes: 4 additions & 1 deletion src/crypto/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 6 additions & 9 deletions src/crypto/crypto_cipher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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>(
Expand Down
25 changes: 19 additions & 6 deletions src/crypto/crypto_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand All @@ -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() {
Expand All @@ -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_; }
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 4755ad5

Please sign in to comment.