From 7a1716d161b26b7f58fbba464afde47dbf635207 Mon Sep 17 00:00:00 2001 From: Erik Corry Date: Wed, 13 Nov 2024 16:50:39 +0100 Subject: [PATCH] Adjust BufferSource for sandbox. --- src/workerd/api/blob.c++ | 2 +- src/workerd/api/crypto/crypto.h | 19 +++++---------- src/workerd/api/streams/standard-test.c++ | 6 ++--- src/workerd/jsg/buffersource-test.c++ | 2 +- src/workerd/jsg/buffersource.h | 28 +++++++++++++++-------- src/workerd/jsg/jsg.h | 3 ++- src/workerd/jsg/jsvalue.c++ | 2 +- src/workerd/jsg/value.h | 3 --- 8 files changed, 33 insertions(+), 32 deletions(-) diff --git a/src/workerd/api/blob.c++ b/src/workerd/api/blob.c++ index 74578186c6a..b2e30812418 100644 --- a/src/workerd/api/blob.c++ +++ b/src/workerd/api/blob.c++ @@ -129,7 +129,7 @@ jsg::BufferSource wrap(jsg::Lock& js, kj::Array data) { // the underlying v8::BackingStore is supposed to free the buffer when // it is done with it. Unfortunately ASAN complains about a leak that // will require more investigation. - // return jsg::BufferSource(js, jsg::BackingStore::from(kj::mv(data))); + // return jsg::BufferSource(js, jsg::BackingStore::from(js, kj::mv(data))); } kj::ArrayPtr getPtr(jsg::BufferSource& source) { diff --git a/src/workerd/api/crypto/crypto.h b/src/workerd/api/crypto/crypto.h index b205f861af3..1c868cfb9cf 100644 --- a/src/workerd/api/crypto/crypto.h +++ b/src/workerd/api/crypto/crypto.h @@ -207,9 +207,8 @@ class CryptoKey: public jsg::Object { KJ_SWITCH_ONEOF(publicExponent) { KJ_CASE_ONEOF(array, BigInteger) { if (fixPublicExp) { - // alloc will, by default create a Uint8Array - auto expBack = jsg::BackingStore::alloc(js, array.size()); - expBack.asArrayPtr().copyFrom(array); + auto expCopy = kj::heapArray(array.asPtr()); + jsg::BackingStore expBack = jsg::BackingStore::from(js, kj::mv(expCopy)); return {name, modulusLength, jsg::BufferSource(js, kj::mv(expBack)), hash}; } else { auto expBack = jsg::BackingStore::alloc(js, array.size()); @@ -219,16 +218,10 @@ class CryptoKey: public jsg::Object { } KJ_CASE_ONEOF(source, jsg::BufferSource) { // Should only happen if the flag is enabled and an algorithm field is cloned twice. - if (fixPublicExp) { - // alloc will, by default create a Uint8Array - auto expBack = jsg::BackingStore::alloc(js, source.size()); - expBack.asArrayPtr().copyFrom(source); - return {name, modulusLength, jsg::BufferSource(js, kj::mv(expBack)), hash}; - } else { - auto expBack = jsg::BackingStore::alloc(js, source.size()); - expBack.asArrayPtr().copyFrom(source); - return {name, modulusLength, jsg::BufferSource(js, kj::mv(expBack)), hash}; - } + KJ_ASSERT(fixPublicExp == true); + auto expCopy = kj::heapArray(source.asArrayPtr()); + jsg::BackingStore expBack = jsg::BackingStore::from(js, kj::mv(expCopy)); + return {name, modulusLength, jsg::BufferSource(js, kj::mv(expBack)), hash}; } } KJ_UNREACHABLE; diff --git a/src/workerd/api/streams/standard-test.c++ b/src/workerd/api/streams/standard-test.c++ index 936d752361f..e730936d928 100644 --- a/src/workerd/api/streams/standard-test.c++ +++ b/src/workerd/api/streams/standard-test.c++ @@ -24,16 +24,16 @@ void preamble(auto callback) { } v8::Local toBytes(jsg::Lock& js, kj::String str) { - return jsg::BackingStore::from(str.asBytes().attach(kj::mv(str))).createHandle(js); + return jsg::BackingStore::from(js, str.asBytes().attach(kj::mv(str))).createHandle(js); } jsg::BufferSource toBufferSource(jsg::Lock& js, kj::String str) { - auto backing = jsg::BackingStore::from(str.asBytes().attach(kj::mv(str))).createHandle(js); + auto backing = jsg::BackingStore::from(js, str.asBytes().attach(kj::mv(str))).createHandle(js); return jsg::BufferSource(js, kj::mv(backing)); } jsg::BufferSource toBufferSource(jsg::Lock& js, kj::Array bytes) { - auto backing = jsg::BackingStore::from(kj::mv(bytes)).createHandle(js); + auto backing = jsg::BackingStore::from(js, kj::mv(bytes)).createHandle(js); return jsg::BufferSource(js, kj::mv(backing)); } diff --git a/src/workerd/jsg/buffersource-test.c++ b/src/workerd/jsg/buffersource-test.c++ index 938953e67b7..9cabe01ef2a 100644 --- a/src/workerd/jsg/buffersource-test.c++ +++ b/src/workerd/jsg/buffersource-test.c++ @@ -48,7 +48,7 @@ struct BufferSourceContext: public jsg::Object, public jsg::ContextGlobal { } BufferSource makeBufferSource(jsg::Lock& js) { - return BufferSource(js, BackingStore::from(kj::arr(1, 2, 3))); + return BufferSource(js, BackingStore::from(js, kj::arr(1, 2, 3))); } BufferSource makeArrayBuffer(jsg::Lock& js) { diff --git a/src/workerd/jsg/buffersource.h b/src/workerd/jsg/buffersource.h index f319ac6b867..00e1916fd61 100644 --- a/src/workerd/jsg/buffersource.h +++ b/src/workerd/jsg/buffersource.h @@ -69,19 +69,29 @@ using BufferSourceViewConstructor = v8::Local (*)(Lock&, BackingStore // the byte length, offset, element size, and constructor type allowing the view to be // recreated. // -// The BackingStore can be safely used outside of the isolate lock and can even be passed -// into another isolate if necessary. +// The BackingStore must be created in a particular isolate, but it can be +// safely used outside of the isolate lock. It cannot be passed to a different +// isolate, unless they are in the same isolate group, which means they are in +// the same sandbox. class BackingStore { public: template - static BackingStore from(kj::Array data) { + static BackingStore from(Lock& js, kj::Array data) { // Creates a new BackingStore that takes over ownership of the given kj::Array. + // The bytes may be moved if they are not inside the sandbox already. size_t size = data.size(); - auto ptr = new kj::Array(kj::mv(data)); - return BackingStore( - v8::ArrayBuffer::NewBackingStore(ptr->begin(), size, - [](void*, size_t, void* ptr) { delete reinterpret_cast*>(ptr); }, ptr), - size, 0, getBufferSourceElementSize(), construct, checkIsIntegerType()); + if (js.v8Isolate->GetGroup().SandboxContains(data.begin())) { + auto ptr = new kj::Array(kj::mv(data)); + return BackingStore( + v8::ArrayBuffer::NewBackingStore(ptr->begin(), size, + [](void*, size_t, void* ptr) { delete reinterpret_cast*>(ptr); }, ptr), + size, 0, getBufferSourceElementSize(), construct, checkIsIntegerType()); + } else { + auto result = BackingStore(v8::ArrayBuffer::NewBackingStore(js.v8Isolate, size), size, 0, + getBufferSourceElementSize(), construct, checkIsIntegerType()); + memcpy(result.asArrayPtr().begin(), data.begin(), size); + return result; + } } // Creates a new BackingStore of the given size. @@ -449,7 +459,7 @@ class BufferSourceWrapper { }; inline BufferSource Lock::arrayBuffer(kj::Array data) { - return BufferSource(*this, BackingStore::from(kj::mv(data))); + return BufferSource(*this, BackingStore::from(*this, kj::mv(data))); } } // namespace workerd::jsg diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 4945a950b6e..d8d7dff62bd 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -2603,7 +2603,8 @@ class Lock { BufferSource bytes(kj::Array data) KJ_WARN_UNUSED_RESULT; // Returns a jsg::BufferSource whose underlying JavaScript handle is an ArrayBuffer - // as opposed to the default Uint8Array. + // as opposed to the default Uint8Array. May copy and move the bytes if they are + // not in the right sandbox. BufferSource arrayBuffer(kj::Array data) KJ_WARN_UNUSED_RESULT; enum RegExpFlags { diff --git a/src/workerd/jsg/jsvalue.c++ b/src/workerd/jsg/jsvalue.c++ index f602442bdd7..38c1cf7f747 100644 --- a/src/workerd/jsg/jsvalue.c++ +++ b/src/workerd/jsg/jsvalue.c++ @@ -466,7 +466,7 @@ JsValue Lock::rangeError(kj::StringPtr message) { } BufferSource Lock::bytes(kj::Array data) { - return BufferSource(*this, BackingStore::from(kj::mv(data))); + return BufferSource(*this, BackingStore::from(*this, kj::mv(data))); } JsSymbol Lock::symbol(kj::StringPtr str) { diff --git a/src/workerd/jsg/value.h b/src/workerd/jsg/value.h index 22653a61adc..4242ff718da 100644 --- a/src/workerd/jsg/value.h +++ b/src/workerd/jsg/value.h @@ -931,9 +931,6 @@ class ArrayBufferWrapper { // impossible to support unifying Array and Vector in the future (i.e. making all // Arrays growable). So it may be best to stick with allocating an Array on the // heap after all... - - fprintf(stderr, "begin is %p\n", begin); - auto ownerPtr = new kj::Array(kj::mv(value)); std::unique_ptr backing =