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/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 54c7fca2d63..4242ff718da 100644 --- a/src/workerd/jsg/value.h +++ b/src/workerd/jsg/value.h @@ -916,12 +916,6 @@ class ArrayBufferWrapper { // v8::ArrayBuffer::NewBackingStore() that accepts a deleter callback, and arrange for it to // delete an Array placed on the heap. // - // TODO(perf): We could avoid an allocation here, perhaps, by decomposing the kj::Array - // into its component pointer and disposer, and then pass the disposer pointer as the - // "deleter_data" for NewBackingStore. However, KJ doesn't give us any way to decompose an - // Array this way, and it might not want to, as this could make it 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... size_t size = value.size(); if (size == 0) { // BackingStore doesn't call custom deleter if begin is null, which it often is for empty @@ -929,15 +923,33 @@ class ArrayBufferWrapper { return v8::ArrayBuffer::New(isolate, 0); } byte* begin = value.begin(); + if (isolate->GetGroup().SandboxContains(begin)) { + // TODO(perf): We could avoid an allocation here, perhaps, by decomposing the + // kj::Array into its component pointer and disposer, and then pass the disposer + // pointer as the "deleter_data" for NewBackingStore. However, KJ doesn't give us any way + // to decompose an Array this way, and it might not want to, as this could make it + // 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... + auto ownerPtr = new kj::Array(kj::mv(value)); + + std::unique_ptr backing = + v8::ArrayBuffer::NewBackingStore(begin, size, [](void* begin, size_t size, void* ownerPtr) { + delete reinterpret_cast*>(ownerPtr); + }, ownerPtr); + + return v8::ArrayBuffer::New(isolate, kj::mv(backing)); + } else { + // The Array is not already inside the sandbox. We have to make a copy and move it in. + // For performance reasons we might want to throw here and fix all callers to allocate + // inside the sandbox. + std::unique_ptr in_sandbox = + v8::ArrayBuffer::NewBackingStore(isolate, size, v8::BackingStoreInitializationMode::kUninitialized); - auto ownerPtr = new kj::Array(kj::mv(value)); - - std::unique_ptr backing = - v8::ArrayBuffer::NewBackingStore(begin, size, [](void* begin, size_t size, void* ownerPtr) { - delete reinterpret_cast*>(ownerPtr); - }, ownerPtr); + memcpy(in_sandbox->Data(), value.begin(), size); - return v8::ArrayBuffer::New(isolate, kj::mv(backing)); + return v8::ArrayBuffer::New(isolate, kj::mv(in_sandbox)); + } } v8::Local wrap(v8::Local context,