From 93d7eb3b1947531ec281b7242ec4b2d72991a34c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 09:05:10 +0100 Subject: [PATCH 01/10] Give native errors one serialization format for C++ and Rust JSG The format in which a native error is written as a host object was defined twice: by jsg::Serializer and jsg::Deserializer (ErrorTag, getErrorTagFromName, toJsError), and by Rust JSG's serialization shims (ERROR_TYPES, writeError, readError), kept equal only by byte-equality tests. Move the shims' version, which uses V8 alone, into workerd/jsg/ser.h as writeNativeError() and readNativeError(), beside writeApiObject() and readApiObject(), and have both serializers call it. The error types become one table whose order is the format. readNativeError() takes PreserveStack for jsg::Deserializer's preserveStackInErrors option; the Rust shim passes YES, as before. The bytes written are unchanged, and so is what JavaScript sees: - The name is compared as UTF-8 up to the first NUL, as kj::str(v8::Local) compared it. - A message that is not a string still throws a DataCloneError DOMException with the same message, now created directly rather than tunneled through a KJ exception. That exception was never internal, so it was neither logged nor reported, and still isn't. - Truncated data still throws "internal error; reference = ". - Reading still runs with JavaScript execution disallowed: jsg::Deserializer disallows it around the whole read, and the nested reads no longer disallow it again. Behavior change: how jsg::Deserializer reports a native error cut short before its type. It was a KJ_ASSERT failure, converted by exceptionToJs(), which reported it to the isolate's observer (reportInternalException) and logged the exception, with its source location, as a "jsgInternalError". It is now thrown with throwInternalError(isolate, message), as the Rust shims throw it: only "deserialization failure, possible corruption" is logged, at ERROR, with the reference id, and the observer is not told. Other corrupt data that jsg::Deserializer reads (a truncated host-object tag, an API object's fields) is reported as before. Such data comes only from a bug or tampering, never from jsg::Serializer. A new ser-test case checks what jsg::Deserializer throws for both malformed native errors; it passes before and after this change. Suggested in review of #8000. --- src/rust/jsg/README.md | 5 +- src/rust/jsg/ser.c++ | 164 +-------------- src/workerd/jsg/ser-test.c++ | 30 +++ src/workerd/jsg/ser.c++ | 388 ++++++++++++++++------------------- src/workerd/jsg/ser.h | 27 ++- 5 files changed, 246 insertions(+), 368 deletions(-) diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index d7337f852ce..69433023027 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -109,8 +109,9 @@ Don't add C++ JSG to the shims. This includes `jsg::Lock` (including `Lock::from `CONSTRUCTS` in `cpp-jsg-allowlist-test.py` has the full list. Using `Wrappable`, `GcVisitor`, `IsolateBase` or the module registries is fine for now, since Rust JSG still shares them with C++ JSG. So are thin calls into `src/workerd/io`, and the serialization hooks in -`workerd/jsg/ser.h` (`writeApiObject`, `readApiObject` and `newDataCloneError`), through which -workerd's API objects serialize, since their serializers are written against C++ JSG. +`workerd/jsg/ser.h`: `writeApiObject`, `readApiObject` and `newDataCloneError`, through which +workerd's API objects serialize, since their serializers are written against C++ JSG, and +`writeNativeError` and `readNativeError`, which give native errors one format for both. Bad: a shim that wraps a V8 call in `JSG_TRY(js) { ... } JSG_CATCH(error) { kj::throwException(createTunneledException(...)); }`. Fix: catch with a `v8::TryCatch` in the diff --git a/src/rust/jsg/ser.c++ b/src/rust/jsg/ser.c++ index b4ce1f489b8..9ebb2b95999 100644 --- a/src/rust/jsg/ser.c++ +++ b/src/rust/jsg/ser.c++ @@ -4,8 +4,10 @@ // Structured serialization for Rust JSG, in the format of C++ JSG's jsg::Serializer and // jsg::Deserializer (workerd/jsg/ser.c++), which can read what these shims write and the other -// way round. They drive V8's serializer with delegates of their own and hand workerd API objects, -// such as Blob and Headers, to C++ JSG through writeApiObject() and readApiObject(). +// way round. They drive V8's serializer with delegates of their own. Native errors are written and +// read by writeNativeError() and readNativeError(), which jsg::Serializer and jsg::Deserializer +// use too, and workerd API objects, such as Blob and Headers, are handed to C++ JSG through +// writeApiObject() and readApiObject(). #include "ffi.h" @@ -23,58 +25,6 @@ namespace workerd::rust::jsg { namespace { -// jsg::Serializer's tag for a native error written as a host object. A uint32 error tag follows: -// 0 for a name that is written out, or the 1-based index of the name in ERROR_TYPES. -constexpr uint32_t SERIALIZATION_TAG_NATIVE_ERROR = 10; -constexpr uint32_t ERROR_TAG_UNKNOWN = 0; - -struct ErrorType { - kj::StringPtr name; - v8::Local (*create)(v8::Local message, v8::Local options); -}; - -// In the order of jsg::Serializer's ErrorTag, which is part of the format. -const ErrorType ERROR_TYPES[] = { - {"Error"_kj, v8::Exception::Error}, - {"TypeError"_kj, v8::Exception::TypeError}, - {"RangeError"_kj, v8::Exception::RangeError}, - {"ReferenceError"_kj, v8::Exception::ReferenceError}, - {"SyntaxError"_kj, v8::Exception::SyntaxError}, - {"WasmCompileError"_kj, v8::Exception::WasmCompileError}, - {"WasmLinkError"_kj, v8::Exception::WasmLinkError}, - {"WasmRuntimeError"_kj, v8::Exception::WasmRuntimeError}, - {"WasmSuspendError"_kj, v8::Exception::WasmSuspendError}, - {"EvalError"_kj, v8::Exception::EvalError}, - {"URIError"_kj, v8::Exception::URIError}, - {"AggregateError"_kj, v8::Exception::AggregateError}, - {"SuppressedError"_kj, v8::Exception::SuppressedError}, -}; - -// The tag of the error type named `name`, compared as jsg::Serializer compares it: as UTF-8, up to -// the first NUL. -uint32_t errorTagOf(v8::Isolate* isolate, v8::Local name) { - v8::String::Utf8Value utf8(isolate, name); - if (*utf8 == nullptr) return ERROR_TAG_UNKNOWN; - kj::StringPtr str(*utf8); - for (auto i: kj::indices(ERROR_TYPES)) { - if (ERROR_TYPES[i].name == str) return i + 1; - } - return ERROR_TAG_UNKNOWN; -} - -// A plain `Error` for a tag that names no error type, as jsg::Deserializer creates. -v8::Local newError(uint32_t tag, v8::Local message) { - auto create = - tag >= 1 && tag <= kj::size(ERROR_TYPES) ? ERROR_TYPES[tag - 1].create : v8::Exception::Error; - return create(message, {}).As(); -} - -v8::Local newString(v8::Isolate* isolate, kj::StringPtr str) { - return v8::String::NewFromUtf8( - isolate, str.begin(), v8::NewStringType::kNormal, static_cast(str.size())) - .ToLocalChecked(); -} - // Throws the DOMException named DataCloneError that jsg::Serializer throws, unless execution is // terminating. void throwDataCloneError(v8::Isolate* isolate, v8::Local message) { @@ -126,7 +76,7 @@ class Serializer final: public v8::ValueSerializer::Delegate { v8::Maybe WriteHostObject(v8::Isolate*, v8::Local object) override { if (errorsAreHostObjects && object->IsNativeError()) { - return writeError(object); + return ::workerd::jsg::writeNativeError(isolate, ser, object); } return ::workerd::jsg::writeApiObject(isolate, ser, object); } @@ -143,56 +93,6 @@ class Serializer final: public v8::ValueSerializer::Delegate { } private: - // Writes `error` as jsg::Serializer does: its type, its name if the type is not a known one, its - // message, and its other own properties as a plain object. Reading the properties can run - // JavaScript; returns nothing if it throws, leaving the exception pending. - v8::Maybe writeError(v8::Local error) { - auto context = isolate->GetCurrentContext(); - auto nameKey = newString(isolate, "name"_kj); - auto messageKey = newString(isolate, "message"_kj); - - ser.WriteUint32(SERIALIZATION_TAG_NATIVE_ERROR); - - v8::Local name; - v8::Local nameString; - if (!error->Get(context, nameKey).ToLocal(&name) || - !name->ToDetailString(context).ToLocal(&nameString)) { - return v8::Nothing(); - } - auto tag = errorTagOf(isolate, nameString); - ser.WriteUint32(tag); - if (tag == ERROR_TAG_UNKNOWN && ser.WriteValue(context, name).IsNothing()) { - return v8::Nothing(); - } - - v8::Local message; - if (!error->Get(context, messageKey).ToLocal(&message) || - ser.WriteValue(context, message).IsNothing()) { - return v8::Nothing(); - } - - v8::Local keys; - if (!error - ->GetPropertyNames(context, v8::KeyCollectionMode::kOwnOnly, - v8::PropertyFilter::ALL_PROPERTIES, v8::IndexFilter::kSkipIndices) - .ToLocal(&keys)) { - return v8::Nothing(); - } - auto properties = v8::Object::New(isolate); - for (uint32_t i = 0; i < keys->Length(); i++) { - v8::Local key; - if (!keys->Get(context, i).ToLocal(&key)) return v8::Nothing(); - // The name and message are written above. - if (key->StrictEquals(nameKey) || key->StrictEquals(messageKey)) continue; - v8::Local value; - if (!error->Get(context, key).ToLocal(&value) || - properties->Set(context, key, value).IsNothing()) { - return v8::Nothing(); - } - } - return ser.WriteValue(context, properties); - } - v8::Isolate* isolate; bool errorsAreHostObjects; v8::LocalVector sharedArrayBuffers; @@ -228,7 +128,9 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { v8::MaybeLocal ReadHostObject(v8::Isolate*) override { uint32_t tag; if (!deser.ReadUint32(&tag)) return throwCorrupt(); - if (tag == SERIALIZATION_TAG_NATIVE_ERROR) return readError(); + if (tag == ::workerd::jsg::SERIALIZATION_TAG_NATIVE_ERROR) { + return ::workerd::jsg::readNativeError(isolate, deser, ::workerd::jsg::PreserveStack::YES); + } return ::workerd::jsg::readApiObject(isolate, deser, tag); } @@ -236,56 +138,6 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { // to by number. private: - // Reads an error that Serializer::writeError() wrote, as jsg::Deserializer does by default - // (with preserveStackInErrors). Runs no JavaScript. - v8::MaybeLocal readError() { - auto context = isolate->GetCurrentContext(); - - uint32_t tag; - if (!deser.ReadUint32(&tag)) return throwCorrupt(); - v8::Local name; - if (tag == ERROR_TAG_UNKNOWN && !deser.ReadValue(context).ToLocal(&name)) return {}; - - v8::Local message; - if (!deser.ReadValue(context).ToLocal(&message)) return {}; - if (!message->IsString()) { - throwDataCloneError( - isolate, newString(isolate, "Deserialization failed: error message is not a string"_kj)); - return {}; - } - auto error = newError(tag, message.As()); - // The name is set as an own property, since an error's `name` is usually inherited from its - // prototype. - if (!name.IsEmpty() && - error->DefineOwnProperty(context, newString(isolate, "name"_kj), name).IsNothing()) { - return {}; - } - - v8::Local properties; - if (!deser.ReadValue(context).ToLocal(&properties)) return {}; - if (properties->IsObject()) { - auto object = properties.As(); - v8::Local keys; - if (!object - ->GetPropertyNames(context, v8::KeyCollectionMode::kOwnOnly, - v8::PropertyFilter::ALL_PROPERTIES, v8::IndexFilter::kSkipIndices) - .ToLocal(&keys)) { - return {}; - } - for (uint32_t i = 0; i < keys->Length(); i++) { - v8::Local key; - v8::Local value; - if (!keys->Get(context, i).ToLocal(&key)) return {}; - // A data property, rather than Set(), so that no setter on the error's prototype runs. - if (!object->Get(context, key).ToLocal(&value) || - error->CreateDataProperty(context, key.As(), value).IsNothing()) { - return {}; - } - } - } - return error; - } - v8::MaybeLocal throwCorrupt() { isolate_throw_internal_error(isolate, "deserialization failure, possible corruption"); return {}; diff --git a/src/workerd/jsg/ser-test.c++ b/src/workerd/jsg/ser-test.c++ index 084c60a0577..ed297c1243f 100644 --- a/src/workerd/jsg/ser-test.c++ +++ b/src/workerd/jsg/ser-test.c++ @@ -232,6 +232,12 @@ struct SerTestContext: public ContextGlobalObject { return jsg::structuredClone(js, value, kj::mv(maybeTransfer)); } + // Reads `data` with Deserializer. + JsValue deserializeBytes(Lock& js, kj::Array data) { + Deserializer deser(js, data); + return deser.readValue(js); + } + JSG_RESOURCE_TYPE(SerTestContext) { JSG_NESTED_TYPE(Foo); JSG_NESTED_TYPE(Bar); @@ -243,6 +249,7 @@ struct SerTestContext: public ContextGlobalObject { JSG_METHOD(foreignToSerializer); JSG_METHOD(serializerToForeign); JSG_METHOD(structuredCloneWithTransfer); + JSG_METHOD(deserializeBytes); } }; JSG_DECLARE_ISOLATE_TYPE(SerTestIsolate, @@ -494,5 +501,28 @@ KJ_TEST("recursive structuredClone with transfer") { "`${buf.byteLength},${outer.nested[0]}`", "string", "0,42"); } + +KJ_TEST("malformed native errors throw") { + Evaluator e(v8System); + + // A header, V8's tag for a host object, and the native error tag. + constexpr auto prefix = "const prefix = [0xff, 0x0f, 0x5c, 0x0a];\n"_kj; + + // Cut short before its type: an internal error. + { + KJ_EXPECT_LOG(ERROR, "deserialization failure, possible corruption"); + e.expectEval(kj::str(prefix, + "try { deserializeBytes(new Uint8Array(prefix)); } catch (e) {\n" + " e.message.startsWith('internal error; reference = ')\n" + "}"), + "boolean", "true"); + } + + // A message that is not a string: a DataCloneError. + e.expectEval(kj::str(prefix, + "try { deserializeBytes(new Uint8Array([...prefix, 0x01, 0x5f])); }\n" + "catch (e) { `${e.name}: ${e.message}` }"), + "string", "DataCloneError: Deserialization failed: error message is not a string"); +} } // namespace } // namespace workerd::jsg::test diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index 71e45b21c72..77e29cdff41 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -14,114 +14,80 @@ namespace workerd::jsg { namespace { -// Keep in sync with the nativeError serialization tag defined in -// worker-interface.capnp -constexpr uint32_t SERIALIZATION_TAG_NATIVE_ERROR = 10; - -// The error tag is serialized as a uint32_t immediately following -// the SERIALIZATION_TAG_NATIVE_ERROR tag in order to more efficiently -// determine the type of error when deserializing so that we can -// construct the appropriate v8::Exception type on deserialization -// without having to expensive string comparison on the error name. -enum class ErrorTag : uint32_t { - // The UNKNOWN tag is used when the error name is not recognized. - // When this occurs, we will serialize the name of the error, and - // when it is deserialized, we will create a generic Error and then - // set the name to the stored name. - UNKNOWN, - ERROR, - TYPE_ERROR, - RANGE_ERROR, - REFERENCE_ERROR, - SYNTAX_ERROR, - WASM_COMPILE_ERROR, - WASM_LINK_ERROR, - WASM_RUNTIME_ERROR, - WASM_SUSPEND_ERROR, - EVAL_ERROR, - URI_ERROR, - AGGREGATE_ERROR, - SUPPRESSED_ERROR, + +// The error tag is serialized as a uint32_t immediately following SERIALIZATION_TAG_NATIVE_ERROR, +// so that the deserializer can construct the appropriate v8::Exception type without comparing +// the error name. It is 0 (ERROR_TAG_UNKNOWN) for an error whose name is not one of +// NATIVE_ERROR_TYPES; that name is then written out, and the error is deserialized as a generic +// Error with the name set on it. Otherwise it is the 1-based index of the name in +// NATIVE_ERROR_TYPES. +constexpr uint32_t ERROR_TAG_UNKNOWN = 0; + +struct NativeErrorType { + kj::StringPtr name; + v8::Local (*create)(v8::Local message, v8::Local options); }; -constexpr ErrorTag getErrorTagFromName(jsg::Lock& js, kj::StringPtr str) { - if (str == "Error"_kj) { - return ErrorTag::ERROR; - } else if (str == "TypeError"_kj) { - return ErrorTag::TYPE_ERROR; - } else if (str == "RangeError"_kj) { - return ErrorTag::RANGE_ERROR; - } else if (str == "ReferenceError"_kj) { - return ErrorTag::REFERENCE_ERROR; - } else if (str == "SyntaxError"_kj) { - return ErrorTag::SYNTAX_ERROR; - } else if (str == "WasmCompileError"_kj) { - return ErrorTag::WASM_COMPILE_ERROR; - } else if (str == "WasmLinkError"_kj) { - return ErrorTag::WASM_LINK_ERROR; - } else if (str == "WasmRuntimeError"_kj) { - return ErrorTag::WASM_RUNTIME_ERROR; - } else if (str == "WasmSuspendError"_kj) { - return ErrorTag::WASM_SUSPEND_ERROR; - } else if (str == "EvalError"_kj) { - return ErrorTag::EVAL_ERROR; - } else if (str == "URIError"_kj) { - return ErrorTag::URI_ERROR; - } else if (str == "AggregateError"_kj) { - return ErrorTag::AGGREGATE_ERROR; - } else if (str == "SuppressedError"_kj) { - return ErrorTag::SUPPRESSED_ERROR; - } - return ErrorTag::UNKNOWN; -} - -JsObject toJsError(Lock& js, ErrorTag tag, JsString message) { - switch (tag) { - case ErrorTag::ERROR: { - return JsObject(v8::Exception::Error(message).As()); - } - case ErrorTag::TYPE_ERROR: { - return JsObject(v8::Exception::TypeError(message).As()); - } - case ErrorTag::RANGE_ERROR: { - return JsObject(v8::Exception::RangeError(message).As()); - } - case ErrorTag::REFERENCE_ERROR: { - return JsObject(v8::Exception::ReferenceError(message).As()); - } - case ErrorTag::SYNTAX_ERROR: { - return JsObject(v8::Exception::SyntaxError(message).As()); - } - case ErrorTag::WASM_COMPILE_ERROR: { - return JsObject(v8::Exception::WasmCompileError(message).As()); - } - case ErrorTag::WASM_LINK_ERROR: { - return JsObject(v8::Exception::WasmLinkError(message).As()); - } - case ErrorTag::WASM_RUNTIME_ERROR: { - return JsObject(v8::Exception::WasmRuntimeError(message).As()); - } - case ErrorTag::WASM_SUSPEND_ERROR: { - return JsObject(v8::Exception::WasmSuspendError(message).As()); - } - case ErrorTag::EVAL_ERROR: { - return JsObject(v8::Exception::EvalError(message).As()); - } - case ErrorTag::URI_ERROR: { - return JsObject(v8::Exception::URIError(message).As()); - } - case ErrorTag::AGGREGATE_ERROR: { - return JsObject(v8::Exception::AggregateError(message).As()); - } - case ErrorTag::SUPPRESSED_ERROR: { - return JsObject(v8::Exception::SuppressedError(message).As()); - } - case ErrorTag::UNKNOWN: { - return JsObject(v8::Exception::Error(message).As()); - } +// The order is part of the format: never reorder, insert or remove entries; only append. +const NativeErrorType NATIVE_ERROR_TYPES[] = { + {"Error"_kj, v8::Exception::Error}, + {"TypeError"_kj, v8::Exception::TypeError}, + {"RangeError"_kj, v8::Exception::RangeError}, + {"ReferenceError"_kj, v8::Exception::ReferenceError}, + {"SyntaxError"_kj, v8::Exception::SyntaxError}, + {"WasmCompileError"_kj, v8::Exception::WasmCompileError}, + {"WasmLinkError"_kj, v8::Exception::WasmLinkError}, + {"WasmRuntimeError"_kj, v8::Exception::WasmRuntimeError}, + {"WasmSuspendError"_kj, v8::Exception::WasmSuspendError}, + {"EvalError"_kj, v8::Exception::EvalError}, + {"URIError"_kj, v8::Exception::URIError}, + {"AggregateError"_kj, v8::Exception::AggregateError}, + {"SuppressedError"_kj, v8::Exception::SuppressedError}, +}; + +// The tag of the error type named `name`, compared as UTF-8 up to the first NUL. +uint32_t errorTagOf(v8::Isolate* isolate, v8::Local name) { + v8::String::Utf8Value utf8(isolate, name); + if (*utf8 == nullptr) return ERROR_TAG_UNKNOWN; + kj::StringPtr str(*utf8); + for (auto i: kj::indices(NATIVE_ERROR_TYPES)) { + if (NATIVE_ERROR_TYPES[i].name == str) return i + 1; } - return JsObject(v8::Exception::Error(message).As()); + return ERROR_TAG_UNKNOWN; +} + +// A new error of the type `tag` names, or a generic Error for a tag that names no type. +v8::Local newNativeError(uint32_t tag, v8::Local message) { + auto create = tag >= 1 && tag <= kj::size(NATIVE_ERROR_TYPES) ? NATIVE_ERROR_TYPES[tag - 1].create + : v8::Exception::Error; + return create(message, {}).As(); } + +v8::Local newInternalizedString(v8::Isolate* isolate, kj::StringPtr str) { + return v8::String::NewFromUtf8( + isolate, str.begin(), v8::NewStringType::kInternalized, static_cast(str.size())) + .ToLocalChecked(); +} + +// Throws what Serializer's ThrowDataCloneError() throws, unless execution is terminating. +void throwDataCloneError(v8::Isolate* isolate, kj::StringPtr message) { + if (isolate->IsExecutionTerminating()) return; + auto messageStr = v8::String::NewFromUtf8( + isolate, message.begin(), v8::NewStringType::kNormal, static_cast(message.size())) + .ToLocalChecked(); + v8::Local exception; + if (newDataCloneError(isolate, messageStr).ToLocal(&exception)) { + isolate->ThrowException(exception); + } +} + +// Throws the internal error that a failed read of the format throws, unless execution is +// terminating. +void throwCorrupt(v8::Isolate* isolate) { + if (isolate->IsExecutionTerminating()) return; + throwInternalError(isolate, "deserialization failure, possible corruption"_kj); +} + } // namespace void Serializer::ExternalHandler::serializeFunction( @@ -245,51 +211,7 @@ v8::Maybe Serializer::WriteHostObject(v8::Isolate* isolate, v8::LocalIsNativeError()) { - auto nameStr = js.str("name"_kj); - auto messageStr = js.str("message"_kj); - - // Get the standard name, message, stack, cause, error, errors properties from - // the error object. - writeRawUint32(SERIALIZATION_TAG_NATIVE_ERROR); - - // A mix of ad-hoc and regular serialization. We first serialize - // the error tag, which is an enum that identifies the type of error - // for faster/easier deserialization. Then we serialize the message which - // usually come from the prototype. Then we grab the own properties, - // serializing the number of them followed by each name and value in - // sequence. - - jsg::JsObject errorObj(object); - auto name = errorObj.get(js, nameStr); - auto nameKjStr = name.toString(js); - auto tag = getErrorTagFromName(js, nameKjStr); - writeRawUint32(static_cast(tag)); - // We only write the name if it is not one of the known error types. - if (tag == ErrorTag::UNKNOWN) { - write(js, name); - } - - write(js, errorObj.get(js, messageStr)); - - auto names = errorObj.getPropertyNames(js, KeyCollectionFilter::OWN_ONLY, - PropertyFilter::ALL_PROPERTIES, IndexFilter::SKIP_INDICES); - - auto obj = js.obj(); - for (size_t n = 0; n < names.size(); n++) { - auto name = names.get(js, n); - // The name typically comes from the prototype and therefore - // do not show up in the own properties of the error object, and the - // message we want to treat specially since we need it early in the - // deserialization. - // Since we already have them serialized above, we can filter them - // out here. - if (name.strictEquals(nameStr) || name.strictEquals(messageStr)) continue; - - obj.set(js, name, errorObj.get(js, name)); - } - write(js, obj); - - return v8::Just(true); + return writeNativeError(isolate, ser, object); } writeApiObject(js, object); @@ -563,64 +485,7 @@ v8::MaybeLocal Deserializer::ReadHostObject(v8::Isolate* isolate) { uint tag = readRawUint32(); if (tag == SERIALIZATION_TAG_NATIVE_ERROR) { - auto& js = Lock::from(isolate); - auto stack = js.str("stack"_kj); - - // The first uint32_t is the error tag, which identifies the type of error. - auto errorTag = static_cast(readRawUint32()); - // If The error tag is UNKNOWN, we will read the name of the error next. - // If the error is known, we don't both serializing the name. - kj::Maybe maybeName; - if (errorTag == ErrorTag::UNKNOWN) { - maybeName = readValue(js); - } - - // The next value is the message, which is always present. - auto messageValue = readValue(js); - auto messageStr = KJ_UNWRAP_OR(messageValue.tryCast(), { - JSG_FAIL_REQUIRE( - DOMDataCloneError, "Deserialization failed: error message is not a string"); - }); - // Now let's create the error object based on the tag and message. - auto obj = toJsError(js, errorTag, messageStr); - - // If we have a name, we set it on the error object. This is not - // perfect but it gets close enough. Specifically, when the error - // was serialized, if the user has modified the name or created - // their own subclass, then we end up having to create just a - // regular error here and change the name. It is not possible - // for us here to clone the exact error class that was used, - // so instanceof checks will not work as expected. But, that's ok. - KJ_IF_SOME(name, maybeName) { - // We use defineProperty here since the name is not typically - // modifiable with set() on error objects. - obj.defineProperty(js, "name"_kj, name); - } - - // Now let's read the remaining properties... They were serialized as - // a plain object with some own properties. - KJ_IF_SOME(serObj, readValue(js).tryCast()) { - auto names = serObj.getPropertyNames(js, KeyCollectionFilter::OWN_ONLY, - PropertyFilter::ALL_PROPERTIES, IndexFilter::SKIP_INDICES); - for (size_t n = 0; n < names.size(); n++) { - auto name = names.get(js, n); - // If the preserveStackInErrors option is false, then we will not - // restore the serialized stack property if it is included in the - // serialized output. - if (!preserveStackInErrors && name.strictEquals(stack)) continue; - auto value = serObj.get(js, name); - // Use createDataProperty instead of ordinary set to avoid - // invoking prototype-chain setters. This code runs inside V8's - // DisallowJavascriptExecution scope; an ordinary Set() that hits - // a tenant-installed Error.prototype setter would trigger - // V8_Fatal -> abort(). CreateDataProperty defines an own data - // property directly, matching the HTML structured-clone spec. - obj.createDataProperty(js, name, value); - } - } - - v8::Local ret = obj; - return ret; + return readNativeError(isolate, deser, PreserveStack(preserveStackInErrors)); } return readApiObject(Lock::from(isolate), tag); @@ -672,6 +537,115 @@ v8::MaybeLocal readApiObject( } } +v8::Maybe writeNativeError( + v8::Isolate* isolate, v8::ValueSerializer& ser, v8::Local error) { + auto context = isolate->GetCurrentContext(); + auto nameKey = newInternalizedString(isolate, "name"_kj); + auto messageKey = newInternalizedString(isolate, "message"_kj); + + ser.WriteUint32(SERIALIZATION_TAG_NATIVE_ERROR); + + v8::Local name; + v8::Local nameString; + if (!error->Get(context, nameKey).ToLocal(&name) || + !name->ToDetailString(context).ToLocal(&nameString)) { + return v8::Nothing(); + } + auto tag = errorTagOf(isolate, nameString); + ser.WriteUint32(tag); + // The name of a known type is implied by the tag. + if (tag == ERROR_TAG_UNKNOWN && ser.WriteValue(context, name).IsNothing()) { + return v8::Nothing(); + } + + // The message comes before the other properties, since the deserializer needs it to create + // the error. + v8::Local message; + if (!error->Get(context, messageKey).ToLocal(&message) || + ser.WriteValue(context, message).IsNothing()) { + return v8::Nothing(); + } + + v8::Local keys; + if (!error + ->GetPropertyNames(context, v8::KeyCollectionMode::kOwnOnly, + v8::PropertyFilter::ALL_PROPERTIES, v8::IndexFilter::kSkipIndices) + .ToLocal(&keys)) { + return v8::Nothing(); + } + auto properties = v8::Object::New(isolate); + for (uint32_t i = 0; i < keys->Length(); i++) { + v8::Local key; + if (!keys->Get(context, i).ToLocal(&key)) return v8::Nothing(); + // The name and message are written above. + if (key->StrictEquals(nameKey) || key->StrictEquals(messageKey)) continue; + v8::Local value; + if (!error->Get(context, key).ToLocal(&value) || + properties->Set(context, key, value).IsNothing()) { + return v8::Nothing(); + } + } + return ser.WriteValue(context, properties); +} + +v8::MaybeLocal readNativeError( + v8::Isolate* isolate, v8::ValueDeserializer& deser, PreserveStack preserveStack) { + auto context = isolate->GetCurrentContext(); + + uint32_t tag; + if (!deser.ReadUint32(&tag)) { + throwCorrupt(isolate); + return {}; + } + v8::Local name; + if (tag == ERROR_TAG_UNKNOWN && !deser.ReadValue(context).ToLocal(&name)) return {}; + + v8::Local message; + if (!deser.ReadValue(context).ToLocal(&message)) return {}; + if (!message->IsString()) { + throwDataCloneError(isolate, "Deserialization failed: error message is not a string"_kj); + return {}; + } + auto error = newNativeError(tag, message.As()); + + // An error of an unknown type is created as a generic Error with the name it was written + // with, so it is not an instance of the class it was created from. The name is defined as an + // own property, since an error's `name` usually comes from its prototype. + if (!name.IsEmpty() && + error->DefineOwnProperty(context, newInternalizedString(isolate, "name"_kj), name) + .IsNothing()) { + return {}; + } + + v8::Local properties; + if (!deser.ReadValue(context).ToLocal(&properties)) return {}; + if (properties->IsObject()) { + auto object = properties.As(); + auto stackKey = newInternalizedString(isolate, "stack"_kj); + v8::Local keys; + if (!object + ->GetPropertyNames(context, v8::KeyCollectionMode::kOwnOnly, + v8::PropertyFilter::ALL_PROPERTIES, v8::IndexFilter::kSkipIndices) + .ToLocal(&keys)) { + return {}; + } + for (uint32_t i = 0; i < keys->Length(); i++) { + v8::Local key; + v8::Local value; + if (!keys->Get(context, i).ToLocal(&key)) return {}; + if (!preserveStack && key->StrictEquals(stackKey)) continue; + // A data property rather than Set(), so that no setter on the error's prototype chain runs: + // deserialization runs with JavaScript execution disallowed, where calling one is fatal, + // and the structured clone spec defines own data properties. + if (!object->Get(context, key).ToLocal(&value) || + error->CreateDataProperty(context, key.As(), value).IsNothing()) { + return {}; + } + } + } + return error; +} + v8::MaybeLocal newDataCloneError(v8::Isolate* isolate, v8::Local message) { auto& js = Lock::from(isolate); try { diff --git a/src/workerd/jsg/ser.h b/src/workerd/jsg/ser.h index b315f3fb302..be2f05d2f3e 100644 --- a/src/workerd/jsg/ser.h +++ b/src/workerd/jsg/ser.h @@ -347,9 +347,30 @@ class Deserializer final: v8::ValueDeserializer::Delegate { }; // For serializers implemented outside C++ JSG, which drive a v8::ValueSerializer or -// v8::ValueDeserializer with a delegate of their own but write workerd API objects in the same -// format as Serializer and Deserializer. Each returns an empty result, leaving the exception -// pending, if what it runs throws. +// v8::ValueDeserializer with a delegate of their own but write native errors and workerd API +// objects in the same format as Serializer and Deserializer. Each returns an empty result, leaving +// the exception pending, if what it runs throws. + +// The tag with which a native error written as a host object begins. Keep in sync with the +// nativeError serialization tag defined in worker-interface.capnp. +constexpr uint32_t SERIALIZATION_TAG_NATIVE_ERROR = 10; + +// Writes the native error `error` as a host object, as Serializer does when +// Lock::isUsingEnhancedErrorSerialization() is true: SERIALIZATION_TAG_NATIVE_ERROR, its type, its +// name if the type is not a known one, its message, and its other own properties as a plain +// object. The values go through `ser`, and so through its delegate. Reading the properties can run +// JavaScript. Serializer writes native errors through this too, so the format has one definition. +v8::Maybe writeNativeError( + v8::Isolate* isolate, v8::ValueSerializer& ser, v8::Local error); + +// Whether readNativeError() restores the `stack` property that was written with an error, or +// leaves the error with the stack captured as it is created. +WD_STRONG_BOOL(PreserveStack); + +// Reads an error that writeNativeError() wrote, once its SERIALIZATION_TAG_NATIVE_ERROR has been +// read, as Deserializer does. Runs no JavaScript beyond what reading values through `deser` runs. +v8::MaybeLocal readNativeError( + v8::Isolate* isolate, v8::ValueDeserializer& deser, PreserveStack preserveStack); // Writes `object` as Serializer's WriteHostObject() writes an object that is not a native error: // through its type's serializer if it is a serializable workerd API object, or else throws a From 1ac71a5bba148f961cf03f41abd4200481a5e235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 09:37:09 +0100 Subject: [PATCH 02/10] Take serializer and deserializer options in Rust JSG jsg::ser::serialize() and deserialize() only did what jsg::Serializer and jsg::Deserializer do with their default options. Callers that share data with C++ code using other options could not match it: RPC, queues, WebSockets and storage write version 15, storage reads legacy data as version 13 without a header, and RPC and streams read errors without their stack. Add serialize_with() and deserialize_with(), taking SerializeOptions and DeserializeOptions, which mirror jsg::Serializer::Options and jsg::Deserializer::Options: - version: the format version to write, or to read in. A version older than 13 is an internal error, as C++ asserts; a version newer than V8's is V8's DataCloneError, left pending. - omit_header / read_header. - preserve_stack_in_errors, passed to readNativeError(). - diagnostic_context, the label in the internal error for a missing or unsupported header. The shims apply them in the order C++ does: a given read version overrides the header's. serialize() and deserialize() are now serialize_with() and deserialize_with() with the defaults. The tests compare the bytes with jsg::Serializer's for each version, with and without a header, and read them with both deserializers. --- src/rust/jsg-test/ffi.c++ | 20 ++- src/rust/jsg-test/ffi.h | 15 +- src/rust/jsg-test/lib.rs | 67 ++++++++- src/rust/jsg-test/tests/serialization.rs | 180 +++++++++++++++++++++++ src/rust/jsg/README.md | 20 ++- src/rust/jsg/ffi.h | 24 +-- src/rust/jsg/ser.c++ | 88 +++++++---- src/rust/jsg/ser.rs | 113 ++++++++++++-- src/rust/jsg/v8.rs | 38 ++++- 9 files changed, 485 insertions(+), 80 deletions(-) diff --git a/src/rust/jsg-test/ffi.c++ b/src/rust/jsg-test/ffi.c++ index bc772005a2a..d1a990c4b7f 100644 --- a/src/rust/jsg-test/ffi.c++ +++ b/src/rust/jsg-test/ffi.c++ @@ -269,9 +269,13 @@ bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value) { return ::workerd::rust::jsg::maybe_local_from_ffi(kj::mv(value)).IsEmpty(); } -::rust::Vec cpp_serialize(Isolate* isolate, ::workerd::rust::jsg::Local value) { +::rust::Vec cpp_serialize(Isolate* isolate, + ::workerd::rust::jsg::Local value, + const ::workerd::rust::jsg::SerializeOptions& options) { auto& js = ::workerd::jsg::Lock::from(isolate); - ::workerd::jsg::Serializer serializer(js); + ::workerd::jsg::Serializer::Options cppOptions{.omitHeader = options.omit_header}; + if (options.has_version) cppOptions.version = options.version; + ::workerd::jsg::Serializer serializer(js, kj::mv(cppOptions)); serializer.write( js, ::workerd::jsg::JsValue(::workerd::rust::jsg::local_from_ffi(kj::mv(value)))); ::rust::Vec result; @@ -281,9 +285,17 @@ bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value) { return result; } -::workerd::rust::jsg::Local cpp_deserialize(Isolate* isolate, ::rust::Slice data) { +::workerd::rust::jsg::Local cpp_deserialize(Isolate* isolate, + ::rust::Slice data, + const ::workerd::rust::jsg::DeserializeOptions& options) { auto& js = ::workerd::jsg::Lock::from(isolate); - ::workerd::jsg::Deserializer deserializer(js, kj::arrayPtr(data.data(), data.size())); + ::workerd::jsg::Deserializer::Options cppOptions{ + .readHeader = options.read_header, + .preserveStackInErrors = options.preserve_stack_in_errors, + }; + if (options.has_version) cppOptions.version = options.version; + ::workerd::jsg::Deserializer deserializer( + js, kj::arrayPtr(data.data(), data.size()), kj::none, kj::none, kj::mv(cppOptions)); v8::Local result = deserializer.readValue(js); return ::workerd::rust::jsg::to_ffi(kj::mv(result)); } diff --git a/src/rust/jsg-test/ffi.h b/src/rust/jsg-test/ffi.h index ebd2731531e..2c82b05b9a8 100644 --- a/src/rust/jsg-test/ffi.h +++ b/src/rust/jsg-test/ffi.h @@ -106,12 +106,17 @@ ::workerd::rust::jsg::MaybeLocal empty_maybe_local(); // Returns whether `value` is empty as a v8::MaybeLocal, to test what Rust passes for `None`. bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value); -// Serializes `value` with C++ JSG's jsg::Serializer, for comparison with Rust JSG's. Throws a KJ -// exception if serialization throws. -::rust::Vec cpp_serialize(Isolate* isolate, ::workerd::rust::jsg::Local value); -// Deserializes `data` with C++ JSG's jsg::Deserializer. Throws a KJ exception if +// Serializes `value` with C++ JSG's jsg::Serializer, for comparison with Rust JSG's, with the +// options of `options` other than errors_are_host_objects, which comes from the isolate. Throws a +// KJ exception if serialization throws. +::rust::Vec cpp_serialize(Isolate* isolate, + ::workerd::rust::jsg::Local value, + const ::workerd::rust::jsg::SerializeOptions& options); +// Deserializes `data` with C++ JSG's jsg::Deserializer and `options`. Throws a KJ exception if // deserialization throws. -::workerd::rust::jsg::Local cpp_deserialize(Isolate* isolate, ::rust::Slice data); +::workerd::rust::jsg::Local cpp_deserialize(Isolate* isolate, + ::rust::Slice data, + const ::workerd::rust::jsg::DeserializeOptions& options); // Returns whether `a` and `b` belong to the same v8::IsolateGroup. bool isolates_share_group(Isolate* a, Isolate* b); diff --git a/src/rust/jsg-test/lib.rs b/src/rust/jsg-test/lib.rs index 98dcb2472f1..6bd8aa9e532 100644 --- a/src/rust/jsg-test/lib.rs +++ b/src/rust/jsg-test/lib.rs @@ -20,6 +20,8 @@ mod ffi { type Isolate = jsg::v8::ffi::Isolate; type Local = jsg::v8::ffi::Local; type MaybeLocal = jsg::v8::ffi::MaybeLocal; + type SerializeOptions = jsg::v8::ffi::SerializeOptions; + type DeserializeOptions = jsg::v8::ffi::DeserializeOptions; type ModuleRegistry = jsg::v8::ffi::ModuleRegistry; } @@ -134,16 +136,25 @@ mod ffi { #[allow(dead_code)] pub fn cpp_sees_empty_maybe_local(value: MaybeLocal) -> bool; - /// Serializes `value` with C++ JSG's `jsg::Serializer`. Fails if serialization throws. + /// Serializes `value` with C++ JSG's `jsg::Serializer` and `options`. Fails if + /// serialization throws. #[expect(clippy::allow_attributes)] #[allow(dead_code)] - pub unsafe fn cpp_serialize(isolate: *mut Isolate, value: Local) -> Result>; + pub unsafe fn cpp_serialize( + isolate: *mut Isolate, + value: Local, + options: &SerializeOptions, + ) -> Result>; - /// Deserializes `data` with C++ JSG's `jsg::Deserializer`. Fails if deserialization - /// throws. + /// Deserializes `data` with C++ JSG's `jsg::Deserializer` and `options`. Fails if + /// deserialization throws. #[expect(clippy::allow_attributes)] #[allow(dead_code)] - pub unsafe fn cpp_deserialize(isolate: *mut Isolate, data: &[u8]) -> Result; + pub unsafe fn cpp_deserialize( + isolate: *mut Isolate, + data: &[u8], + options: &DeserializeOptions, + ) -> Result; /// Returns whether `a` and `b` belong to the same `v8::IsolateGroup`. pub unsafe fn isolates_share_group(a: *mut Isolate, b: *mut Isolate) -> bool; @@ -533,9 +544,29 @@ impl Harness { /// /// Panics if serialization throws. pub fn cpp_serialize<'s>(lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>) -> Vec { + Self::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()) + } + + /// Serializes `value` with C++ JSG's `jsg::Serializer` and the same options, to compare with + /// [`jsg::ser::serialize_with`]. + /// + /// # Panics + /// + /// Panics if serialization throws. + pub fn cpp_serialize_with<'s>( + lock: &mut jsg::Lock<'s>, + value: v8::Local<'s, v8::Value>, + options: &jsg::ser::SerializeOptions, + ) -> Vec { + let options = jsg::v8::ffi::SerializeOptions { + has_version: options.version.is_some(), + version: options.version.unwrap_or_default(), + omit_header: options.omit_header, + errors_are_host_objects: false, + }; // SAFETY: `lock` proves the isolate is locked, a context is entered and a HandleScope is // open, which `value` belongs to. - unsafe { ffi::cpp_serialize(lock.isolate().as_ffi(), value.into_ffi()) } + unsafe { ffi::cpp_serialize(lock.isolate().as_ffi(), value.into_ffi(), &options) } .expect("C++ JSG should serialize the value") } @@ -546,10 +577,30 @@ impl Harness { /// /// Panics if deserialization throws. pub fn cpp_deserialize<'s>(lock: &mut jsg::Lock<'s>, data: &[u8]) -> v8::Local<'s, v8::Value> { - // SAFETY: as for `cpp_serialize`; the result belongs to the lock's scope. + Self::cpp_deserialize_with(lock, data, &jsg::ser::DeserializeOptions::default()) + } + + /// Deserializes `data` with C++ JSG's `jsg::Deserializer` and the same options, to compare + /// with [`jsg::ser::deserialize_with`]. + /// + /// # Panics + /// + /// Panics if deserialization throws. + pub fn cpp_deserialize_with<'s>( + lock: &mut jsg::Lock<'s>, + data: &[u8], + options: &jsg::ser::DeserializeOptions<'_>, + ) -> v8::Local<'s, v8::Value> { + let options = jsg::v8::ffi::DeserializeOptions { + has_version: options.version.is_some(), + version: options.version.unwrap_or_default(), + read_header: options.read_header, + preserve_stack_in_errors: options.preserve_stack_in_errors, + }; + // SAFETY: as for `cpp_serialize_with`; the result belongs to the lock's scope. unsafe { v8::Local::from_ffi( - ffi::cpp_deserialize(lock.isolate().as_ffi(), data) + ffi::cpp_deserialize(lock.isolate().as_ffi(), data, &options) .expect("C++ JSG should deserialize the data"), ) } diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index d75ead4a419..a7318381113 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -386,6 +386,186 @@ fn shared_array_buffers_are_written_as_cpp_jsg_writes_them() { }); } +/// Serializes the value of `expr` with `options` and checks that the bytes are those C++ JSG writes +/// with the same options, and that what Rust JSG and C++ JSG read from them with `read` satisfies +/// `check`, a JavaScript expression of `v`. +fn assert_round_trip_with( + lock: &mut jsg::Lock<'_>, + ctx: &EvalContext<'_>, + expr: &str, + options: &jsg::ser::SerializeOptions, + read: &jsg::ser::DeserializeOptions<'_>, + check: &str, +) { + let value = eval(lock, ctx, expr); + let data = jsg::ser::serialize_with(lock, value, options).unwrap(); + assert_eq!( + data, + Harness::cpp_serialize_with(lock, value, options), + "{expr}: {options:?}" + ); + + let copy = jsg::ser::deserialize_with(lock, &data, read).unwrap(); + ctx.set_global(lock, "v", copy); + assert!( + ctx.eval::(lock, check).unwrap(), + "{expr}: {options:?}: Rust JSG read: {check}" + ); + + let copy = Harness::cpp_deserialize_with(lock, &data, read); + ctx.set_global(lock, "v", copy); + assert!( + ctx.eval::(lock, check).unwrap(), + "{expr}: {options:?}: C++ JSG read: {check}" + ); +} + +#[test] +fn versions_and_headers_are_written_and_read_as_cpp_jsg_does() { + Harness::new().run_in_context(|lock, ctx| { + let expr = "({ a: [1, 'x'], m: new Map([[1, 2]]), d: new Date(7) })"; + let check = "v.a[1] === 'x' && v.m.get(1) === 2 && v.d.getTime() === 7"; + for version in [None, Some(13), Some(14), Some(15)] { + let options = jsg::ser::SerializeOptions { + version, + ..Default::default() + }; + assert_round_trip_with( + lock, + ctx, + expr, + &options, + &jsg::ser::DeserializeOptions::default(), + check, + ); + + // Without a header, the reader is told the version, as storage reads data it wrote + // before it wrote headers. + let options = jsg::ser::SerializeOptions { + version: Some(version.unwrap_or(15)), + omit_header: true, + }; + let read = jsg::ser::DeserializeOptions { + version: options.version, + read_header: false, + ..Default::default() + }; + assert_round_trip_with(lock, ctx, expr, &options, &read, check); + } + + // The header says the data is version 13, and the version given overrides it, as in C++. + let options = jsg::ser::SerializeOptions { + version: Some(13), + ..Default::default() + }; + let read = jsg::ser::DeserializeOptions { + version: Some(13), + ..Default::default() + }; + assert_round_trip_with(lock, ctx, expr, &options, &read, check); + Ok(()) + }); +} + +#[test] +fn versions_out_of_range_are_rejected() { + Harness::new().run_in_context(|lock, ctx| { + let value = lock.undefined(); + for version in [0, 12] { + let options = jsg::ser::SerializeOptions { + version: Some(version), + ..Default::default() + }; + let err = jsg::ser::serialize_with(lock, value, &options).unwrap_err(); + assert!(!err.is_pending(), "{version}"); + assert!( + err.message + .contains("The minimum serialization version is 13."), + "{version}: {}", + err.message + ); + + let read = jsg::ser::DeserializeOptions { + version: Some(version), + read_header: false, + ..Default::default() + }; + let err = jsg::ser::deserialize_with(lock, b"_", &read).unwrap_err(); + assert!(!err.is_pending(), "{version}"); + assert!( + err.message + .contains("The minimum serialization version is 13."), + "{version}: {}", + err.message + ); + } + + // V8 throws for a version newer than its own. + let caught = lock + .try_catch(|lock| { + let options = jsg::ser::SerializeOptions { + version: Some(1000), + ..Default::default() + }; + let result = jsg::ser::serialize_with(lock, value, &options); + assert!(result.as_ref().is_err_and(jsg::Error::is_pending)); + result + }) + .unwrap(); + ctx.set_global(lock, "e", caught.unwrap_err()); + assert!( + ctx.eval::(lock, "e.name === 'DataCloneError'") + .unwrap() + ); + Ok(()) + }); +} + +#[test] +fn errors_can_be_read_without_their_stack() { + let harness = Harness::with_flags(|mut flags| flags.set_enhanced_error_serialization(true)); + harness.run_in_context(|lock, ctx| { + let read = jsg::ser::DeserializeOptions { + preserve_stack_in_errors: false, + ..Default::default() + }; + assert_round_trip_with( + lock, + ctx, + "Object.assign(new TypeError('t'), { stack: 'original', code: 1 })", + &jsg::ser::SerializeOptions::default(), + &read, + "v instanceof TypeError && v.code === 1 && typeof v.stack === 'string' && \ + v.stack !== 'original'", + ); + Ok(()) + }); +} + +#[test] +fn header_errors_name_the_diagnostic_context() { + Harness::new().run_in_context(|lock, ctx| { + let read = jsg::ser::DeserializeOptions { + diagnostic_context: Some("RPC result"), + ..Default::default() + }; + let err = jsg::ser::deserialize_with(lock, b"\x49\x02", &read).unwrap_err(); + assert!(!err.is_pending()); + assert!(err.message.contains("RPC result"), "{}", err.message); + + // Without a header to read, the same bytes are a value. + let read = jsg::ser::DeserializeOptions { + version: Some(15), + read_header: false, + ..Default::default() + }; + let value = jsg::ser::deserialize_with(lock, b"\x49\x02", &read).unwrap(); + ctx.set_global(lock, "v", value); + assert!(ctx.eval::(lock, "v === 1").unwrap()); + Ok(()) + }); +} + static CHECKED_WHILE_TERMINATING: AtomicBool = AtomicBool::new(false); #[jsg_resource] diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index 69433023027..94f185c81b1 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -514,7 +514,7 @@ propagating. As with the functions C++ JSG wraps, the function's `name` is empty ## Errors from JavaScript -When a function that `Local::call` calls throws, the exception stays pending in V8, as with C++ JSG's `JsExceptionThrown`, and `call` returns an error for which `Error::is_pending()` is true. The same goes for these other operations that run JavaScript: converting a value to a string or a number (its `toString`, `valueOf` or `Symbol.toPrimitive`, or a Proxy trap), `Object::get` and `Object::has` (a getter or Proxy trap), `Object::set` (a setter, also one on the prototype chain, or a Proxy trap) and `Array::set` (a setter, also one on the prototype chain), `Array::iterate` and so converting to a `Vec` (an element's getter), and `ser::serialize` (a getter or Proxy trap, or a `DataCloneError`). Building the array when converting a `Vec` to JavaScript runs no JavaScript: like C++ JSG, it defines the elements as data properties (`Array::with_elements`), so no setter on `Array.prototype` runs. A `#[jsg_struct]` follows C++ JSG's `fast_jsg_struct` flag: with it, the fields are defined as data properties (`Object::with_data_properties`), an absent optional field as `undefined`, and no JavaScript runs; without it, they are set one by one with `Object::set`, an absent optional field is omitted, so a setter on `Object.prototype` runs, and what it throws is passed on. `Error`'s own `FromJS`, which reads `name` and `message`, passes such an error on too. `Lock::throw_exception` throws nothing for that error, so a callback that returns it propagates the original exception to its JavaScript caller. V8 keeps reporting the exception where it was thrown, and DevTools' "pause on uncaught exceptions" still sees whether JavaScript further up catches it. A termination is handled the same way. +When a function that `Local::call` calls throws, the exception stays pending in V8, as with C++ JSG's `JsExceptionThrown`, and `call` returns an error for which `Error::is_pending()` is true. The same goes for these other operations that run JavaScript: converting a value to a string or a number (its `toString`, `valueOf` or `Symbol.toPrimitive`, or a Proxy trap), `Object::get` and `Object::has` (a getter or Proxy trap), `Object::set` (a setter, also one on the prototype chain, or a Proxy trap) and `Array::set` (a setter, also one on the prototype chain), `Array::iterate` and so converting to a `Vec` (an element's getter), and `ser::serialize` and `ser::serialize_with` (a getter or Proxy trap, or a `DataCloneError`). Building the array when converting a `Vec` to JavaScript runs no JavaScript: like C++ JSG, it defines the elements as data properties (`Array::with_elements`), so no setter on `Array.prototype` runs. A `#[jsg_struct]` follows C++ JSG's `fast_jsg_struct` flag: with it, the fields are defined as data properties (`Object::with_data_properties`), an absent optional field as `undefined`, and no JavaScript runs; without it, they are set one by one with `Object::set`, an absent optional field is omitted, so a setter on `Object.prototype` runs, and what it throws is passed on. `Error`'s own `FromJS`, which reads `name` and `message`, passes such an error on too. `Lock::throw_exception` throws nothing for that error, so a callback that returns it propagates the original exception to its JavaScript caller. V8 keeps reporting the exception where it was thrown, and DevTools' "pause on uncaught exceptions" still sees whether JavaScript further up catches it. A termination is handled the same way. Converting a value to JavaScript can fail too, as C++ JSG's `wrap()` can throw, so `ToJS::to_js` returns a `Result`: creating a `Date` or a resource's wrapper fails, leaving the exception pending, when V8 throws or execution is terminating, and a container passes on the error from converting an element. These conversions fail only with pending errors. A `#[jsg_method]`, property getter or wildcard property throws an error from converting its return value as it throws one the method returns, with `Lock::throw_exception`: for a pending error the original exception, or the termination, keeps propagating, and any other error is thrown as a new JavaScript error of its type. The method has already run by then; only its return value is lost. @@ -533,13 +533,17 @@ match lock.try_catch(|lock| callback.call::(lock, None, &[]))? { `jsg::ser::serialize` and `jsg::ser::deserialize` convert a value to and from bytes in the format of C++ JSG's `jsg::Serializer` and `jsg::Deserializer`, with their default options: the bytes are -the same, and each reads what the other writes, so the two can share stored data. Native errors -are written as host objects when the `enhanced_error_serialization` flag is on, and workerd API -objects such as `Blob` and `Headers` through their C++ serializers; Rust resources can't be -serialized. A `SharedArrayBuffer` is written as a reference to a buffer that is not part of the -bytes, which neither `ser::deserialize` nor a `jsg::Deserializer` given only the bytes can -resolve, so data containing one can't be read back. Deserialization runs no JavaScript. Transfer -lists, externals and the other options are not supported. The C++ side is `src/rust/jsg/ser.c++`. +the same, and each reads what the other writes, so the two can share stored data. +`ser::serialize_with` and `ser::deserialize_with` take the options of `jsg::Serializer::Options` +and `jsg::Deserializer::Options` as `ser::SerializeOptions` and `ser::DeserializeOptions`: the +format version, whether there is a header, whether errors get back their `stack`, and the label +for header errors. Native errors are written as host objects when the +`enhanced_error_serialization` flag is on, and workerd API objects such as `Blob` and `Headers` +through their C++ serializers; Rust resources can't be serialized. A `SharedArrayBuffer` is +written as a reference to a buffer that is not part of the bytes, which neither +`ser::deserialize` nor a `jsg::Deserializer` given only the bytes can resolve, so data containing +one can't be read back. Deserialization runs no JavaScript. Transfer lists and externals are not +supported. The C++ side is `src/rust/jsg/ser.c++`. ## Type Conversions diff --git a/src/rust/jsg/ffi.h b/src/rust/jsg/ffi.h index bc7b8c96c6e..42abd4aaa16 100644 --- a/src/rust/jsg/ffi.h +++ b/src/rust/jsg/ffi.h @@ -32,6 +32,8 @@ struct TracedReference; struct Realm; struct IsolateState; struct GcVisitor; +struct SerializeOptions; +struct DeserializeOptions; enum class BackingStoreInitializationMode : ::std::uint8_t; enum class ExceptionType : ::std::uint8_t; enum class PromiseState : ::std::uint8_t; @@ -470,19 +472,23 @@ void realm_detach(Isolate* isolate, const IsolateState* state); // In the format of C++ JSG's jsg::Serializer and jsg::Deserializer, which read what these write // and the other way round; see ser.c++. // -// Writes `value`, after a header, as jsg::Serializer does: native errors as host objects if -// `errors_are_host_objects` (jsg::Lock::isUsingEnhancedErrorSerialization()), and workerd API -// objects through their serializers. Stores the bytes in `result` and returns true, or returns +// Writes `value` as jsg::Serializer does with `options`: native errors as host objects if +// `options.errors_are_host_objects` (jsg::Lock::isUsingEnhancedErrorSerialization()), and workerd +// API objects through their serializers. Stores the bytes in `result` and returns true, or returns // false if serialization threw, leaving the exception pending: a DataCloneError, or what a getter, -// Proxy trap or serializer threw. +// Proxy trap or serializer threw. Throws a KJ exception for a version older than 13. bool local_serialize(Isolate* isolate, const Local& value, - bool errors_are_host_objects, + const SerializeOptions& options, ::rust::Vec& result); -// Reads a value as jsg::Deserializer does with its default options, and returns it, or -// none if deserialization threw, leaving the exception pending. Runs no JavaScript. Throws a KJ -// exception if the header is missing or of an unsupported version. -MaybeLocal value_deserialize(Isolate* isolate, ::rust::Slice data); +// Reads a value as jsg::Deserializer does with `options`, and returns it, or none if +// deserialization threw, leaving the exception pending. Runs no JavaScript. Throws a KJ exception, +// labeled with `diagnostic_context`, if the header is to be read and is missing or of an +// unsupported version, and for a version older than 13. +MaybeLocal value_deserialize(Isolate* isolate, + ::rust::Slice data, + const DeserializeOptions& options, + ::rust::Str diagnostic_context); // Errors // Builds an exception whose message is raw bytes interpreted as UTF-8. Invalid diff --git a/src/rust/jsg/ser.c++ b/src/rust/jsg/ser.c++ index 9ebb2b95999..90905623962 100644 --- a/src/rust/jsg/ser.c++ +++ b/src/rust/jsg/ser.c++ @@ -37,12 +37,24 @@ void throwDataCloneError(v8::Isolate* isolate, v8::Local message) { class Serializer final: public v8::ValueSerializer::Delegate { public: - Serializer(v8::Isolate* isolate, bool errorsAreHostObjects) + Serializer(v8::Isolate* isolate, const SerializeOptions& options) : isolate(isolate), - errorsAreHostObjects(errorsAreHostObjects), + options(options), sharedArrayBuffers(isolate), - ser(isolate, this) { - ser.WriteHeader(); + ser(isolate, this) {} + + // Applies the options as jsg::Serializer's constructor does. Returns false if V8 rejected the + // version, leaving a DataCloneError pending. Throws a KJ exception for a version older than 13. + bool init() { + if (options.has_version) { + KJ_REQUIRE( + options.version >= 13, "The minimum serialization version is 13.", options.version); + if (ser.SetWriteVersion(options.version).IsNothing()) return false; + } + if (!options.omit_header) { + ser.WriteHeader(); + } + return true; } // Returns false if serialization threw, leaving the exception pending. @@ -67,7 +79,7 @@ class Serializer final: public v8::ValueSerializer::Delegate { bool HasCustomHostObject(v8::Isolate*) override { // V8 already treats objects with internal fields, such as API objects, as host objects. - return errorsAreHostObjects; + return options.errors_are_host_objects; } v8::Maybe IsHostObject(v8::Isolate*, v8::Local object) override { @@ -75,7 +87,7 @@ class Serializer final: public v8::ValueSerializer::Delegate { } v8::Maybe WriteHostObject(v8::Isolate*, v8::Local object) override { - if (errorsAreHostObjects && object->IsNativeError()) { + if (options.errors_are_host_objects && object->IsNativeError()) { return ::workerd::jsg::writeNativeError(isolate, ser, object); } return ::workerd::jsg::writeApiObject(isolate, ser, object); @@ -94,30 +106,31 @@ class Serializer final: public v8::ValueSerializer::Delegate { private: v8::Isolate* isolate; - bool errorsAreHostObjects; + SerializeOptions options; v8::LocalVector sharedArrayBuffers; v8::ValueSerializer ser; }; class Deserializer final: public v8::ValueDeserializer::Delegate { public: - Deserializer(v8::Isolate* isolate, ::rust::Slice data) + Deserializer( + v8::Isolate* isolate, ::rust::Slice data, const DeserializeOptions& options) : isolate(isolate), + options(options), deser(isolate, data.data(), data.size(), this) {} - // Reads the header as jsg::Deserializer does. If it is missing or of an unsupported version, - // throws a KJ exception, which reaches JavaScript as an internal error; if execution is - // terminating, returns false. - bool readHeader(::rust::Slice data) { - v8::TryCatch tryCatch(isolate); - if (deser.ReadHeader(isolate->GetCurrentContext()).IsJust()) return true; - MaybeLocal exception; - if (!catch_exception(tryCatch, exception)) return false; - // Only header metadata, never bytes of the value. - auto header = data.empty() ? "empty" : data[0] == 0xff ? "present" : "missing"; - KJ_FAIL_REQUIRE("Unable to deserialize cloned data due to invalid or unsupported version.", - data.size(), header, deser.GetWireFormatVersion(), - v8::CurrentValueSerializerFormatVersion()); + // Applies the options as jsg::Deserializer's constructor does, reading the header unless + // `options.read_header` is false. If the header is missing or of an unsupported version, or the + // version is older than 13, throws a KJ exception, which reaches JavaScript as an internal error; + // if execution is terminating, returns false. + bool init(::rust::Slice data, ::rust::Str diagnosticContext) { + if (options.read_header && !readHeader(data, diagnosticContext)) return false; + if (options.has_version) { + KJ_REQUIRE( + options.version >= 13, "The minimum serialization version is 13.", options.version); + deser.SetWireFormatVersion(options.version); + } + return true; } // Returns none if deserialization threw, leaving the exception pending. @@ -129,7 +142,8 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { uint32_t tag; if (!deser.ReadUint32(&tag)) return throwCorrupt(); if (tag == ::workerd::jsg::SERIALIZATION_TAG_NATIVE_ERROR) { - return ::workerd::jsg::readNativeError(isolate, deser, ::workerd::jsg::PreserveStack::YES); + return ::workerd::jsg::readNativeError( + isolate, deser, ::workerd::jsg::PreserveStack(options.preserve_stack_in_errors)); } return ::workerd::jsg::readApiObject(isolate, deser, tag); } @@ -138,12 +152,26 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { // to by number. private: + bool readHeader(::rust::Slice data, ::rust::Str diagnosticContext) { + v8::TryCatch tryCatch(isolate); + if (deser.ReadHeader(isolate->GetCurrentContext()).IsJust()) return true; + MaybeLocal exception; + if (!catch_exception(tryCatch, exception)) return false; + // Only header metadata, never bytes of the value. + auto context = kj::str(diagnosticContext); + auto header = data.empty() ? "empty" : data[0] == 0xff ? "present" : "missing"; + KJ_FAIL_REQUIRE("Unable to deserialize cloned data due to invalid or unsupported version.", + context, data.size(), header, deser.GetWireFormatVersion(), + v8::CurrentValueSerializerFormatVersion()); + } + v8::MaybeLocal throwCorrupt() { isolate_throw_internal_error(isolate, "deserialization failure, possible corruption"); return {}; } v8::Isolate* isolate; + DeserializeOptions options; v8::ValueDeserializer deser; }; @@ -152,17 +180,21 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { // Serialization bool local_serialize(Isolate* isolate, const Local& value, - bool errors_are_host_objects, + const SerializeOptions& options, ::rust::Vec& result) { - Serializer serializer(isolate, errors_are_host_objects); - if (!serializer.write(local_as_ref_from_ffi(value))) return false; + Serializer serializer(isolate, options); + if (!serializer.init() || !serializer.write(local_as_ref_from_ffi(value))) + return false; result = serializer.release(); return true; } -MaybeLocal value_deserialize(Isolate* isolate, ::rust::Slice data) { - Deserializer deserializer(isolate, data); - if (!deserializer.readHeader(data)) return {}; +MaybeLocal value_deserialize(Isolate* isolate, + ::rust::Slice data, + const DeserializeOptions& options, + ::rust::Str diagnostic_context) { + Deserializer deserializer(isolate, data, options); + if (!deserializer.init(data, diagnostic_context)) return {}; return maybe_local_to_ffi(deserializer.read()); } diff --git a/src/rust/jsg/ser.rs b/src/rust/jsg/ser.rs index 2a7ab4f23db..86aa69bf806 100644 --- a/src/rust/jsg/ser.rs +++ b/src/rust/jsg/ser.rs @@ -7,8 +7,8 @@ //! Structured serialization of JavaScript values, in the format of C++ JSG's `jsg::Serializer` //! and `jsg::Deserializer`. //! -//! Each reads what the other writes, and [`serialize`] writes the same bytes as -//! `jsg::Serializer` with its default options. That covers what V8's serializer supports, plus: +//! Each reads what the other writes, and [`serialize_with`] writes the same bytes as +//! `jsg::Serializer` with the same options. That covers what V8's serializer supports, plus: //! //! - Native errors, as host objects with their own properties (including `stack`), when the //! `enhanced_error_serialization` compatibility flag is on. @@ -16,26 +16,85 @@ //! `DOMException`, through their own serializers. Rust resources are not serializable. //! //! A `SharedArrayBuffer` is written as a reference that [`deserialize`] cannot resolve. Transfer -//! lists, externals and the other `jsg::Serializer` options are not supported. +//! lists and externals are not supported. use crate::Error; use crate::Lock; use crate::v8; +/// How [`serialize_with`] writes a value, as `jsg::Serializer::Options`. The default is +/// `jsg::Serializer`'s. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct SerializeOptions { + /// The format version to write, at least 13, or `None` for V8's current one. Data written in + /// an older version can be read by an older V8. + pub version: Option, + /// Whether to leave out the header, which holds the version. Data without one is read with + /// [`DeserializeOptions::read_header`] false and the version it was written in. + pub omit_header: bool, +} + +/// How [`deserialize_with`] reads data, as `jsg::Deserializer::Options`. The default is +/// `jsg::Deserializer`'s. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct DeserializeOptions<'a> { + /// The format version to read in, at least 13, or `None` for the one the header holds. Set + /// it for data written without a header. + pub version: Option, + /// Whether the data begins with a header. + pub read_header: bool, + /// Whether a native error gets back the `stack` it was written with, or keeps the one + /// captured as it is created. Untrusted data should not choose the stack. + pub preserve_stack_in_errors: bool, + /// What the data is, such as `"RPC result"`, to tell apart the internal errors reported for + /// a missing or unsupported header. A fixed label, never user data: it is logged. + pub diagnostic_context: Option<&'a str>, +} + +impl Default for DeserializeOptions<'_> { + fn default() -> Self { + Self { + version: None, + read_header: true, + preserve_stack_in_errors: true, + diagnostic_context: None, + } + } +} + /// Serializes `value`, with a header, as `jsg::Serializer` does. /// /// # Errors /// -/// Serialization runs JavaScript (getters and Proxy traps). If it throws, or `value` can't be -/// serialized (a `DataCloneError`), or execution is terminating, the exception stays pending -/// and the error is one for which [`Error::is_pending`] is true. Return it, as for -/// [`v8::Local::::call`]. +/// As for [`serialize_with`]. pub fn serialize<'s>( lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, +) -> Result, Error> { + serialize_with(lock, value, &SerializeOptions::default()) +} + +/// Serializes `value` as `jsg::Serializer` does with `options`. +/// +/// # Errors +/// +/// Serialization runs JavaScript (getters and Proxy traps). If it throws, or `value` can't be +/// serialized (a `DataCloneError`), or `options.version` is newer than V8's (a +/// `DataCloneError`), or execution is terminating, the exception stays pending and the error is +/// one for which [`Error::is_pending`] is true. Return it, as for +/// [`v8::Local::::call`]. A version older than 13 is an internal error. +pub fn serialize_with<'s>( + lock: &mut Lock<'s>, + value: v8::Local<'s, v8::Value>, + options: &SerializeOptions, ) -> Result, Error> { value.debug_assert_isolate(lock); - let errors_are_host_objects = lock.feature_flags().get_enhanced_error_serialization(); + let ffi_options = v8::ffi::SerializeOptions { + has_version: options.version.is_some(), + version: options.version.unwrap_or_default(), + omit_header: options.omit_header, + errors_are_host_objects: lock.feature_flags().get_enhanced_error_serialization(), + }; v8::call_into_js(lock, |lock| { let mut data = Vec::new(); // SAFETY: the lock guarantees the isolate is locked and a HandleScope is active, and @@ -44,7 +103,7 @@ pub fn serialize<'s>( v8::ffi::local_serialize( lock.isolate().as_ffi(), value.as_ffi(), - errors_are_host_objects, + &ffi_options, &mut data, ) }?; @@ -57,18 +116,46 @@ pub fn serialize<'s>( /// /// # Errors /// +/// As for [`deserialize_with`]. +pub fn deserialize<'s>( + lock: &mut Lock<'s>, + data: &[u8], +) -> Result, Error> { + deserialize_with(lock, data, &DeserializeOptions::default()) +} + +/// Deserializes a value that [`serialize_with`] or `jsg::Serializer` wrote, as +/// `jsg::Deserializer` does with `options`. Runs no JavaScript. +/// +/// # Errors +/// /// If `data` is malformed or names an unknown API type, or a type's deserializer throws, or /// execution is terminating, the exception stays pending and the error is one for which -/// [`Error::is_pending`] is true. If the header is missing or of an unsupported version, the -/// error is an internal one. -pub fn deserialize<'s>( +/// [`Error::is_pending`] is true. If the header is read and is missing or of an unsupported +/// version, or `options.version` is older than 13, the error is an internal one. +pub fn deserialize_with<'s>( lock: &mut Lock<'s>, data: &[u8], + options: &DeserializeOptions<'_>, ) -> Result, Error> { + let ffi_options = v8::ffi::DeserializeOptions { + has_version: options.version.is_some(), + version: options.version.unwrap_or_default(), + read_header: options.read_header, + preserve_stack_in_errors: options.preserve_stack_in_errors, + }; + let diagnostic_context = options.diagnostic_context.unwrap_or("unknown"); v8::call_into_js(lock, |lock| { // SAFETY: the lock guarantees the isolate is locked and a HandleScope is active. The // result belongs to the lock's scope. - let value = unsafe { v8::ffi::value_deserialize(lock.isolate().as_ffi(), data) }?; + let value = unsafe { + v8::ffi::value_deserialize( + lock.isolate().as_ffi(), + data, + &ffi_options, + diagnostic_context, + ) + }?; // SAFETY: as above. Ok(unsafe { v8::Local::from_ffi_maybe(value) }) }) diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 86f57f72a20..feb5544da60 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -106,6 +106,29 @@ pub mod ffi { ptr: usize, } + /// The options of `ser::serialize_with` that the C++ shim applies, as + /// `jsg::Serializer::Options`. + #[derive(Debug, Clone, Copy)] + struct SerializeOptions { + /// Whether to write `version` rather than V8's current version. + has_version: bool, + version: u32, + omit_header: bool, + /// `jsg::Lock::isUsingEnhancedErrorSerialization()`. + errors_are_host_objects: bool, + } + + /// The options of `ser::deserialize_with` that the C++ shim applies, as + /// `jsg::Deserializer::Options`. + #[derive(Debug, Clone, Copy)] + struct DeserializeOptions { + /// Whether to read in `version` rather than the header's version. + has_version: bool, + version: u32, + read_header: bool, + preserve_stack_in_errors: bool, + } + /// Mirrors `v8::BackingStoreInitializationMode`. Must be kept in sync with /// the V8-defined enum in `v8-array-buffer.h`. #[derive(Debug, PartialEq, Eq, Copy, Clone)] @@ -620,16 +643,21 @@ pub mod ffi { // Serialization // Leaves what serialization throws pending; see `call_into_js()`. Fallible: the result - // is a rust::Vec. + // is a rust::Vec, and a version older than 13 throws. pub unsafe fn local_serialize( isolate: *mut Isolate, value: &Local, - errors_are_host_objects: bool, + options: &SerializeOptions, result: &mut Vec, ) -> Result; - // Leaves what deserialization throws pending. Fallible: a missing or unsupported header - // throws. - pub unsafe fn value_deserialize(isolate: *mut Isolate, data: &[u8]) -> Result; + // Leaves what deserialization throws pending. Fallible: a missing or unsupported header, + // or a version older than 13, throws. + pub unsafe fn value_deserialize( + isolate: *mut Isolate, + data: &[u8], + options: &DeserializeOptions, + diagnostic_context: &str, + ) -> Result; // Errors pub unsafe fn exception_create_from_bytes( From 2856fc7a43d1bbe728f0c17240d4c209d41ca4e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 09:39:46 +0100 Subject: [PATCH 03/10] Let Rust JSG serialization reject class instances jsg::Serializer::Options::treatClassInstancesAsPlainObjects = false makes jsg::Serializer throw a DataCloneError for a class instance rather than write its own properties as a plain object, which would be read back without its prototype or methods. RPC and Frankenvalue use it. Add SerializeOptions::class_instances: AsPlainObjects (the default and the structured clone algorithm's behavior) or Reject. With Reject, the shim decides what is a host object as jsg::Serializer::IsHostObject() does: objects with internal fields, native errors when they are written as host objects, and objects whose prototype is not Object.prototype. writeApiObject() then throws the DataCloneError jsg::Serializer throws for such an object, since the shim has no ExternalHandler to offer it to. Plain objects, arrays, built-in types (including instances of their subclasses, which V8 writes as the built-in type), errors and API objects are written as before. The tests compare the bytes with jsg::Serializer's with and without enhanced_error_serialization. --- src/rust/jsg-test/ffi.c++ | 5 +- src/rust/jsg-test/lib.rs | 2 + src/rust/jsg-test/tests/serialization.rs | 90 ++++++++++++++++++++++++ src/rust/jsg/README.md | 5 +- src/rust/jsg/ser.c++ | 15 +++- src/rust/jsg/ser.rs | 19 ++++- src/rust/jsg/v8.rs | 2 + 7 files changed, 132 insertions(+), 6 deletions(-) diff --git a/src/rust/jsg-test/ffi.c++ b/src/rust/jsg-test/ffi.c++ index d1a990c4b7f..2264d227ab5 100644 --- a/src/rust/jsg-test/ffi.c++ +++ b/src/rust/jsg-test/ffi.c++ @@ -273,7 +273,10 @@ bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value) { ::workerd::rust::jsg::Local value, const ::workerd::rust::jsg::SerializeOptions& options) { auto& js = ::workerd::jsg::Lock::from(isolate); - ::workerd::jsg::Serializer::Options cppOptions{.omitHeader = options.omit_header}; + ::workerd::jsg::Serializer::Options cppOptions{ + .omitHeader = options.omit_header, + .treatClassInstancesAsPlainObjects = options.class_instances_as_plain_objects, + }; if (options.has_version) cppOptions.version = options.version; ::workerd::jsg::Serializer serializer(js, kj::mv(cppOptions)); serializer.write( diff --git a/src/rust/jsg-test/lib.rs b/src/rust/jsg-test/lib.rs index 6bd8aa9e532..ca2217f9aa7 100644 --- a/src/rust/jsg-test/lib.rs +++ b/src/rust/jsg-test/lib.rs @@ -562,6 +562,8 @@ impl Harness { has_version: options.version.is_some(), version: options.version.unwrap_or_default(), omit_header: options.omit_header, + class_instances_as_plain_objects: options.class_instances + == jsg::ser::ClassInstances::AsPlainObjects, errors_are_host_objects: false, }; // SAFETY: `lock` proves the isolate is locked, a context is entered and a HandleScope is diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index a7318381113..9532d2f4415 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -444,6 +444,7 @@ fn versions_and_headers_are_written_and_read_as_cpp_jsg_does() { let options = jsg::ser::SerializeOptions { version: Some(version.unwrap_or(15)), omit_header: true, + ..Default::default() }; let read = jsg::ser::DeserializeOptions { version: options.version, @@ -566,6 +567,95 @@ fn header_errors_name_the_diagnostic_context() { }); } +#[test] +fn class_instances_are_plain_objects_or_rejected() { + let reject = jsg::ser::SerializeOptions { + class_instances: jsg::ser::ClassInstances::Reject, + ..Default::default() + }; + let read = jsg::ser::DeserializeOptions::default(); + for enhanced_errors in [false, true] { + let harness = Harness::with_flags(|mut flags| { + flags.set_enhanced_error_serialization(enhanced_errors); + }); + harness.run_in_context(|lock, ctx| { + ctx.eval_raw( + lock, + "globalThis.Point = class Point { constructor() { this.x = 1; } }", + ) + .unwrap(); + + // By default a class instance is written as a plain object. + assert_round_trip_with( + lock, + ctx, + "new Point()", + &jsg::ser::SerializeOptions::default(), + &read, + "Object.getPrototypeOf(v) === Object.prototype && v.x === 1", + ); + + // Rejecting class instances leaves plain objects, built-in types, errors and API + // objects alone. + assert_round_trip_with( + lock, + ctx, + "({ a: [1], m: new Map([[1, 2]]), d: new Date(3), e: new RangeError('r') })", + &reject, + &read, + "v.a[0] === 1 && v.m.get(1) === 2 && v.d.getTime() === 3 && \ + v.e instanceof RangeError", + ); + let exception = serialize_exception(lock, ctx, "() => 1"); + ctx.set_global(lock, "exception", exception); + assert_round_trip_with( + lock, + ctx, + "[exception]", + &reject, + &read, + "v[0].name === 'DataCloneError'", + ); + + // V8 writes an instance of a subclass of a built-in type as the built-in type. + assert_round_trip_with( + lock, + ctx, + "new (class extends Map {})([[1, 2]])", + &reject, + &read, + "Object.getPrototypeOf(v) === Map.prototype && v.get(1) === 2", + ); + + // writeApiObject() rejects these with the DataCloneError jsg::Serializer throws. + for (expr, type_name) in [ + ("({ p: new Point() })", "Point"), + ("[Object.create(null)]", "Object"), + ] { + let value = eval(lock, ctx, expr); + let caught = lock + .try_catch(|lock| { + let result = jsg::ser::serialize_with(lock, value, &reject); + assert!(result.as_ref().is_err_and(jsg::Error::is_pending), "{expr}"); + result + }) + .unwrap(); + assert_error( + lock, + ctx, + caught.unwrap_err(), + "DataCloneError", + &format!( + "Could not serialize object of type \"{type_name}\". This type does not \ + support serialization." + ), + ); + } + Ok(()) + }); + } +} + static CHECKED_WHILE_TERMINATING: AtomicBool = AtomicBool::new(false); #[jsg_resource] diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index 94f185c81b1..71eb8ad330b 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -536,8 +536,9 @@ of C++ JSG's `jsg::Serializer` and `jsg::Deserializer`, with their default optio the same, and each reads what the other writes, so the two can share stored data. `ser::serialize_with` and `ser::deserialize_with` take the options of `jsg::Serializer::Options` and `jsg::Deserializer::Options` as `ser::SerializeOptions` and `ser::DeserializeOptions`: the -format version, whether there is a header, whether errors get back their `stack`, and the label -for header errors. Native errors are written as host objects when the +format version, whether there is a header, whether class instances are written as plain objects +or rejected (as RPC rejects them), whether errors get back their `stack`, and the label for +header errors. Native errors are written as host objects when the `enhanced_error_serialization` flag is on, and workerd API objects such as `Blob` and `Headers` through their C++ serializers; Rust resources can't be serialized. A `SharedArrayBuffer` is written as a reference to a buffer that is not part of the bytes, which neither diff --git a/src/rust/jsg/ser.c++ b/src/rust/jsg/ser.c++ index 90905623962..14662702848 100644 --- a/src/rust/jsg/ser.c++ +++ b/src/rust/jsg/ser.c++ @@ -54,6 +54,9 @@ class Serializer final: public v8::ValueSerializer::Delegate { if (!options.omit_header) { ser.WriteHeader(); } + if (!options.class_instances_as_plain_objects) { + prototypeOfObject = v8::Object::New(isolate)->GetPrototype(); + } return true; } @@ -79,11 +82,17 @@ class Serializer final: public v8::ValueSerializer::Delegate { bool HasCustomHostObject(v8::Isolate*) override { // V8 already treats objects with internal fields, such as API objects, as host objects. - return options.errors_are_host_objects; + return !options.class_instances_as_plain_objects || options.errors_are_host_objects; } + // Decides as jsg::Serializer::IsHostObject() does. v8::Maybe IsHostObject(v8::Isolate*, v8::Local object) override { - return v8::Just(object->InternalFieldCount() > 0 || object->IsNativeError()); + if (object->InternalFieldCount() > 0) return v8::Just(true); + if (object->IsNativeError()) return v8::Just(options.errors_are_host_objects); + if (options.class_instances_as_plain_objects) return v8::Just(false); + // A class instance, whose prototype is not Object.prototype, is a host object, which + // writeApiObject() rejects with a DataCloneError. + return v8::Just(object->GetPrototype() != prototypeOfObject); } v8::Maybe WriteHostObject(v8::Isolate*, v8::Local object) override { @@ -107,6 +116,8 @@ class Serializer final: public v8::ValueSerializer::Delegate { private: v8::Isolate* isolate; SerializeOptions options; + // Object.prototype, if `options.class_instances_as_plain_objects` is false. + v8::Local prototypeOfObject; v8::LocalVector sharedArrayBuffers; v8::ValueSerializer ser; }; diff --git a/src/rust/jsg/ser.rs b/src/rust/jsg/ser.rs index 86aa69bf806..1092a12b5a7 100644 --- a/src/rust/jsg/ser.rs +++ b/src/rust/jsg/ser.rs @@ -22,6 +22,19 @@ use crate::Error; use crate::Lock; use crate::v8; +/// What [`serialize_with`] does with a class instance: an object whose prototype is not +/// `Object.prototype` and that is not a workerd API object or a native error. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum ClassInstances { + /// Writes its own properties, as for a plain object, and drops its prototype, as the + /// structured clone algorithm does. + #[default] + AsPlainObjects, + /// Throws a `DataCloneError`, as RPC does, so that a class instance is not read back as a + /// plain object without its methods. + Reject, +} + /// How [`serialize_with`] writes a value, as `jsg::Serializer::Options`. The default is /// `jsg::Serializer`'s. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -32,6 +45,9 @@ pub struct SerializeOptions { /// Whether to leave out the header, which holds the version. Data without one is read with /// [`DeserializeOptions::read_header`] false and the version it was written in. pub omit_header: bool, + /// What to do with a class instance, as + /// `jsg::Serializer::Options::treatClassInstancesAsPlainObjects`. + pub class_instances: ClassInstances, } /// How [`deserialize_with`] reads data, as `jsg::Deserializer::Options`. The default is @@ -79,7 +95,7 @@ pub fn serialize<'s>( /// # Errors /// /// Serialization runs JavaScript (getters and Proxy traps). If it throws, or `value` can't be -/// serialized (a `DataCloneError`), or `options.version` is newer than V8's (a +/// serialized (a `DataCloneError`, as for a class instance with [`ClassInstances::Reject`]), or `options.version` is newer than V8's (a /// `DataCloneError`), or execution is terminating, the exception stays pending and the error is /// one for which [`Error::is_pending`] is true. Return it, as for /// [`v8::Local::::call`]. A version older than 13 is an internal error. @@ -93,6 +109,7 @@ pub fn serialize_with<'s>( has_version: options.version.is_some(), version: options.version.unwrap_or_default(), omit_header: options.omit_header, + class_instances_as_plain_objects: options.class_instances == ClassInstances::AsPlainObjects, errors_are_host_objects: lock.feature_flags().get_enhanced_error_serialization(), }; v8::call_into_js(lock, |lock| { diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index feb5544da60..5a38cbc3b75 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -114,6 +114,8 @@ pub mod ffi { has_version: bool, version: u32, omit_header: bool, + /// `jsg::Serializer::Options::treatClassInstancesAsPlainObjects`. + class_instances_as_plain_objects: bool, /// `jsg::Lock::isUsingEnhancedErrorSerialization()`. errors_are_host_objects: bool, } From dd369fa4d197388193273b1fe4ec48ea89a69adb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 12:17:26 +0100 Subject: [PATCH 04/10] Pass SharedArrayBuffers and transfer lists through Rust JSG serialization jsg::Serializer keeps the backing store of each SharedArrayBuffer it writes, and takes a transfer list whose ArrayBuffers it detaches, handing their backing stores over in Released. jsg::Deserializer takes both sets of backing stores back. Rust JSG's serializer numbered SharedArrayBuffers but dropped their backing stores, so data holding one could not be read back, and it had no transfer list. serialize_with() now returns Serialized, the counterpart of Released: the bytes and the two lists of backing stores, as jsg::v8::BackingStore. SerializeOptions::transfer lists the values to transfer, and DeserializeOptions takes transferred_array_buffers and shared_array_buffers; Serialized::deserialize_options() fills them in, as Deserializer(js, released) does. serialize() still returns only the bytes. The shims transfer as jsg::Serializer::transfer() does, so the bytes are the same: - A value that is not an ArrayBuffer or a view of one throws a TypeError, and a view of a SharedArrayBuffer a DataCloneError, before anything is detached. - Values are numbered as given, a value given twice once. They are compared by value, not by buffer, as C++ compares them. - Buffers are detached after the value is written, skipping any that JavaScript run by serialization already detached, so a getter sees the buffer intact and V8 still finds the buffer of a view written after it. Reading, a backing store of the wrong kind (a shared one to transfer, or one that is not shared for a SharedArrayBuffer) is an internal error rather than a V8 check failure, and data that names a SharedArrayBuffer beyond the given list is an internal error rather than jsg::Deserializer's assertion thrown through V8. Without the list, V8 throws for a SharedArrayBuffer, as before. BackingStore gains hidden as_ffi()/from_ffi() for bridges in other crates, which the jsg-test helpers use to hand backing stores to and from C++ JSG, and a Debug impl. Externals remain unsupported; the module docs and README say what throws without them. The tests compare the bytes and the number of transferred buffers with jsg::Serializer's, check what is detached after each, and read each serializer's output with the other's deserializer and backing stores. --- src/rust/jsg-test/ffi.c++ | 50 ++++- src/rust/jsg-test/ffi.h | 20 +- src/rust/jsg-test/lib.rs | 75 +++++-- src/rust/jsg-test/tests/serialization.rs | 265 ++++++++++++++++++++++- src/rust/jsg/README.md | 20 +- src/rust/jsg/ffi.h | 30 ++- src/rust/jsg/ser.c++ | 169 +++++++++++++-- src/rust/jsg/ser.rs | 113 ++++++++-- src/rust/jsg/v8.rs | 51 ++++- 9 files changed, 693 insertions(+), 100 deletions(-) diff --git a/src/rust/jsg-test/ffi.c++ b/src/rust/jsg-test/ffi.c++ index 2264d227ab5..55664969812 100644 --- a/src/rust/jsg-test/ffi.c++ +++ b/src/rust/jsg-test/ffi.c++ @@ -269,9 +269,30 @@ bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value) { return ::workerd::rust::jsg::maybe_local_from_ffi(kj::mv(value)).IsEmpty(); } -::rust::Vec cpp_serialize(Isolate* isolate, +namespace { + +void releaseBackingStores( + kj::Array>& stores, ::rust::Vec& addresses) { + for (auto& store: stores) { + addresses.push_back( + reinterpret_cast(new std::shared_ptr(kj::mv(store)))); + } +} + +kj::Array> copyBackingStores( + ::rust::Slice addresses) { + return KJ_MAP(address, addresses) { + return *reinterpret_cast*>(address); + }; +} + +} // namespace + +void cpp_serialize(Isolate* isolate, ::workerd::rust::jsg::Local value, - const ::workerd::rust::jsg::SerializeOptions& options) { + const ::workerd::rust::jsg::SerializeOptions& options, + ::rust::Slice transfer, + ::workerd::rust::jsg::Serialized& result) { auto& js = ::workerd::jsg::Lock::from(isolate); ::workerd::jsg::Serializer::Options cppOptions{ .omitHeader = options.omit_header, @@ -279,26 +300,37 @@ bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value) { }; if (options.has_version) cppOptions.version = options.version; ::workerd::jsg::Serializer serializer(js, kj::mv(cppOptions)); + for (auto& item: transfer) { + serializer.transfer( + js, ::workerd::jsg::JsValue(::workerd::rust::jsg::local_as_ref_from_ffi(item))); + } serializer.write( js, ::workerd::jsg::JsValue(::workerd::rust::jsg::local_from_ffi(kj::mv(value)))); - ::rust::Vec result; - for (auto byte: serializer.release().data) { - result.push_back(byte); + auto released = serializer.release(); + for (auto byte: released.data) { + result.data.push_back(byte); } - return result; + releaseBackingStores(released.sharedArrayBuffers, result.shared_array_buffers); + releaseBackingStores(released.transferredArrayBuffers, result.transferred_array_buffers); } ::workerd::rust::jsg::Local cpp_deserialize(Isolate* isolate, ::rust::Slice data, - const ::workerd::rust::jsg::DeserializeOptions& options) { + const ::workerd::rust::jsg::DeserializeOptions& options, + ::rust::Slice transferred_array_buffers, + ::rust::Slice shared_array_buffers) { auto& js = ::workerd::jsg::Lock::from(isolate); ::workerd::jsg::Deserializer::Options cppOptions{ .readHeader = options.read_header, .preserveStackInErrors = options.preserve_stack_in_errors, }; if (options.has_version) cppOptions.version = options.version; - ::workerd::jsg::Deserializer deserializer( - js, kj::arrayPtr(data.data(), data.size()), kj::none, kj::none, kj::mv(cppOptions)); + auto transferred = copyBackingStores(transferred_array_buffers); + auto shared = copyBackingStores(shared_array_buffers); + kj::Maybe>> maybeShared; + if (options.has_shared_array_buffers) maybeShared = shared.asPtr(); + ::workerd::jsg::Deserializer deserializer(js, kj::arrayPtr(data.data(), data.size()), + transferred.asPtr(), kj::mv(maybeShared), kj::mv(cppOptions)); v8::Local result = deserializer.readValue(js); return ::workerd::rust::jsg::to_ffi(kj::mv(result)); } diff --git a/src/rust/jsg-test/ffi.h b/src/rust/jsg-test/ffi.h index 2c82b05b9a8..51f947a2107 100644 --- a/src/rust/jsg-test/ffi.h +++ b/src/rust/jsg-test/ffi.h @@ -107,16 +107,22 @@ ::workerd::rust::jsg::MaybeLocal empty_maybe_local(); bool cpp_sees_empty_maybe_local(::workerd::rust::jsg::MaybeLocal value); // Serializes `value` with C++ JSG's jsg::Serializer, for comparison with Rust JSG's, with the -// options of `options` other than errors_are_host_objects, which comes from the isolate. Throws a -// KJ exception if serialization throws. -::rust::Vec cpp_serialize(Isolate* isolate, +// options of `options` other than errors_are_host_objects, which comes from the isolate, after +// transferring each value of `transfer`. Stores what jsg::Serializer::release() returns in `result`, +// each backing store as a heap-allocated std::shared_ptr that Rust owns. Throws a KJ exception if +// serialization throws. +void cpp_serialize(Isolate* isolate, ::workerd::rust::jsg::Local value, - const ::workerd::rust::jsg::SerializeOptions& options); -// Deserializes `data` with C++ JSG's jsg::Deserializer and `options`. Throws a KJ exception if -// deserialization throws. + const ::workerd::rust::jsg::SerializeOptions& options, + ::rust::Slice transfer, + ::workerd::rust::jsg::Serialized& result); +// Deserializes `data` with C++ JSG's jsg::Deserializer, `options` and the given backing stores +// (borrowed std::shared_ptr addresses). Throws a KJ exception if deserialization throws. ::workerd::rust::jsg::Local cpp_deserialize(Isolate* isolate, ::rust::Slice data, - const ::workerd::rust::jsg::DeserializeOptions& options); + const ::workerd::rust::jsg::DeserializeOptions& options, + ::rust::Slice transferred_array_buffers, + ::rust::Slice shared_array_buffers); // Returns whether `a` and `b` belong to the same v8::IsolateGroup. bool isolates_share_group(Isolate* a, Isolate* b); diff --git a/src/rust/jsg-test/lib.rs b/src/rust/jsg-test/lib.rs index ca2217f9aa7..2f4b9023d21 100644 --- a/src/rust/jsg-test/lib.rs +++ b/src/rust/jsg-test/lib.rs @@ -22,6 +22,7 @@ mod ffi { type MaybeLocal = jsg::v8::ffi::MaybeLocal; type SerializeOptions = jsg::v8::ffi::SerializeOptions; type DeserializeOptions = jsg::v8::ffi::DeserializeOptions; + type Serialized = jsg::v8::ffi::Serialized; type ModuleRegistry = jsg::v8::ffi::ModuleRegistry; } @@ -136,24 +137,28 @@ mod ffi { #[allow(dead_code)] pub fn cpp_sees_empty_maybe_local(value: MaybeLocal) -> bool; - /// Serializes `value` with C++ JSG's `jsg::Serializer` and `options`. Fails if - /// serialization throws. + /// Serializes `value` with C++ JSG's `jsg::Serializer` and `options`, after transferring + /// `transfer`. Fails if serialization throws. #[expect(clippy::allow_attributes)] #[allow(dead_code)] pub unsafe fn cpp_serialize( isolate: *mut Isolate, value: Local, options: &SerializeOptions, - ) -> Result>; + transfer: &[Local], + result: &mut Serialized, + ) -> Result<()>; - /// Deserializes `data` with C++ JSG's `jsg::Deserializer` and `options`. Fails if - /// deserialization throws. + /// Deserializes `data` with C++ JSG's `jsg::Deserializer`, `options` and the given + /// backing stores. Fails if deserialization throws. #[expect(clippy::allow_attributes)] #[allow(dead_code)] pub unsafe fn cpp_deserialize( isolate: *mut Isolate, data: &[u8], options: &DeserializeOptions, + transferred_array_buffers: &[usize], + shared_array_buffers: &[usize], ) -> Result; /// Returns whether `a` and `b` belong to the same `v8::IsolateGroup`. @@ -544,7 +549,7 @@ impl Harness { /// /// Panics if serialization throws. pub fn cpp_serialize<'s>(lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>) -> Vec { - Self::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()) + Self::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).data } /// Serializes `value` with C++ JSG's `jsg::Serializer` and the same options, to compare with @@ -556,8 +561,14 @@ impl Harness { pub fn cpp_serialize_with<'s>( lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>, - options: &jsg::ser::SerializeOptions, - ) -> Vec { + options: &jsg::ser::SerializeOptions<'_, 's>, + ) -> jsg::ser::Serialized { + let transfer: Vec<_> = options + .transfer + .iter() + // SAFETY: the handles are used only in the call below, while their scope is open. + .map(|item| unsafe { item.into_ffi() }) + .collect(); let options = jsg::v8::ffi::SerializeOptions { has_version: options.version.is_some(), version: options.version.unwrap_or_default(), @@ -566,10 +577,31 @@ impl Harness { == jsg::ser::ClassInstances::AsPlainObjects, errors_are_host_objects: false, }; + let mut result = jsg::v8::ffi::Serialized::default(); // SAFETY: `lock` proves the isolate is locked, a context is entered and a HandleScope is - // open, which `value` belongs to. - unsafe { ffi::cpp_serialize(lock.isolate().as_ffi(), value.into_ffi(), &options) } - .expect("C++ JSG should serialize the value") + // open, which `value` and the values to transfer belong to. + unsafe { + ffi::cpp_serialize( + lock.isolate().as_ffi(), + value.into_ffi(), + &options, + &transfer, + &mut result, + ) + } + .expect("C++ JSG should serialize the value"); + let take = |addresses: Vec| { + addresses + .into_iter() + // SAFETY: C++ handed over each heap-allocated std::shared_ptr. + .map(|address| unsafe { v8::BackingStore::from_ffi(address) }) + .collect() + }; + jsg::ser::Serialized { + data: result.data, + shared_array_buffers: take(result.shared_array_buffers), + transferred_array_buffers: take(result.transferred_array_buffers), + } } /// Deserializes `data` with C++ JSG's `jsg::Deserializer`, to compare with @@ -593,17 +625,32 @@ impl Harness { data: &[u8], options: &jsg::ser::DeserializeOptions<'_>, ) -> v8::Local<'s, v8::Value> { - let options = jsg::v8::ffi::DeserializeOptions { + let ffi_options = jsg::v8::ffi::DeserializeOptions { has_version: options.version.is_some(), version: options.version.unwrap_or_default(), read_header: options.read_header, preserve_stack_in_errors: options.preserve_stack_in_errors, + has_shared_array_buffers: options.shared_array_buffers.is_some(), + }; + let addresses = |stores: &[v8::BackingStore]| { + stores + .iter() + .map(v8::BackingStore::as_ffi) + .collect::>() }; + let transferred = addresses(options.transferred_array_buffers); + let shared = addresses(options.shared_array_buffers.unwrap_or_default()); // SAFETY: as for `cpp_serialize_with`; the result belongs to the lock's scope. unsafe { v8::Local::from_ffi( - ffi::cpp_deserialize(lock.isolate().as_ffi(), data, &options) - .expect("C++ JSG should deserialize the data"), + ffi::cpp_deserialize( + lock.isolate().as_ffi(), + data, + &ffi_options, + &transferred, + &shared, + ) + .expect("C++ JSG should deserialize the data"), ) } } diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index 9532d2f4415..3742f21b2fb 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -371,17 +371,41 @@ fn missing_or_unsupported_header_is_an_internal_error() { } #[test] -fn shared_array_buffers_are_written_as_cpp_jsg_writes_them() { +fn shared_array_buffers_are_shared_with_the_deserializer() { Harness::new().run_in_context(|lock, ctx| { let value = eval( lock, ctx, - "(() => { const b = new SharedArrayBuffer(1); return [b, new SharedArrayBuffer(2), b]; })()", + "globalThis.b = new SharedArrayBuffer(1); [b, new SharedArrayBuffer(2), b]", ); - let data = jsg::ser::serialize(lock, value).unwrap(); - assert_eq!(data, Harness::cpp_serialize(lock, value)); - // No deserializer is given the buffers. - deserialize_exception(lock, &data); + let serialized = + jsg::ser::serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).unwrap(); + let cpp = Harness::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()); + assert_eq!(serialized.data, cpp.data); + assert_eq!(serialized.shared_array_buffers.len(), 2); + assert!( + serialized + .shared_array_buffers + .iter() + .all(v8::BackingStore::is_shared) + ); + + // Each reader reads the other's data with its backing stores, and shares the buffers. + let check = "v[0] === v[2] && v[0] !== b && v[1].byteLength === 2 && \ + (new Uint8Array(b)[0] = 5, new Uint8Array(v[0])[0] === 5)"; + let copy = jsg::ser::deserialize_with(lock, &cpp.data, &cpp.deserialize_options()).unwrap(); + ctx.set_global(lock, "v", copy); + assert!(ctx.eval::(lock, check).unwrap(), "Rust JSG read"); + let copy = Harness::cpp_deserialize_with( + lock, + &serialized.data, + &serialized.deserialize_options(), + ); + ctx.set_global(lock, "v", copy); + assert!(ctx.eval::(lock, check).unwrap(), "C++ JSG read"); + + // Without the backing stores, the data can't be read. + deserialize_exception(lock, &serialized.data); Ok(()) }); } @@ -389,19 +413,19 @@ fn shared_array_buffers_are_written_as_cpp_jsg_writes_them() { /// Serializes the value of `expr` with `options` and checks that the bytes are those C++ JSG writes /// with the same options, and that what Rust JSG and C++ JSG read from them with `read` satisfies /// `check`, a JavaScript expression of `v`. -fn assert_round_trip_with( - lock: &mut jsg::Lock<'_>, +fn assert_round_trip_with<'s>( + lock: &mut jsg::Lock<'s>, ctx: &EvalContext<'_>, expr: &str, - options: &jsg::ser::SerializeOptions, + options: &jsg::ser::SerializeOptions<'_, 's>, read: &jsg::ser::DeserializeOptions<'_>, check: &str, ) { let value = eval(lock, ctx, expr); - let data = jsg::ser::serialize_with(lock, value, options).unwrap(); + let data = jsg::ser::serialize_with(lock, value, options).unwrap().data; assert_eq!( data, - Harness::cpp_serialize_with(lock, value, options), + Harness::cpp_serialize_with(lock, value, options).data, "{expr}: {options:?}" ); @@ -656,6 +680,225 @@ fn class_instances_are_plain_objects_or_rejected() { } } +/// Runs `setup`, which sets the globals `value` and `transfer`, an array of `count` values, and +/// returns them. +fn transfer_setup<'s>( + lock: &mut jsg::Lock<'s>, + ctx: &EvalContext<'_>, + setup: &str, + count: usize, +) -> (v8::Local<'s, v8::Value>, Vec>) { + ctx.eval_raw(lock, setup).unwrap(); + let value = eval(lock, ctx, "value"); + let transfer = (0..count) + .map(|i| eval(lock, ctx, &format!("transfer[{i}]"))) + .collect(); + (value, transfer) +} + +/// Serializes with each serializer what `setup` sets up, transferring `count` buffers, and checks +/// that the bytes are the same, that `detached` holds after each, and that each reader reads with +/// the other's buffers a value for which `check` holds. +fn assert_transfer( + lock: &mut jsg::Lock<'_>, + ctx: &EvalContext<'_>, + setup: &str, + count: usize, + detached: &str, + check: &str, +) { + let (value, transfer) = transfer_setup(lock, ctx, setup, count); + let options = jsg::ser::SerializeOptions { + transfer: &transfer, + ..Default::default() + }; + let serialized = jsg::ser::serialize_with(lock, value, &options).unwrap(); + assert!( + ctx.eval::(lock, detached).unwrap(), + "{setup}: Rust JSG detached" + ); + + let (value, transfer) = transfer_setup(lock, ctx, setup, count); + let options = jsg::ser::SerializeOptions { + transfer: &transfer, + ..Default::default() + }; + let cpp = Harness::cpp_serialize_with(lock, value, &options); + assert!( + ctx.eval::(lock, detached).unwrap(), + "{setup}: C++ JSG detached" + ); + assert_eq!(serialized.data, cpp.data, "{setup}"); + assert_eq!( + serialized.transferred_array_buffers.len(), + cpp.transferred_array_buffers.len(), + "{setup}" + ); + + let copy = jsg::ser::deserialize_with(lock, &cpp.data, &cpp.deserialize_options()).unwrap(); + ctx.set_global(lock, "v", copy); + assert!( + ctx.eval::(lock, check).unwrap(), + "{setup}: Rust JSG read: {check}" + ); + let copy = + Harness::cpp_deserialize_with(lock, &serialized.data, &serialized.deserialize_options()); + ctx.set_global(lock, "v", copy); + assert!( + ctx.eval::(lock, check).unwrap(), + "{setup}: C++ JSG read: {check}" + ); +} + +#[test] +fn transferred_array_buffers_move_to_the_deserializer() { + Harness::new().run_in_context(|lock, ctx| { + // A buffer and a view of it written after it. + assert_transfer( + lock, + ctx, + "globalThis.b = new ArrayBuffer(4); new Uint8Array(b)[1] = 9; + globalThis.value = { b, v: new Uint8Array(b, 1) }; globalThis.transfer = [b];", + 1, + "b.byteLength === 0", + "v.b.byteLength === 4 && v.v.buffer === v.b && v.v[0] === 9", + ); + // A view given for its buffer, and a value given twice, which is transferred once. + assert_transfer( + lock, + ctx, + "globalThis.b = new ArrayBuffer(2); globalThis.u = new Uint8Array(b); u[0] = 3; + globalThis.value = [u]; globalThis.transfer = [u, u];", + 2, + "b.byteLength === 0 && u.length === 0", + "v[0][0] === 3 && v[0].buffer.byteLength === 2", + ); + // A getter that serialization runs sees the buffer intact: it is detached afterwards. + assert_transfer( + lock, + ctx, + "globalThis.b = new ArrayBuffer(8); + globalThis.value = { get size() { return b.byteLength; }, b }; + globalThis.transfer = [b];", + 1, + "b.byteLength === 0", + "v.size === 8 && v.b.byteLength === 8", + ); + // A buffer that the value doesn't hold is still detached, and passed on. + assert_transfer( + lock, + ctx, + "globalThis.b = new ArrayBuffer(1); globalThis.value = 1; globalThis.transfer = [b];", + 1, + "b.byteLength === 0", + "v === 1", + ); + Ok(()) + }); +} + +#[test] +fn values_that_cannot_be_transferred_throw() { + Harness::new().run_in_context(|lock, ctx| { + for (setup, name, message) in [ + ( + "globalThis.b = new ArrayBuffer(1); globalThis.transfer = [b, {}];", + "TypeError", + "Object is not transferable", + ), + ( + "globalThis.b = new ArrayBuffer(1); + globalThis.transfer = [b, new Uint8Array(new SharedArrayBuffer(1))];", + "DataCloneError", + "Object is not transferable.", + ), + ( + "globalThis.b = new ArrayBuffer(1); + globalThis.transfer = [b, new SharedArrayBuffer(1)];", + "TypeError", + "Object is not transferable", + ), + ] { + let (value, transfer) = + transfer_setup(lock, ctx, &format!("{setup} globalThis.value = b;"), 2); + let caught = lock + .try_catch(|lock| { + let options = jsg::ser::SerializeOptions { + transfer: &transfer, + ..Default::default() + }; + let result = jsg::ser::serialize_with(lock, value, &options); + assert!( + result.as_ref().is_err_and(jsg::Error::is_pending), + "{setup}" + ); + result.map(drop) + }) + .unwrap(); + assert_error(lock, ctx, caught.unwrap_err(), name, message); + // Nothing is detached. + assert!( + ctx.eval::(lock, "b.byteLength === 1").unwrap(), + "{setup}" + ); + } + Ok(()) + }); +} + +#[test] +fn buffers_must_match_the_data() { + Harness::new().run_in_context(|lock, ctx| { + let (value, transfer) = transfer_setup( + lock, + ctx, + "globalThis.value = new ArrayBuffer(1); globalThis.transfer = [value];", + 1, + ); + let options = jsg::ser::SerializeOptions { + transfer: &transfer, + ..Default::default() + }; + let transferred = jsg::ser::serialize_with(lock, value, &options).unwrap(); + let value = eval(lock, ctx, "new SharedArrayBuffer(1)"); + let shared = + jsg::ser::serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).unwrap(); + + // Data that refers to buffers the reader isn't given. + deserialize_exception(lock, &transferred.data); + let read = jsg::ser::DeserializeOptions { + shared_array_buffers: Some(&[]), + ..Default::default() + }; + let caught = lock + .try_catch(|lock| jsg::ser::deserialize_with(lock, &shared.data, &read)) + .unwrap(); + let err = jsg::Error::from_js(lock, caught.unwrap_err()).unwrap(); + assert!(err.message.starts_with("internal error"), "{}", err.message); + + // Backing stores of the wrong kind. + for read in [ + jsg::ser::DeserializeOptions { + transferred_array_buffers: &shared.shared_array_buffers, + ..Default::default() + }, + jsg::ser::DeserializeOptions { + shared_array_buffers: Some(&transferred.transferred_array_buffers), + ..Default::default() + }, + ] { + let err = jsg::ser::deserialize_with(lock, &shared.data, &read).unwrap_err(); + assert!(!err.is_pending()); + assert!( + err.message.contains("backing store must"), + "{}", + err.message + ); + } + Ok(()) + }); +} + static CHECKED_WHILE_TERMINATING: AtomicBool = AtomicBool::new(false); #[jsg_resource] diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index 71eb8ad330b..b94d9a86c2f 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -514,7 +514,7 @@ propagating. As with the functions C++ JSG wraps, the function's `name` is empty ## Errors from JavaScript -When a function that `Local::call` calls throws, the exception stays pending in V8, as with C++ JSG's `JsExceptionThrown`, and `call` returns an error for which `Error::is_pending()` is true. The same goes for these other operations that run JavaScript: converting a value to a string or a number (its `toString`, `valueOf` or `Symbol.toPrimitive`, or a Proxy trap), `Object::get` and `Object::has` (a getter or Proxy trap), `Object::set` (a setter, also one on the prototype chain, or a Proxy trap) and `Array::set` (a setter, also one on the prototype chain), `Array::iterate` and so converting to a `Vec` (an element's getter), and `ser::serialize` and `ser::serialize_with` (a getter or Proxy trap, or a `DataCloneError`). Building the array when converting a `Vec` to JavaScript runs no JavaScript: like C++ JSG, it defines the elements as data properties (`Array::with_elements`), so no setter on `Array.prototype` runs. A `#[jsg_struct]` follows C++ JSG's `fast_jsg_struct` flag: with it, the fields are defined as data properties (`Object::with_data_properties`), an absent optional field as `undefined`, and no JavaScript runs; without it, they are set one by one with `Object::set`, an absent optional field is omitted, so a setter on `Object.prototype` runs, and what it throws is passed on. `Error`'s own `FromJS`, which reads `name` and `message`, passes such an error on too. `Lock::throw_exception` throws nothing for that error, so a callback that returns it propagates the original exception to its JavaScript caller. V8 keeps reporting the exception where it was thrown, and DevTools' "pause on uncaught exceptions" still sees whether JavaScript further up catches it. A termination is handled the same way. +When a function that `Local::call` calls throws, the exception stays pending in V8, as with C++ JSG's `JsExceptionThrown`, and `call` returns an error for which `Error::is_pending()` is true. The same goes for these other operations that run JavaScript: converting a value to a string or a number (its `toString`, `valueOf` or `Symbol.toPrimitive`, or a Proxy trap), `Object::get` and `Object::has` (a getter or Proxy trap), `Object::set` (a setter, also one on the prototype chain, or a Proxy trap) and `Array::set` (a setter, also one on the prototype chain), `Array::iterate` and so converting to a `Vec` (an element's getter), and `ser::serialize` and `ser::serialize_with` (a getter or Proxy trap, a `DataCloneError`, or for a value that can't be transferred a `TypeError` or `DataCloneError`). Building the array when converting a `Vec` to JavaScript runs no JavaScript: like C++ JSG, it defines the elements as data properties (`Array::with_elements`), so no setter on `Array.prototype` runs. A `#[jsg_struct]` follows C++ JSG's `fast_jsg_struct` flag: with it, the fields are defined as data properties (`Object::with_data_properties`), an absent optional field as `undefined`, and no JavaScript runs; without it, they are set one by one with `Object::set`, an absent optional field is omitted, so a setter on `Object.prototype` runs, and what it throws is passed on. `Error`'s own `FromJS`, which reads `name` and `message`, passes such an error on too. `Lock::throw_exception` throws nothing for that error, so a callback that returns it propagates the original exception to its JavaScript caller. V8 keeps reporting the exception where it was thrown, and DevTools' "pause on uncaught exceptions" still sees whether JavaScript further up catches it. A termination is handled the same way. Converting a value to JavaScript can fail too, as C++ JSG's `wrap()` can throw, so `ToJS::to_js` returns a `Result`: creating a `Date` or a resource's wrapper fails, leaving the exception pending, when V8 throws or execution is terminating, and a container passes on the error from converting an element. These conversions fail only with pending errors. A `#[jsg_method]`, property getter or wildcard property throws an error from converting its return value as it throws one the method returns, with `Lock::throw_exception`: for a pending error the original exception, or the termination, keeps propagating, and any other error is thrown as a new JavaScript error of its type. The method has already run by then; only its return value is lost. @@ -540,11 +540,19 @@ format version, whether there is a header, whether class instances are written a or rejected (as RPC rejects them), whether errors get back their `stack`, and the label for header errors. Native errors are written as host objects when the `enhanced_error_serialization` flag is on, and workerd API objects such as `Blob` and `Headers` -through their C++ serializers; Rust resources can't be serialized. A `SharedArrayBuffer` is -written as a reference to a buffer that is not part of the bytes, which neither -`ser::deserialize` nor a `jsg::Deserializer` given only the bytes can resolve, so data containing -one can't be read back. Deserialization runs no JavaScript. Transfer lists and externals are not -supported. The C++ side is `src/rust/jsg/ser.c++`. +through their C++ serializers; Rust resources can't be serialized. Deserialization runs no +JavaScript. + +`serialize_with` returns `ser::Serialized`, as `jsg::Serializer::release()` returns `Released`: +the bytes, and the backing stores of the value's `SharedArrayBuffer`s and of the `ArrayBuffer`s +in `SerializeOptions::transfer`, which are detached once the value is written. The data refers +to these buffers by number, so a reader needs their backing stores, which +`Serialized::deserialize_options()` hands it; `ser::serialize` drops them. + +Externals are not supported: without a `jsg::Serializer::ExternalHandler`, functions, `Proxy`s +and API objects that serialize only for RPC (stubs, streams, sockets) throw a `DataCloneError`, +as they do in `jsg::Serializer` without one. +The C++ side is `src/rust/jsg/ser.c++`. ## Type Conversions diff --git a/src/rust/jsg/ffi.h b/src/rust/jsg/ffi.h index 42abd4aaa16..255e8228731 100644 --- a/src/rust/jsg/ffi.h +++ b/src/rust/jsg/ffi.h @@ -34,6 +34,7 @@ struct IsolateState; struct GcVisitor; struct SerializeOptions; struct DeserializeOptions; +struct Serialized; enum class BackingStoreInitializationMode : ::std::uint8_t; enum class ExceptionType : ::std::uint8_t; enum class PromiseState : ::std::uint8_t; @@ -472,23 +473,32 @@ void realm_detach(Isolate* isolate, const IsolateState* state); // In the format of C++ JSG's jsg::Serializer and jsg::Deserializer, which read what these write // and the other way round; see ser.c++. // -// Writes `value` as jsg::Serializer does with `options`: native errors as host objects if +// Writes `value` as jsg::Serializer does with `options`, after transferring each value of +// `transfer` as jsg::Serializer::transfer() does: native errors as host objects if // `options.errors_are_host_objects` (jsg::Lock::isUsingEnhancedErrorSerialization()), and workerd -// API objects through their serializers. Stores the bytes in `result` and returns true, or returns -// false if serialization threw, leaving the exception pending: a DataCloneError, or what a getter, -// Proxy trap or serializer threw. Throws a KJ exception for a version older than 13. +// API objects through their serializers. Detaches the transferred ArrayBuffers, stores the bytes +// and the backing stores of the SharedArrayBuffers and transferred ArrayBuffers in `result`, and +// returns true, or returns false if anything threw, leaving the exception pending: a TypeError or +// DataCloneError for a value that can't be transferred, a DataCloneError for one that can't be +// serialized, or what a getter, Proxy trap or serializer threw. Throws a KJ exception for a +// version older than 13. bool local_serialize(Isolate* isolate, const Local& value, const SerializeOptions& options, - ::rust::Vec& result); -// Reads a value as jsg::Deserializer does with `options`, and returns it, or none if -// deserialization threw, leaving the exception pending. Runs no JavaScript. Throws a KJ exception, -// labeled with `diagnostic_context`, if the header is to be read and is missing or of an -// unsupported version, and for a version older than 13. + ::rust::Slice transfer, + Serialized& result); +// Reads a value as jsg::Deserializer does with `options` and the given backing stores (borrowed +// `std::shared_ptr` addresses), and returns it, or none if deserialization threw, +// leaving the exception pending. Runs no JavaScript. Throws a KJ exception, labeled with +// `diagnostic_context`, if the header is to be read and is missing or of an unsupported version, +// for a version older than 13, and for a transferred backing store that is shared or a shared one +// that is not. MaybeLocal value_deserialize(Isolate* isolate, ::rust::Slice data, const DeserializeOptions& options, - ::rust::Str diagnostic_context); + ::rust::Str diagnostic_context, + ::rust::Slice transferred_array_buffers, + ::rust::Slice shared_array_buffers); // Errors // Builds an exception whose message is raw bytes interpreted as UTF-8. Invalid diff --git a/src/rust/jsg/ser.c++ b/src/rust/jsg/ser.c++ index 14662702848..9eb4fb1e955 100644 --- a/src/rust/jsg/ser.c++ +++ b/src/rust/jsg/ser.c++ @@ -19,6 +19,7 @@ #include #include +#include #include @@ -35,13 +36,44 @@ void throwDataCloneError(v8::Isolate* isolate, v8::Local message) { } } +// Throws a TypeError with `message`, unless execution is terminating. +void throwTypeError(v8::Isolate* isolate, kj::StringPtr message) { + if (isolate->IsExecutionTerminating()) return; + auto messageStr = v8::String::NewFromUtf8( + isolate, message.begin(), v8::NewStringType::kNormal, static_cast(message.size())) + .ToLocalChecked(); + isolate->ThrowException(v8::Exception::TypeError(messageStr)); +} + +// The backing store at `address`, a heap-allocated std::shared_ptr that Rust's +// jsg::v8::BackingStore owns. +const std::shared_ptr& backingStoreAt(size_t address) { + return *reinterpret_cast*>(address); +} + +// Moves `stores` into `addresses` as heap-allocated std::shared_ptrs, which Rust takes ownership +// of. +void releaseBackingStores( + kj::Vector>& stores, ::rust::Vec& addresses) { + addresses.reserve(stores.size()); + for (auto& store: stores) { + addresses.push_back( + reinterpret_cast(new std::shared_ptr(kj::mv(store)))); + } + stores.clear(); +} + class Serializer final: public v8::ValueSerializer::Delegate { public: Serializer(v8::Isolate* isolate, const SerializeOptions& options) : isolate(isolate), options(options), sharedArrayBuffers(isolate), + transferredValues(isolate), + buffersToDetach(isolate), ser(isolate, this) {} + // noexcept(true), as the Delegate's destructor is, though kj::Vector's is not. + ~Serializer() noexcept(true) {} // Applies the options as jsg::Serializer's constructor does. Returns false if V8 rejected the // version, leaving a DataCloneError pending. Throws a KJ exception for a version older than 13. @@ -60,20 +92,67 @@ class Serializer final: public v8::ValueSerializer::Delegate { return true; } + // Transfers `value`, an ArrayBuffer or a view of one, as jsg::Serializer::transfer() does: the + // data refers to the buffer by number, and the buffer is detached once the value is written. + // Returns false if `value` can't be transferred, leaving a TypeError or DataCloneError pending. + bool transfer(v8::Local value) { + v8::Local buffer; + if (value->IsArrayBufferView()) { + buffer = value.As()->Buffer(); + } else if (value->IsArrayBuffer()) { + buffer = value.As(); + } else { + throwTypeError(isolate, "Object is not transferable"_kj); + return false; + } + // A view of a SharedArrayBuffer, which can't be detached. + if (!buffer->IsDetachable()) { + throwDataCloneError( + isolate, v8::String::NewFromUtf8Literal(isolate, "Object is not transferable.")); + return false; + } + // A value given twice is transferred once. Values are compared, not their buffers, as + // jsg::Serializer compares them, so that the numbers are the same. + for (auto transferred: transferredValues) { + if (transferred == value) return true; + } + auto id = static_cast(transferredValues.size()); + transferredValues.push_back(value); + transferredBackingStores.add(buffer->GetBackingStore()); + ser.TransferArrayBuffer(id, buffer); + // Detached once the value is written, as the structured clone algorithm does, so that a getter + // that serialization runs sees the buffer intact, and so that V8 still finds the buffer of a + // view written after it. + buffersToDetach.push_back(buffer); + return true; + } + // Returns false if serialization threw, leaving the exception pending. bool write(v8::Local value) { return ser.WriteValue(isolate->GetCurrentContext(), value).IsJust(); } - ::rust::Vec release() { + // Detaches the transferred buffers and moves the data and the backing stores into `result`, as + // jsg::Serializer::release() does. Returns false if detaching threw, leaving the exception + // pending. + bool release(Serialized& result) { + for (auto buffer: buffersToDetach) { + // JavaScript that serialization ran may have detached it already, as a nested + // structuredClone() that transfers it does. + if (buffer->IsDetachable() && !buffer->WasDetached() && + buffer->Detach(v8::Local()).IsNothing()) { + return false; + } + } auto [data, size] = ser.Release(); KJ_DEFER(free(data)); - ::rust::Vec result; - result.reserve(size); + result.data.reserve(size); for (auto byte: kj::arrayPtr(data, size)) { - result.push_back(byte); + result.data.push_back(byte); } - return result; + releaseBackingStores(sharedBackingStores, result.shared_array_buffers); + releaseBackingStores(transferredBackingStores, result.transferred_array_buffers); + return true; } void ThrowDataCloneError(v8::Local message) override { @@ -102,14 +181,15 @@ class Serializer final: public v8::ValueSerializer::Delegate { return ::workerd::jsg::writeApiObject(isolate, ser, object); } - // Numbers each SharedArrayBuffer, as jsg::Serializer does. The buffers are not passed on: the - // data refers to them by number, and no deserializer is given them. + // Numbers each SharedArrayBuffer and keeps its backing store, as jsg::Serializer does. The data + // refers to the buffer by number; a deserializer given the backing stores shares it. v8::Maybe GetSharedArrayBufferId( v8::Isolate*, v8::Local buffer) override { for (auto i: kj::zeroTo(sharedArrayBuffers.size())) { if (sharedArrayBuffers[i] == buffer) return v8::Just(static_cast(i)); } sharedArrayBuffers.push_back(buffer); + sharedBackingStores.add(buffer->GetBackingStore()); return v8::Just(static_cast(sharedArrayBuffers.size() - 1)); } @@ -119,28 +199,51 @@ class Serializer final: public v8::ValueSerializer::Delegate { // Object.prototype, if `options.class_instances_as_plain_objects` is false. v8::Local prototypeOfObject; v8::LocalVector sharedArrayBuffers; + kj::Vector> sharedBackingStores; + // The values given to transfer(), the backing stores of their buffers, and the buffers. + v8::LocalVector transferredValues; + kj::Vector> transferredBackingStores; + v8::LocalVector buffersToDetach; v8::ValueSerializer ser; }; class Deserializer final: public v8::ValueDeserializer::Delegate { public: - Deserializer( - v8::Isolate* isolate, ::rust::Slice data, const DeserializeOptions& options) + Deserializer(v8::Isolate* isolate, + ::rust::Slice data, + const DeserializeOptions& options, + ::rust::Slice sharedArrayBuffers) : isolate(isolate), options(options), + sharedArrayBuffers(sharedArrayBuffers), deser(isolate, data.data(), data.size(), this) {} - // Applies the options as jsg::Deserializer's constructor does, reading the header unless - // `options.read_header` is false. If the header is missing or of an unsupported version, or the - // version is older than 13, throws a KJ exception, which reaches JavaScript as an internal error; - // if execution is terminating, returns false. - bool init(::rust::Slice data, ::rust::Str diagnosticContext) { + // Applies the options and hands over the transferred buffers as jsg::Deserializer's constructor + // does, reading the header unless `options.read_header` is false. Throws a KJ exception, which + // reaches JavaScript as an internal error, if the header is missing or of an unsupported + // version, the version is older than 13, or a backing store is of the wrong kind; if execution + // is terminating, returns false. + bool init(::rust::Slice data, + ::rust::Str diagnosticContext, + ::rust::Slice transferredArrayBuffers) { + for (auto address: sharedArrayBuffers) { + KJ_REQUIRE(backingStoreAt(address)->IsShared(), + "A SharedArrayBuffer's backing store must be shared."); + } + for (auto address: transferredArrayBuffers) { + KJ_REQUIRE(!backingStoreAt(address)->IsShared(), + "A transferred ArrayBuffer's backing store must not be shared."); + } if (options.read_header && !readHeader(data, diagnosticContext)) return false; if (options.has_version) { KJ_REQUIRE( options.version >= 13, "The minimum serialization version is 13.", options.version); deser.SetWireFormatVersion(options.version); } + for (auto i: kj::zeroTo(transferredArrayBuffers.size())) { + deser.TransferArrayBuffer(static_cast(i), + v8::ArrayBuffer::New(isolate, backingStoreAt(transferredArrayBuffers[i]))); + } return true; } @@ -159,8 +262,21 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { return ::workerd::jsg::readApiObject(isolate, deser, tag); } - // V8's GetSharedArrayBufferFromId() throws for a SharedArrayBuffer, which the data only refers - // to by number. + // A new SharedArrayBuffer over the backing store the data numbers `id`, as jsg::Deserializer + // creates. + v8::MaybeLocal GetSharedArrayBufferFromId( + v8::Isolate*, uint32_t id) override { + if (!options.has_shared_array_buffers) { + // V8 throws its "Unable to deserialize cloned data" error, as it does for + // jsg::Deserializer. + return v8::ValueDeserializer::Delegate::GetSharedArrayBufferFromId(isolate, id); + } + if (id >= sharedArrayBuffers.size()) { + throwCorrupt(); + return {}; + } + return v8::SharedArrayBuffer::New(isolate, backingStoreAt(sharedArrayBuffers[id])); + } private: bool readHeader(::rust::Slice data, ::rust::Str diagnosticContext) { @@ -183,6 +299,7 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { v8::Isolate* isolate; DeserializeOptions options; + ::rust::Slice sharedArrayBuffers; v8::ValueDeserializer deser; }; @@ -192,20 +309,24 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { bool local_serialize(Isolate* isolate, const Local& value, const SerializeOptions& options, - ::rust::Vec& result) { + ::rust::Slice transfer, + Serialized& result) { Serializer serializer(isolate, options); - if (!serializer.init() || !serializer.write(local_as_ref_from_ffi(value))) - return false; - result = serializer.release(); - return true; + if (!serializer.init()) return false; + for (auto& item: transfer) { + if (!serializer.transfer(local_as_ref_from_ffi(item))) return false; + } + return serializer.write(local_as_ref_from_ffi(value)) && serializer.release(result); } MaybeLocal value_deserialize(Isolate* isolate, ::rust::Slice data, const DeserializeOptions& options, - ::rust::Str diagnostic_context) { - Deserializer deserializer(isolate, data, options); - if (!deserializer.init(data, diagnostic_context)) return {}; + ::rust::Str diagnostic_context, + ::rust::Slice transferred_array_buffers, + ::rust::Slice shared_array_buffers) { + Deserializer deserializer(isolate, data, options, shared_array_buffers); + if (!deserializer.init(data, diagnostic_context, transferred_array_buffers)) return {}; return maybe_local_to_ffi(deserializer.read()); } diff --git a/src/rust/jsg/ser.rs b/src/rust/jsg/ser.rs index 1092a12b5a7..840c9939fb2 100644 --- a/src/rust/jsg/ser.rs +++ b/src/rust/jsg/ser.rs @@ -15,8 +15,14 @@ //! - Workerd API objects whose C++ type is serializable, such as `Blob`, `Headers` and //! `DOMException`, through their own serializers. Rust resources are not serializable. //! -//! A `SharedArrayBuffer` is written as a reference that [`deserialize`] cannot resolve. Transfer -//! lists and externals are not supported. +//! A `SharedArrayBuffer` is written as a reference to its backing store, which [`Serialized`] +//! holds alongside the bytes, and is shared by a deserializer given it; so is an `ArrayBuffer` in +//! [`SerializeOptions::transfer`], whose contents move to the deserializer. +//! +//! Externals are not supported: there is no `jsg::Serializer::ExternalHandler` to write an object +//! as a reference to something outside the data, so serializing a function, a `Proxy` or an API +//! object that needs one (an RPC stub, a `ReadableStream`) throws a `DataCloneError`, as +//! `jsg::Serializer` does without a handler. use crate::Error; use crate::Lock; @@ -37,8 +43,8 @@ pub enum ClassInstances { /// How [`serialize_with`] writes a value, as `jsg::Serializer::Options`. The default is /// `jsg::Serializer`'s. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] -pub struct SerializeOptions { +#[derive(Clone, Copy, Debug, Default)] +pub struct SerializeOptions<'a, 's> { /// The format version to write, at least 13, or `None` for V8's current one. Data written in /// an older version can be read by an older V8. pub version: Option, @@ -48,11 +54,41 @@ pub struct SerializeOptions { /// What to do with a class instance, as /// `jsg::Serializer::Options::treatClassInstancesAsPlainObjects`. pub class_instances: ClassInstances, + /// `ArrayBuffer`s, or views of them, whose contents move to the deserializer rather than + /// being copied, as `structuredClone`'s `transfer` option and `jsg::Serializer::transfer()`: + /// the data refers to each by number, and each is detached once the value is written. Their + /// backing stores are in [`Serialized::transferred_array_buffers`]. + pub transfer: &'a [v8::Local<'s, v8::Value>], +} + +/// What [`serialize_with`] writes, as `jsg::Serializer::Released`. +#[derive(Debug, Default)] +pub struct Serialized { + /// The bytes. + pub data: Vec, + /// The backing store of each `SharedArrayBuffer` in the value, in the order the data numbers + /// them. Pass them to [`DeserializeOptions::shared_array_buffers`] to share the buffers. + pub shared_array_buffers: Vec, + /// The backing store of each value in [`SerializeOptions::transfer`], in order. Pass them to + /// [`DeserializeOptions::transferred_array_buffers`]. + pub transferred_array_buffers: Vec, +} + +impl Serialized { + /// The default options with this data's buffers, to read it back as + /// `jsg::Deserializer(js, released)` does. + pub fn deserialize_options(&self) -> DeserializeOptions<'_> { + DeserializeOptions { + transferred_array_buffers: &self.transferred_array_buffers, + shared_array_buffers: Some(&self.shared_array_buffers), + ..Default::default() + } + } } /// How [`deserialize_with`] reads data, as `jsg::Deserializer::Options`. The default is /// `jsg::Deserializer`'s. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug)] pub struct DeserializeOptions<'a> { /// The format version to read in, at least 13, or `None` for the one the header holds. Set /// it for data written without a header. @@ -65,6 +101,14 @@ pub struct DeserializeOptions<'a> { /// What the data is, such as `"RPC result"`, to tell apart the internal errors reported for /// a missing or unsupported header. A fixed label, never user data: it is logged. pub diagnostic_context: Option<&'a str>, + /// The backing stores of the transferred `ArrayBuffer`s, as + /// [`Serialized::transferred_array_buffers`]. Each becomes a new `ArrayBuffer` that shares + /// it. A shared one is an internal error. + pub transferred_array_buffers: &'a [v8::BackingStore], + /// The backing stores of the `SharedArrayBuffer`s, as [`Serialized::shared_array_buffers`], + /// or `None` to throw for data that has one. Each becomes a new `SharedArrayBuffer` that + /// shares it. One that is not shared is an internal error. + pub shared_array_buffers: Option<&'a [v8::BackingStore]>, } impl Default for DeserializeOptions<'_> { @@ -74,6 +118,8 @@ impl Default for DeserializeOptions<'_> { read_header: true, preserve_stack_in_errors: true, diagnostic_context: None, + transferred_array_buffers: &[], + shared_array_buffers: None, } } } @@ -83,17 +129,22 @@ impl Default for DeserializeOptions<'_> { /// # Errors /// /// As for [`serialize_with`]. +/// The data refers to a `SharedArrayBuffer` in `value` by number, without its backing store, so +/// it can't be read back; use [`serialize_with`] to get the backing stores. pub fn serialize<'s>( lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, ) -> Result, Error> { - serialize_with(lock, value, &SerializeOptions::default()) + Ok(serialize_with(lock, value, &SerializeOptions::default())?.data) } -/// Serializes `value` as `jsg::Serializer` does with `options`. +/// Serializes `value` as `jsg::Serializer` does with `options`, transferring +/// `options.transfer` first. /// /// # Errors /// +/// A value in `options.transfer` that is not an `ArrayBuffer` or a view of one throws a +/// `TypeError`, and a view of a `SharedArrayBuffer` a `DataCloneError`; nothing is detached. /// Serialization runs JavaScript (getters and Proxy traps). If it throws, or `value` can't be /// serialized (a `DataCloneError`, as for a class instance with [`ClassInstances::Reject`]), or `options.version` is newer than V8's (a /// `DataCloneError`), or execution is terminating, the exception stays pending and the error is @@ -102,9 +153,12 @@ pub fn serialize<'s>( pub fn serialize_with<'s>( lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, - options: &SerializeOptions, -) -> Result, Error> { + options: &SerializeOptions<'_, 's>, +) -> Result { value.debug_assert_isolate(lock); + for item in options.transfer { + item.debug_assert_isolate(lock); + } let ffi_options = v8::ffi::SerializeOptions { has_version: options.version.is_some(), version: options.version.unwrap_or_default(), @@ -113,18 +167,34 @@ pub fn serialize_with<'s>( errors_are_host_objects: lock.feature_flags().get_enhanced_error_serialization(), }; v8::call_into_js(lock, |lock| { - let mut data = Vec::new(); + let mut result = v8::ffi::Serialized::default(); // SAFETY: the lock guarantees the isolate is locked and a HandleScope is active, and - // `value` belongs to its scope. + // `value` and the values to transfer belong to its scope. let ok = unsafe { v8::ffi::local_serialize( lock.isolate().as_ffi(), value.as_ffi(), &ffi_options, - &mut data, + v8::Local::raw_slice(options.transfer), + &mut result, ) }?; - Ok(ok.then_some(data)) + if !ok { + return Ok(None); + } + let take = |addresses: Vec| { + addresses + .into_iter() + // SAFETY: each is a heap-allocated std::shared_ptr that C++ + // handed over and nothing else owns. + .map(|address| unsafe { v8::BackingStore::from_ffi(address) }) + .collect() + }; + Ok(Some(Serialized { + data: result.data, + shared_array_buffers: take(result.shared_array_buffers), + transferred_array_buffers: take(result.transferred_array_buffers), + })) }) } @@ -146,10 +216,12 @@ pub fn deserialize<'s>( /// /// # Errors /// -/// If `data` is malformed or names an unknown API type, or a type's deserializer throws, or +/// If `data` is malformed or names an unknown API type or buffer, or a type's deserializer +/// throws, or /// execution is terminating, the exception stays pending and the error is one for which /// [`Error::is_pending`] is true. If the header is read and is missing or of an unsupported -/// version, or `options.version` is older than 13, the error is an internal one. +/// version, or `options.version` is older than 13, or a backing store is of the wrong kind, the +/// error is an internal one. pub fn deserialize_with<'s>( lock: &mut Lock<'s>, data: &[u8], @@ -160,7 +232,16 @@ pub fn deserialize_with<'s>( version: options.version.unwrap_or_default(), read_header: options.read_header, preserve_stack_in_errors: options.preserve_stack_in_errors, + has_shared_array_buffers: options.shared_array_buffers.is_some(), + }; + let addresses = |stores: &[v8::BackingStore]| { + stores + .iter() + .map(v8::BackingStore::as_ffi) + .collect::>() }; + let transferred_array_buffers = addresses(options.transferred_array_buffers); + let shared_array_buffers = addresses(options.shared_array_buffers.unwrap_or_default()); let diagnostic_context = options.diagnostic_context.unwrap_or("unknown"); v8::call_into_js(lock, |lock| { // SAFETY: the lock guarantees the isolate is locked and a HandleScope is active. The @@ -171,6 +252,8 @@ pub fn deserialize_with<'s>( data, &ffi_options, diagnostic_context, + &transferred_array_buffers, + &shared_array_buffers, ) }?; // SAFETY: as above. diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 5a38cbc3b75..99c97b5e17c 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -129,6 +129,19 @@ pub mod ffi { version: u32, read_header: bool, preserve_stack_in_errors: bool, + /// Whether the data's `SharedArrayBuffer`s are given, as `jsg::Deserializer`'s + /// `sharedArrayBuffers` is not none. + has_shared_array_buffers: bool, + } + + /// What `ser::serialize_with` writes, as `jsg::Serializer::Released`. Each backing store is + /// the address of a heap-allocated `std::shared_ptr` that the receiver + /// owns. + #[derive(Debug, Default)] + struct Serialized { + data: Vec, + shared_array_buffers: Vec, + transferred_array_buffers: Vec, } /// Mirrors `v8::BackingStoreInitializationMode`. Must be kept in sync with @@ -645,20 +658,24 @@ pub mod ffi { // Serialization // Leaves what serialization throws pending; see `call_into_js()`. Fallible: the result - // is a rust::Vec, and a version older than 13 throws. + // holds rust::Vecs, and a version older than 13 throws. pub unsafe fn local_serialize( isolate: *mut Isolate, value: &Local, options: &SerializeOptions, - result: &mut Vec, + transfer: &[Local], + result: &mut Serialized, ) -> Result; // Leaves what deserialization throws pending. Fallible: a missing or unsupported header, - // or a version older than 13, throws. + // a version older than 13, or a backing store of the wrong kind throws. The backing + // stores are borrowed `BackingStore` addresses. pub unsafe fn value_deserialize( isolate: *mut Isolate, data: &[u8], options: &DeserializeOptions, diagnostic_context: &str, + transferred_array_buffers: &[usize], + shared_array_buffers: &[usize], ) -> Result; // Errors @@ -1150,6 +1167,15 @@ pub struct BackingStore { ptr: NonZeroUsize, } +impl std::fmt::Debug for BackingStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("BackingStore") + .field("byte_length", &self.byte_length()) + .field("is_shared", &self.is_shared()) + .finish() + } +} + impl Drop for BackingStore { fn drop(&mut self) { // SAFETY: ptr was obtained from a `local_*_get_backing_store` FFI call @@ -1184,6 +1210,23 @@ impl BackingStore { } } + /// The address of the `std::shared_ptr` this owns, for C++ to borrow. + #[doc(hidden)] + pub fn as_ffi(&self) -> usize { + self.ptr.get() + } + + /// Takes ownership of the heap-allocated `std::shared_ptr` at `ptr`. + /// + /// # Safety + /// `ptr` must be the non-zero address of a heap-allocated `std::shared_ptr` + /// that nothing else owns or frees. + #[doc(hidden)] + pub unsafe fn from_ffi(ptr: usize) -> Self { + // SAFETY: the caller's guarantees are `from_raw`'s. + unsafe { Self::from_raw(ptr) } + } + /// Returns a raw pointer to the backing store's data. /// /// Valid for the lifetime of this `BackingStore`. @@ -1408,7 +1451,7 @@ impl<'s, T> Local<'s, T> { } /// Views `locals` as the FFI handles C++ functions take, without copying. - fn raw_slice(locals: &[Self]) -> &[ffi::Local] { + pub(crate) fn raw_slice(locals: &[Self]) -> &[ffi::Local] { // SAFETY: as for `raw`, element by element. unsafe { std::slice::from_raw_parts(locals.as_ptr().cast::(), locals.len()) } } From 0606553f9abb75e6bcc95ec68c1bb402f04bb7c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 12:27:29 +0100 Subject: [PATCH 05/10] Read Rust JSG's serialized buffers only with their data DeserializeOptions took any backing stores, so a caller could hand a reader another serialization's buffers, or shared and transferred ones swapped, which the shim had to catch to avoid V8 check failures. C++ JSG can't get there: every caller that gives jsg::Deserializer backing stores takes them from the same serializer's Released. Make the Rust API equally unable to: Serialized's fields are private, with data() and into_data() for the bytes, and its buffers reach a reader only through Serialized::deserialize(), the counterpart of jsg::Deserializer(js, released, options). deserialize_with() reads bytes with no buffers. DeserializeOptions no longer holds backing stores, and the shim no longer checks their kinds. The shim keeps one check: data that names a SharedArrayBuffer beyond the list it came with is read as corrupt, an internal error, rather than indexing past the list. The hidden from_ffi() and backing_store_addresses(), for the jsg-test helpers that compare with C++ JSG, require only owned backing stores of the right kinds, so a test reaches that check without breaking from_ffi()'s contract. --- src/rust/jsg-test/lib.rs | 66 +++++--- src/rust/jsg-test/tests/serialization.rs | 107 ++++++------ src/rust/jsg/README.md | 7 +- src/rust/jsg/ffi.h | 12 +- src/rust/jsg/ser.c++ | 12 +- src/rust/jsg/ser.rs | 197 +++++++++++++---------- src/rust/jsg/v8.rs | 4 +- 7 files changed, 220 insertions(+), 185 deletions(-) diff --git a/src/rust/jsg-test/lib.rs b/src/rust/jsg-test/lib.rs index 2f4b9023d21..c30c26a185f 100644 --- a/src/rust/jsg-test/lib.rs +++ b/src/rust/jsg-test/lib.rs @@ -549,7 +549,7 @@ impl Harness { /// /// Panics if serialization throws. pub fn cpp_serialize<'s>(lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>) -> Vec { - Self::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).data + Self::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).into_data() } /// Serializes `value` with C++ JSG's `jsg::Serializer` and the same options, to compare with @@ -590,18 +590,9 @@ impl Harness { ) } .expect("C++ JSG should serialize the value"); - let take = |addresses: Vec| { - addresses - .into_iter() - // SAFETY: C++ handed over each heap-allocated std::shared_ptr. - .map(|address| unsafe { v8::BackingStore::from_ffi(address) }) - .collect() - }; - jsg::ser::Serialized { - data: result.data, - shared_array_buffers: take(result.shared_array_buffers), - transferred_array_buffers: take(result.transferred_array_buffers), - } + // SAFETY: `jsg::Serializer` released the data and the backing stores, and C++ handed over + // each std::shared_ptr. + unsafe { jsg::ser::Serialized::from_ffi(result) } } /// Deserializes `data` with C++ JSG's `jsg::Deserializer`, to compare with @@ -624,31 +615,56 @@ impl Harness { lock: &mut jsg::Lock<'s>, data: &[u8], options: &jsg::ser::DeserializeOptions<'_>, + ) -> v8::Local<'s, v8::Value> { + Self::cpp_deserialize_impl(lock, data, options, &[], None) + } + + /// Deserializes what `serialized` holds with C++ JSG's `jsg::Deserializer`, `options` and its + /// buffers, as `jsg::Deserializer(js, released, options)` does, to compare with + /// [`jsg::ser::Serialized::deserialize`]. + /// + /// # Panics + /// + /// Panics if deserialization throws. + pub fn cpp_deserialize_serialized<'s>( + lock: &mut jsg::Lock<'s>, + serialized: &jsg::ser::Serialized, + options: &jsg::ser::DeserializeOptions<'_>, + ) -> v8::Local<'s, v8::Value> { + let (transferred, shared) = serialized.backing_store_addresses(); + Self::cpp_deserialize_impl( + lock, + serialized.data(), + options, + &transferred, + Some(&shared), + ) + } + + fn cpp_deserialize_impl<'s>( + lock: &mut jsg::Lock<'s>, + data: &[u8], + options: &jsg::ser::DeserializeOptions<'_>, + transferred: &[usize], + shared: Option<&[usize]>, ) -> v8::Local<'s, v8::Value> { let ffi_options = jsg::v8::ffi::DeserializeOptions { has_version: options.version.is_some(), version: options.version.unwrap_or_default(), read_header: options.read_header, preserve_stack_in_errors: options.preserve_stack_in_errors, - has_shared_array_buffers: options.shared_array_buffers.is_some(), - }; - let addresses = |stores: &[v8::BackingStore]| { - stores - .iter() - .map(v8::BackingStore::as_ffi) - .collect::>() + has_shared_array_buffers: shared.is_some(), }; - let transferred = addresses(options.transferred_array_buffers); - let shared = addresses(options.shared_array_buffers.unwrap_or_default()); - // SAFETY: as for `cpp_serialize_with`; the result belongs to the lock's scope. + // SAFETY: as for `cpp_serialize_with`; the backing stores outlive the call, and the + // result belongs to the lock's scope. unsafe { v8::Local::from_ffi( ffi::cpp_deserialize( lock.isolate().as_ffi(), data, &ffi_options, - &transferred, - &shared, + transferred, + shared.unwrap_or_default(), ) .expect("C++ JSG should deserialize the data"), ) diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index 3742f21b2fb..aee11786ce4 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -381,31 +381,24 @@ fn shared_array_buffers_are_shared_with_the_deserializer() { let serialized = jsg::ser::serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).unwrap(); let cpp = Harness::cpp_serialize_with(lock, value, &jsg::ser::SerializeOptions::default()); - assert_eq!(serialized.data, cpp.data); - assert_eq!(serialized.shared_array_buffers.len(), 2); - assert!( - serialized - .shared_array_buffers - .iter() - .all(v8::BackingStore::is_shared) - ); + assert_eq!(serialized.data(), cpp.data()); + let (transferred, shared) = serialized.backing_store_addresses(); + assert!(transferred.is_empty()); + assert_eq!(shared.len(), 2); + + // Without the backing stores, the data can't be read. + deserialize_exception(lock, serialized.data()); // Each reader reads the other's data with its backing stores, and shares the buffers. let check = "v[0] === v[2] && v[0] !== b && v[1].byteLength === 2 && \ (new Uint8Array(b)[0] = 5, new Uint8Array(v[0])[0] === 5)"; - let copy = jsg::ser::deserialize_with(lock, &cpp.data, &cpp.deserialize_options()).unwrap(); - ctx.set_global(lock, "v", copy); - assert!(ctx.eval::(lock, check).unwrap(), "Rust JSG read"); - let copy = Harness::cpp_deserialize_with( - lock, - &serialized.data, - &serialized.deserialize_options(), - ); + let read = jsg::ser::DeserializeOptions::default(); + let copy = Harness::cpp_deserialize_serialized(lock, &serialized, &read); ctx.set_global(lock, "v", copy); assert!(ctx.eval::(lock, check).unwrap(), "C++ JSG read"); - - // Without the backing stores, the data can't be read. - deserialize_exception(lock, &serialized.data); + let copy = cpp.deserialize(lock, &read).unwrap(); + ctx.set_global(lock, "v", copy); + assert!(ctx.eval::(lock, check).unwrap(), "Rust JSG read"); Ok(()) }); } @@ -422,10 +415,12 @@ fn assert_round_trip_with<'s>( check: &str, ) { let value = eval(lock, ctx, expr); - let data = jsg::ser::serialize_with(lock, value, options).unwrap().data; + let data = jsg::ser::serialize_with(lock, value, options) + .unwrap() + .into_data(); assert_eq!( data, - Harness::cpp_serialize_with(lock, value, options).data, + Harness::cpp_serialize_with(lock, value, options).into_data(), "{expr}: {options:?}" ); @@ -696,14 +691,16 @@ fn transfer_setup<'s>( (value, transfer) } -/// Serializes with each serializer what `setup` sets up, transferring `count` buffers, and checks -/// that the bytes are the same, that `detached` holds after each, and that each reader reads with -/// the other's buffers a value for which `check` holds. +/// Serializes with each serializer what `setup` sets up, transferring `count` values, and checks +/// that the bytes are the same, that each transferred `transferred` buffers, that `detached` +/// holds after each, and that each reader reads with the other's buffers a value for which +/// `check` holds. fn assert_transfer( lock: &mut jsg::Lock<'_>, ctx: &EvalContext<'_>, setup: &str, count: usize, + transferred: usize, detached: &str, check: &str, ) { @@ -728,25 +725,30 @@ fn assert_transfer( ctx.eval::(lock, detached).unwrap(), "{setup}: C++ JSG detached" ); - assert_eq!(serialized.data, cpp.data, "{setup}"); + assert_eq!(serialized.data(), cpp.data(), "{setup}"); + assert_eq!( + serialized.backing_store_addresses().0.len(), + transferred, + "{setup}" + ); assert_eq!( - serialized.transferred_array_buffers.len(), - cpp.transferred_array_buffers.len(), + cpp.backing_store_addresses().0.len(), + transferred, "{setup}" ); - let copy = jsg::ser::deserialize_with(lock, &cpp.data, &cpp.deserialize_options()).unwrap(); + let read = jsg::ser::DeserializeOptions::default(); + let copy = Harness::cpp_deserialize_serialized(lock, &serialized, &read); ctx.set_global(lock, "v", copy); assert!( ctx.eval::(lock, check).unwrap(), - "{setup}: Rust JSG read: {check}" + "{setup}: C++ JSG read: {check}" ); - let copy = - Harness::cpp_deserialize_with(lock, &serialized.data, &serialized.deserialize_options()); + let copy = cpp.deserialize(lock, &read).unwrap(); ctx.set_global(lock, "v", copy); assert!( ctx.eval::(lock, check).unwrap(), - "{setup}: C++ JSG read: {check}" + "{setup}: Rust JSG read: {check}" ); } @@ -760,6 +762,7 @@ fn transferred_array_buffers_move_to_the_deserializer() { "globalThis.b = new ArrayBuffer(4); new Uint8Array(b)[1] = 9; globalThis.value = { b, v: new Uint8Array(b, 1) }; globalThis.transfer = [b];", 1, + 1, "b.byteLength === 0", "v.b.byteLength === 4 && v.v.buffer === v.b && v.v[0] === 9", ); @@ -770,6 +773,7 @@ fn transferred_array_buffers_move_to_the_deserializer() { "globalThis.b = new ArrayBuffer(2); globalThis.u = new Uint8Array(b); u[0] = 3; globalThis.value = [u]; globalThis.transfer = [u, u];", 2, + 1, "b.byteLength === 0 && u.length === 0", "v[0][0] === 3 && v[0].buffer.byteLength === 2", ); @@ -781,6 +785,7 @@ fn transferred_array_buffers_move_to_the_deserializer() { globalThis.value = { get size() { return b.byteLength; }, b }; globalThis.transfer = [b];", 1, + 1, "b.byteLength === 0", "v.size === 8 && v.b.byteLength === 8", ); @@ -790,6 +795,7 @@ fn transferred_array_buffers_move_to_the_deserializer() { ctx, "globalThis.b = new ArrayBuffer(1); globalThis.value = 1; globalThis.transfer = [b];", 1, + 1, "b.byteLength === 0", "v === 1", ); @@ -847,7 +853,7 @@ fn values_that_cannot_be_transferred_throw() { } #[test] -fn buffers_must_match_the_data() { +fn buffers_are_read_only_with_their_data() { Harness::new().run_in_context(|lock, ctx| { let (value, transfer) = transfer_setup( lock, @@ -864,37 +870,22 @@ fn buffers_must_match_the_data() { let shared = jsg::ser::serialize_with(lock, value, &jsg::ser::SerializeOptions::default()).unwrap(); - // Data that refers to buffers the reader isn't given. - deserialize_exception(lock, &transferred.data); - let read = jsg::ser::DeserializeOptions { - shared_array_buffers: Some(&[]), + // The bytes alone don't carry the buffers. + deserialize_exception(lock, transferred.data()); + deserialize_exception(lock, shared.data()); + + // Data that names a SharedArrayBuffer beyond the list it came with is corrupt. + let bare = jsg::v8::ffi::Serialized { + data: shared.into_data(), ..Default::default() }; + // SAFETY: there are no backing stores to own or check. + let bare = unsafe { jsg::ser::Serialized::from_ffi(bare) }; let caught = lock - .try_catch(|lock| jsg::ser::deserialize_with(lock, &shared.data, &read)) + .try_catch(|lock| bare.deserialize(lock, &jsg::ser::DeserializeOptions::default())) .unwrap(); let err = jsg::Error::from_js(lock, caught.unwrap_err()).unwrap(); assert!(err.message.starts_with("internal error"), "{}", err.message); - - // Backing stores of the wrong kind. - for read in [ - jsg::ser::DeserializeOptions { - transferred_array_buffers: &shared.shared_array_buffers, - ..Default::default() - }, - jsg::ser::DeserializeOptions { - shared_array_buffers: Some(&transferred.transferred_array_buffers), - ..Default::default() - }, - ] { - let err = jsg::ser::deserialize_with(lock, &shared.data, &read).unwrap_err(); - assert!(!err.is_pending()); - assert!( - err.message.contains("backing store must"), - "{}", - err.message - ); - } Ok(()) }); } diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index b94d9a86c2f..947c783633c 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -546,8 +546,11 @@ JavaScript. `serialize_with` returns `ser::Serialized`, as `jsg::Serializer::release()` returns `Released`: the bytes, and the backing stores of the value's `SharedArrayBuffer`s and of the `ArrayBuffer`s in `SerializeOptions::transfer`, which are detached once the value is written. The data refers -to these buffers by number, so a reader needs their backing stores, which -`Serialized::deserialize_options()` hands it; `ser::serialize` drops them. +to these buffers by number, so only `Serialized::deserialize` reads it with them, as +`jsg::Deserializer` reads a `Released`. Safe code has no other way to hand a reader backing stores +(the hidden hooks for C++ comparison tests give their addresses, which only `unsafe` code can +use), so a reader can't be given another serialization's buffers. `Serialized::data()` and `into_data()` +give the bytes alone, which `ser::deserialize_with` reads when they refer to no buffer. Externals are not supported: without a `jsg::Serializer::ExternalHandler`, functions, `Proxy`s and API objects that serialize only for RPC (stubs, streams, sockets) throw a `DataCloneError`, diff --git a/src/rust/jsg/ffi.h b/src/rust/jsg/ffi.h index 255e8228731..89acec883dd 100644 --- a/src/rust/jsg/ffi.h +++ b/src/rust/jsg/ffi.h @@ -487,12 +487,12 @@ bool local_serialize(Isolate* isolate, const SerializeOptions& options, ::rust::Slice transfer, Serialized& result); -// Reads a value as jsg::Deserializer does with `options` and the given backing stores (borrowed -// `std::shared_ptr` addresses), and returns it, or none if deserialization threw, -// leaving the exception pending. Runs no JavaScript. Throws a KJ exception, labeled with -// `diagnostic_context`, if the header is to be read and is missing or of an unsupported version, -// for a version older than 13, and for a transferred backing store that is shared or a shared one -// that is not. +// Reads a value as jsg::Deserializer does with `options` and the backing stores of the +// SharedArrayBuffers and transferred ArrayBuffers that the serializer released with the data +// (borrowed `std::shared_ptr` addresses), and returns it, or none if +// deserialization threw, leaving the exception pending. Runs no JavaScript. Throws a KJ exception, +// labeled with `diagnostic_context`, if the header is to be read and is missing or of an +// unsupported version, and for a version older than 13. MaybeLocal value_deserialize(Isolate* isolate, ::rust::Slice data, const DeserializeOptions& options, diff --git a/src/rust/jsg/ser.c++ b/src/rust/jsg/ser.c++ index 9eb4fb1e955..7885532fd17 100644 --- a/src/rust/jsg/ser.c++ +++ b/src/rust/jsg/ser.c++ @@ -221,19 +221,10 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { // Applies the options and hands over the transferred buffers as jsg::Deserializer's constructor // does, reading the header unless `options.read_header` is false. Throws a KJ exception, which // reaches JavaScript as an internal error, if the header is missing or of an unsupported - // version, the version is older than 13, or a backing store is of the wrong kind; if execution - // is terminating, returns false. + // version, or the version is older than 13; if execution is terminating, returns false. bool init(::rust::Slice data, ::rust::Str diagnosticContext, ::rust::Slice transferredArrayBuffers) { - for (auto address: sharedArrayBuffers) { - KJ_REQUIRE(backingStoreAt(address)->IsShared(), - "A SharedArrayBuffer's backing store must be shared."); - } - for (auto address: transferredArrayBuffers) { - KJ_REQUIRE(!backingStoreAt(address)->IsShared(), - "A transferred ArrayBuffer's backing store must not be shared."); - } if (options.read_header && !readHeader(data, diagnosticContext)) return false; if (options.has_version) { KJ_REQUIRE( @@ -271,6 +262,7 @@ class Deserializer final: public v8::ValueDeserializer::Delegate { // jsg::Deserializer. return v8::ValueDeserializer::Delegate::GetSharedArrayBufferFromId(isolate, id); } + // The buffers are the data's own, so only corrupt data names one beyond them. if (id >= sharedArrayBuffers.size()) { throwCorrupt(); return {}; diff --git a/src/rust/jsg/ser.rs b/src/rust/jsg/ser.rs index 840c9939fb2..975cdf7c896 100644 --- a/src/rust/jsg/ser.rs +++ b/src/rust/jsg/ser.rs @@ -15,9 +15,11 @@ //! - Workerd API objects whose C++ type is serializable, such as `Blob`, `Headers` and //! `DOMException`, through their own serializers. Rust resources are not serializable. //! -//! A `SharedArrayBuffer` is written as a reference to its backing store, which [`Serialized`] -//! holds alongside the bytes, and is shared by a deserializer given it; so is an `ArrayBuffer` in -//! [`SerializeOptions::transfer`], whose contents move to the deserializer. +//! The data refers to a `SharedArrayBuffer`, and to an `ArrayBuffer` in +//! [`SerializeOptions::transfer`], by number. Their backing stores stay in the [`Serialized`] +//! that [`serialize_with`] returns, and only [`Serialized::deserialize`] hands them to a reader, +//! so a reader always gets the buffers of the data it reads, as `jsg::Deserializer` does when +//! given a serializer's `Released`. //! //! Externals are not supported: there is no `jsg::Serializer::ExternalHandler` to write an object //! as a reference to something outside the data, so serializing a function, a `Proxy` or an API @@ -54,41 +56,94 @@ pub struct SerializeOptions<'a, 's> { /// What to do with a class instance, as /// `jsg::Serializer::Options::treatClassInstancesAsPlainObjects`. pub class_instances: ClassInstances, - /// `ArrayBuffer`s, or views of them, whose contents move to the deserializer rather than - /// being copied, as `structuredClone`'s `transfer` option and `jsg::Serializer::transfer()`: - /// the data refers to each by number, and each is detached once the value is written. Their - /// backing stores are in [`Serialized::transferred_array_buffers`]. + /// `ArrayBuffer`s, or views of them, whose contents move to the reader rather than being + /// copied, as with `structuredClone`'s `transfer` option and `jsg::Serializer::transfer()`. + /// A value listed twice is transferred once. Each buffer is detached once the value is + /// written. pub transfer: &'a [v8::Local<'s, v8::Value>], } -/// What [`serialize_with`] writes, as `jsg::Serializer::Released`. -#[derive(Debug, Default)] +/// What [`serialize_with`] writes, as `jsg::Serializer::Released`: the bytes, and the backing +/// stores of the buffers they refer to by number. +#[derive(Debug)] pub struct Serialized { - /// The bytes. - pub data: Vec, - /// The backing store of each `SharedArrayBuffer` in the value, in the order the data numbers - /// them. Pass them to [`DeserializeOptions::shared_array_buffers`] to share the buffers. - pub shared_array_buffers: Vec, - /// The backing store of each value in [`SerializeOptions::transfer`], in order. Pass them to - /// [`DeserializeOptions::transferred_array_buffers`]. - pub transferred_array_buffers: Vec, + data: Vec, + shared_array_buffers: Vec, + transferred_array_buffers: Vec, } impl Serialized { - /// The default options with this data's buffers, to read it back as - /// `jsg::Deserializer(js, released)` does. - pub fn deserialize_options(&self) -> DeserializeOptions<'_> { - DeserializeOptions { - transferred_array_buffers: &self.transferred_array_buffers, - shared_array_buffers: Some(&self.shared_array_buffers), - ..Default::default() + /// The bytes. + pub fn data(&self) -> &[u8] { + &self.data + } + + /// The bytes, without the buffers. Data that refers to a buffer can then not be read back. + pub fn into_data(self) -> Vec { + self.data + } + + /// Deserializes the value, as `jsg::Deserializer(js, released, options)` does: each + /// `SharedArrayBuffer` is read as a new one that shares the original's memory, and each + /// transferred `ArrayBuffer` as a new one that holds its contents. Runs no JavaScript. + /// + /// # Errors + /// + /// As for [`deserialize_with`]. + pub fn deserialize<'s>( + self, + lock: &mut Lock<'s>, + options: &DeserializeOptions<'_>, + ) -> Result, Error> { + let (transferred, shared) = self.backing_store_addresses(); + deserialize_impl(lock, &self.data, options, &transferred, Some(&shared)) + } + + /// Takes ownership of what C++ wrote, normally what one `jsg::Serializer` released. Data that + /// names a `SharedArrayBuffer` beyond the list is read as corrupt. + /// + /// # Safety + /// + /// Each backing store must be the address of a heap-allocated + /// `std::shared_ptr` that nothing else owns, those in + /// `shared_array_buffers` shared and those in `transferred_array_buffers` not, as V8 requires + /// of the buffers it creates over them. + #[doc(hidden)] + pub unsafe fn from_ffi(serialized: v8::ffi::Serialized) -> Self { + let take = |addresses: Vec| { + addresses + .into_iter() + // SAFETY: the caller hands over each std::shared_ptr. + .map(|address| unsafe { v8::BackingStore::from_ffi(address) }) + .collect() + }; + Self { + data: serialized.data, + shared_array_buffers: take(serialized.shared_array_buffers), + transferred_array_buffers: take(serialized.transferred_array_buffers), } } + + /// The addresses of the transferred and the shared backing stores, for C++ to borrow, in the + /// order the data numbers them. + #[doc(hidden)] + pub fn backing_store_addresses(&self) -> (Vec, Vec) { + let addresses = |stores: &[v8::BackingStore]| { + stores + .iter() + .map(v8::BackingStore::as_ffi) + .collect::>() + }; + ( + addresses(&self.transferred_array_buffers), + addresses(&self.shared_array_buffers), + ) + } } -/// How [`deserialize_with`] reads data, as `jsg::Deserializer::Options`. The default is -/// `jsg::Deserializer`'s. -#[derive(Clone, Copy, Debug)] +/// How [`deserialize_with`] and [`Serialized::deserialize`] read data, as +/// `jsg::Deserializer::Options`. The default is `jsg::Deserializer`'s. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct DeserializeOptions<'a> { /// The format version to read in, at least 13, or `None` for the one the header holds. Set /// it for data written without a header. @@ -101,14 +156,6 @@ pub struct DeserializeOptions<'a> { /// What the data is, such as `"RPC result"`, to tell apart the internal errors reported for /// a missing or unsupported header. A fixed label, never user data: it is logged. pub diagnostic_context: Option<&'a str>, - /// The backing stores of the transferred `ArrayBuffer`s, as - /// [`Serialized::transferred_array_buffers`]. Each becomes a new `ArrayBuffer` that shares - /// it. A shared one is an internal error. - pub transferred_array_buffers: &'a [v8::BackingStore], - /// The backing stores of the `SharedArrayBuffer`s, as [`Serialized::shared_array_buffers`], - /// or `None` to throw for data that has one. Each becomes a new `SharedArrayBuffer` that - /// shares it. One that is not shared is an internal error. - pub shared_array_buffers: Option<&'a [v8::BackingStore]>, } impl Default for DeserializeOptions<'_> { @@ -118,38 +165,36 @@ impl Default for DeserializeOptions<'_> { read_header: true, preserve_stack_in_errors: true, diagnostic_context: None, - transferred_array_buffers: &[], - shared_array_buffers: None, } } } -/// Serializes `value`, with a header, as `jsg::Serializer` does. +/// Serializes `value`, with a header, as `jsg::Serializer` does, and returns the bytes. Data that +/// refers to a `SharedArrayBuffer` can't be read back from them; [`serialize_with`] keeps the +/// buffers. /// /// # Errors /// /// As for [`serialize_with`]. -/// The data refers to a `SharedArrayBuffer` in `value` by number, without its backing store, so -/// it can't be read back; use [`serialize_with`] to get the backing stores. pub fn serialize<'s>( lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, ) -> Result, Error> { - Ok(serialize_with(lock, value, &SerializeOptions::default())?.data) + Ok(serialize_with(lock, value, &SerializeOptions::default())?.into_data()) } -/// Serializes `value` as `jsg::Serializer` does with `options`, transferring -/// `options.transfer` first. +/// Serializes `value` as `jsg::Serializer` does with `options`, transferring `options.transfer` +/// first. /// /// # Errors /// /// A value in `options.transfer` that is not an `ArrayBuffer` or a view of one throws a /// `TypeError`, and a view of a `SharedArrayBuffer` a `DataCloneError`; nothing is detached. /// Serialization runs JavaScript (getters and Proxy traps). If it throws, or `value` can't be -/// serialized (a `DataCloneError`, as for a class instance with [`ClassInstances::Reject`]), or `options.version` is newer than V8's (a -/// `DataCloneError`), or execution is terminating, the exception stays pending and the error is -/// one for which [`Error::is_pending`] is true. Return it, as for -/// [`v8::Local::::call`]. A version older than 13 is an internal error. +/// serialized (a `DataCloneError`, as for a class instance with [`ClassInstances::Reject`]), or +/// `options.version` is newer than V8's (a `DataCloneError`), or execution is terminating, the +/// exception stays pending and the error is one for which [`Error::is_pending`] is true. Return +/// it, as for [`v8::Local::::call`]. A version older than 13 is an internal error. pub fn serialize_with<'s>( lock: &mut Lock<'s>, value: v8::Local<'s, v8::Value>, @@ -179,22 +224,8 @@ pub fn serialize_with<'s>( &mut result, ) }?; - if !ok { - return Ok(None); - } - let take = |addresses: Vec| { - addresses - .into_iter() - // SAFETY: each is a heap-allocated std::shared_ptr that C++ - // handed over and nothing else owns. - .map(|address| unsafe { v8::BackingStore::from_ffi(address) }) - .collect() - }; - Ok(Some(Serialized { - data: result.data, - shared_array_buffers: take(result.shared_array_buffers), - transferred_array_buffers: take(result.transferred_array_buffers), - })) + // SAFETY: on success the shim hands over the data and the backing stores it wrote. + Ok(ok.then(|| unsafe { Serialized::from_ffi(result) })) }) } @@ -212,48 +243,50 @@ pub fn deserialize<'s>( } /// Deserializes a value that [`serialize_with`] or `jsg::Serializer` wrote, as -/// `jsg::Deserializer` does with `options`. Runs no JavaScript. +/// `jsg::Deserializer` does with `options` and no buffers. Runs no JavaScript. +/// +/// Data that refers to a buffer throws; read it with [`Serialized::deserialize`]. /// /// # Errors /// /// If `data` is malformed or names an unknown API type or buffer, or a type's deserializer -/// throws, or -/// execution is terminating, the exception stays pending and the error is one for which -/// [`Error::is_pending`] is true. If the header is read and is missing or of an unsupported -/// version, or `options.version` is older than 13, or a backing store is of the wrong kind, the -/// error is an internal one. +/// throws, or execution is terminating, the exception stays pending and the error is one for +/// which [`Error::is_pending`] is true. If the header is read and is missing or of an unsupported +/// version, or `options.version` is older than 13, the error is an internal one. pub fn deserialize_with<'s>( lock: &mut Lock<'s>, data: &[u8], options: &DeserializeOptions<'_>, +) -> Result, Error> { + deserialize_impl(lock, data, options, &[], None) +} + +fn deserialize_impl<'s>( + lock: &mut Lock<'s>, + data: &[u8], + options: &DeserializeOptions<'_>, + transferred_array_buffers: &[usize], + shared_array_buffers: Option<&[usize]>, ) -> Result, Error> { let ffi_options = v8::ffi::DeserializeOptions { has_version: options.version.is_some(), version: options.version.unwrap_or_default(), read_header: options.read_header, preserve_stack_in_errors: options.preserve_stack_in_errors, - has_shared_array_buffers: options.shared_array_buffers.is_some(), - }; - let addresses = |stores: &[v8::BackingStore]| { - stores - .iter() - .map(v8::BackingStore::as_ffi) - .collect::>() + has_shared_array_buffers: shared_array_buffers.is_some(), }; - let transferred_array_buffers = addresses(options.transferred_array_buffers); - let shared_array_buffers = addresses(options.shared_array_buffers.unwrap_or_default()); let diagnostic_context = options.diagnostic_context.unwrap_or("unknown"); v8::call_into_js(lock, |lock| { - // SAFETY: the lock guarantees the isolate is locked and a HandleScope is active. The - // result belongs to the lock's scope. + // SAFETY: the lock guarantees the isolate is locked and a HandleScope is active, and the + // backing stores outlive the call. The result belongs to the lock's scope. let value = unsafe { v8::ffi::value_deserialize( lock.isolate().as_ffi(), data, &ffi_options, diagnostic_context, - &transferred_array_buffers, - &shared_array_buffers, + transferred_array_buffers, + shared_array_buffers.unwrap_or_default(), ) }?; // SAFETY: as above. diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 99c97b5e17c..b1e16306d69 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -667,8 +667,8 @@ pub mod ffi { result: &mut Serialized, ) -> Result; // Leaves what deserialization throws pending. Fallible: a missing or unsupported header, - // a version older than 13, or a backing store of the wrong kind throws. The backing - // stores are borrowed `BackingStore` addresses. + // or a version older than 13, throws. The backing stores are borrowed `BackingStore` + // addresses, released with the data. pub unsafe fn value_deserialize( isolate: *mut Isolate, data: &[u8], From 9abf7d6b8c4f50c84285a1596725e9962caae21c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 15:31:20 +0100 Subject: [PATCH 06/10] Keep Rust JSG's Serialized in the isolate that wrote it Serialized was Send, since BackingStore holds only an address, so safe code could move it to another thread and read it into another isolate. When serialization ran a getter that transferred a buffer the outer value also transfers, the outer Serialized still holds that buffer's backing store, which isn't shared, while the inner clone owns a live buffer over it; reading the outer one on another thread would let two threads reach that memory. jsg::Serializer's Released has the same contents, but its callers read it in the isolate that wrote it. Make Serialized neither Send nor Sync, so that it is read where it was written, as Released is. --- src/rust/jsg/README.md | 3 ++- src/rust/jsg/ser.rs | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index 947c783633c..cda4ae9ada2 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -549,7 +549,8 @@ in `SerializeOptions::transfer`, which are detached once the value is written. T to these buffers by number, so only `Serialized::deserialize` reads it with them, as `jsg::Deserializer` reads a `Released`. Safe code has no other way to hand a reader backing stores (the hidden hooks for C++ comparison tests give their addresses, which only `unsafe` code can -use), so a reader can't be given another serialization's buffers. `Serialized::data()` and `into_data()` +use), so a reader can't be given another serialization's buffers. Like `Released`, `Serialized` +is not `Send`: its buffers are read in the isolate that wrote them. `Serialized::data()` and `into_data()` give the bytes alone, which `ser::deserialize_with` reads when they refer to no buffer. Externals are not supported: without a `jsg::Serializer::ExternalHandler`, functions, `Proxy`s diff --git a/src/rust/jsg/ser.rs b/src/rust/jsg/ser.rs index 975cdf7c896..785c4dc86a9 100644 --- a/src/rust/jsg/ser.rs +++ b/src/rust/jsg/ser.rs @@ -26,6 +26,8 @@ //! object that needs one (an RPC stub, a `ReadableStream`) throws a `DataCloneError`, as //! `jsg::Serializer` does without a handler. +use std::marker::PhantomData; + use crate::Error; use crate::Lock; use crate::v8; @@ -65,11 +67,15 @@ pub struct SerializeOptions<'a, 's> { /// What [`serialize_with`] writes, as `jsg::Serializer::Released`: the bytes, and the backing /// stores of the buffers they refer to by number. +/// +/// `Serialized` is neither `Send` nor `Sync`: its buffers are read in the isolate that wrote them, +/// as with `Released`, so that memory that isn't shared is never reached from two threads. #[derive(Debug)] pub struct Serialized { data: Vec, shared_array_buffers: Vec, transferred_array_buffers: Vec, + _not_send: PhantomData<*mut ()>, } impl Serialized { @@ -121,6 +127,7 @@ impl Serialized { data: serialized.data, shared_array_buffers: take(serialized.shared_array_buffers), transferred_array_buffers: take(serialized.transferred_array_buffers), + _not_send: PhantomData, } } From 8882c583fba324b9b07344dbd926cac916a39264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 15:31:48 +0100 Subject: [PATCH 07/10] Throw corrupt SharedArrayBuffer ids as JavaScript errors in jsg::Deserializer jsg::Deserializer::GetSharedArrayBufferFromId() asserted, with KJ_ASSERT, that the id was within the SharedArrayBuffers it was given. V8 calls it from ValueDeserializer::ReadValue(), so a failed assertion threw a KJ exception through V8's frames rather than to a catch in jsg::Deserializer; in ser-test it ends the process with a fatal uncaught kj::Exception. Throw the internal error that other corrupt data throws, as Rust JSG's shim does. Callers give jsg::Deserializer the SharedArrayBuffers of the same serializer's Released, so only corrupt data reaches this; the new ser-test case gives it an empty list. --- src/workerd/jsg/ser-test.c++ | 25 +++++++++++++++++++++++++ src/workerd/jsg/ser.c++ | 6 +++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/workerd/jsg/ser-test.c++ b/src/workerd/jsg/ser-test.c++ index ed297c1243f..cb40aa4eb8e 100644 --- a/src/workerd/jsg/ser-test.c++ +++ b/src/workerd/jsg/ser-test.c++ @@ -232,6 +232,19 @@ struct SerTestContext: public ContextGlobalObject { return jsg::structuredClone(js, value, kj::mv(maybeTransfer)); } + // The bytes Serializer writes for `value`. + kj::Array serializeBytes(Lock& js, JsValue value) { + Serializer ser(js); + ser.write(js, value); + return ser.release().data; + } + + // Reads `data` with Deserializer, given an empty list of SharedArrayBuffers. + JsValue deserializeWithoutSharedArrayBuffers(Lock& js, kj::Array data) { + Deserializer deser(js, data, kj::none, kj::ArrayPtr>()); + return deser.readValue(js); + } + // Reads `data` with Deserializer. JsValue deserializeBytes(Lock& js, kj::Array data) { Deserializer deser(js, data); @@ -250,6 +263,8 @@ struct SerTestContext: public ContextGlobalObject { JSG_METHOD(serializerToForeign); JSG_METHOD(structuredCloneWithTransfer); JSG_METHOD(deserializeBytes); + JSG_METHOD(serializeBytes); + JSG_METHOD(deserializeWithoutSharedArrayBuffers); } }; JSG_DECLARE_ISOLATE_TYPE(SerTestIsolate, @@ -524,5 +539,15 @@ KJ_TEST("malformed native errors throw") { "catch (e) { `${e.name}: ${e.message}` }"), "string", "DataCloneError: Deserialization failed: error message is not a string"); } + +KJ_TEST("a SharedArrayBuffer beyond the given list is corrupt data") { + Evaluator e(v8System); + KJ_EXPECT_LOG(ERROR, "deserialization failure, possible corruption"); + e.expectEval("const data = serializeBytes(new SharedArrayBuffer(1));\n" + "try { deserializeWithoutSharedArrayBuffers(data); } catch (e) {\n" + " e.message.startsWith('internal error; reference = ')\n" + "}", + "boolean", "true"); +} } // namespace } // namespace workerd::jsg::test diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index 77e29cdff41..1f4990e599d 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -474,7 +474,11 @@ kj::String Deserializer::readLengthDelimitedString() { v8::MaybeLocal Deserializer::GetSharedArrayBufferFromId( v8::Isolate* isolate, uint32_t clone_id) { KJ_IF_SOME(backingStores, sharedBackingStores) { - KJ_ASSERT(clone_id < backingStores.size()); + // V8 calls this, so it must throw a JavaScript exception, not a KJ one. + if (clone_id >= backingStores.size()) { + throwCorrupt(isolate); + return {}; + } return v8::SharedArrayBuffer::New(isolate, backingStores[clone_id]); } return v8::MaybeLocal(); From a5bb3e685f7227d0b0842410948e72b356ccbd86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 27 Oct 2026 12:37:35 +0100 Subject: [PATCH 08/10] Transfer each ArrayBuffer once in C++ and Rust JSG serialization jsg::Serializer::transfer() skipped a value already listed, but compared the values, not their buffers, so a transfer list of a view and its buffer, or of two views of one buffer, registered the buffer with V8 more than once. V8 debug builds DCHECK against that in ValueSerializer::TransferArrayBuffer(); release builds let the last registration win and deserialize correctly, with the extra entries in Released::transferredArrayBuffers holding the same backing store. The Rust JSG shim compared values too, to write the same bytes. Compare buffers in both, so each buffer is registered, numbered and released once. Nothing visible to JavaScript changes: the clone is the same, and transferred buffers are only ever read by the deserializer given the same Released, never persisted, so their numbering needs no compatibility. --- src/rust/jsg-test/tests/serialization.rs | 12 +++++++++ src/rust/jsg/ser.c++ | 27 +++++++++---------- src/rust/jsg/ser.rs | 4 +-- src/workerd/jsg/ser-test.c++ | 34 ++++++++++++++++++++++++ src/workerd/jsg/ser.c++ | 8 +++--- src/workerd/jsg/ser.h | 1 + 6 files changed, 66 insertions(+), 20 deletions(-) diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index aee11786ce4..a4f853e0111 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -777,6 +777,18 @@ fn transferred_array_buffers_move_to_the_deserializer() { "b.byteLength === 0 && u.length === 0", "v[0][0] === 3 && v[0].buffer.byteLength === 2", ); + // A view and its buffer, and two views of one buffer, transfer the buffer once. + assert_transfer( + lock, + ctx, + "globalThis.b = new ArrayBuffer(8); globalThis.u = new Uint8Array(b, 4); u[0] = 7; + globalThis.w = new Uint8Array(b); + globalThis.value = { u, b }; globalThis.transfer = [u, b, w];", + 3, + 1, + "b.byteLength === 0 && u.length === 0 && w.length === 0", + "v.u[0] === 7 && v.u.buffer === v.b && v.b.byteLength === 8", + ); // A getter that serialization runs sees the buffer intact: it is detached afterwards. assert_transfer( lock, diff --git a/src/rust/jsg/ser.c++ b/src/rust/jsg/ser.c++ index 7885532fd17..c29ebf1ea91 100644 --- a/src/rust/jsg/ser.c++ +++ b/src/rust/jsg/ser.c++ @@ -69,8 +69,7 @@ class Serializer final: public v8::ValueSerializer::Delegate { : isolate(isolate), options(options), sharedArrayBuffers(isolate), - transferredValues(isolate), - buffersToDetach(isolate), + transferredBuffers(isolate), ser(isolate, this) {} // noexcept(true), as the Delegate's destructor is, though kj::Vector's is not. ~Serializer() noexcept(true) {} @@ -111,19 +110,18 @@ class Serializer final: public v8::ValueSerializer::Delegate { isolate, v8::String::NewFromUtf8Literal(isolate, "Object is not transferable.")); return false; } - // A value given twice is transferred once. Values are compared, not their buffers, as - // jsg::Serializer compares them, so that the numbers are the same. - for (auto transferred: transferredValues) { - if (transferred == value) return true; + // Each buffer is transferred once, however many times it or views of it are listed, as + // jsg::Serializer transfers it: V8 must be told about a buffer only once. + for (auto transferred: transferredBuffers) { + if (transferred == buffer) return true; } - auto id = static_cast(transferredValues.size()); - transferredValues.push_back(value); - transferredBackingStores.add(buffer->GetBackingStore()); - ser.TransferArrayBuffer(id, buffer); + auto id = static_cast(transferredBuffers.size()); // Detached once the value is written, as the structured clone algorithm does, so that a getter // that serialization runs sees the buffer intact, and so that V8 still finds the buffer of a // view written after it. - buffersToDetach.push_back(buffer); + transferredBuffers.push_back(buffer); + transferredBackingStores.add(buffer->GetBackingStore()); + ser.TransferArrayBuffer(id, buffer); return true; } @@ -136,7 +134,7 @@ class Serializer final: public v8::ValueSerializer::Delegate { // jsg::Serializer::release() does. Returns false if detaching threw, leaving the exception // pending. bool release(Serialized& result) { - for (auto buffer: buffersToDetach) { + for (auto buffer: transferredBuffers) { // JavaScript that serialization ran may have detached it already, as a nested // structuredClone() that transfers it does. if (buffer->IsDetachable() && !buffer->WasDetached() && @@ -200,10 +198,9 @@ class Serializer final: public v8::ValueSerializer::Delegate { v8::Local prototypeOfObject; v8::LocalVector sharedArrayBuffers; kj::Vector> sharedBackingStores; - // The values given to transfer(), the backing stores of their buffers, and the buffers. - v8::LocalVector transferredValues; + // The buffers transfer() transferred, in the order they are numbered, and their backing stores. + v8::LocalVector transferredBuffers; kj::Vector> transferredBackingStores; - v8::LocalVector buffersToDetach; v8::ValueSerializer ser; }; diff --git a/src/rust/jsg/ser.rs b/src/rust/jsg/ser.rs index 785c4dc86a9..f00a8400f1b 100644 --- a/src/rust/jsg/ser.rs +++ b/src/rust/jsg/ser.rs @@ -60,8 +60,8 @@ pub struct SerializeOptions<'a, 's> { pub class_instances: ClassInstances, /// `ArrayBuffer`s, or views of them, whose contents move to the reader rather than being /// copied, as with `structuredClone`'s `transfer` option and `jsg::Serializer::transfer()`. - /// A value listed twice is transferred once. Each buffer is detached once the value is - /// written. + /// Each buffer is transferred once, however many times it or views of it are listed, and is + /// detached once the value is written. pub transfer: &'a [v8::Local<'s, v8::Value>], } diff --git a/src/workerd/jsg/ser-test.c++ b/src/workerd/jsg/ser-test.c++ index cb40aa4eb8e..794c8da2402 100644 --- a/src/workerd/jsg/ser-test.c++ +++ b/src/workerd/jsg/ser-test.c++ @@ -251,6 +251,16 @@ struct SerTestContext: public ContextGlobalObject { return deser.readValue(js); } + // How many buffers Serializer transfers when it writes `value` with `transfer`. + uint32_t transferredBufferCount(Lock& js, JsValue value, kj::Array transfer) { + Serializer ser(js); + for (auto& item: transfer) { + ser.transfer(js, item); + } + ser.write(js, value); + return ser.release().transferredArrayBuffers.size(); + } + JSG_RESOURCE_TYPE(SerTestContext) { JSG_NESTED_TYPE(Foo); JSG_NESTED_TYPE(Bar); @@ -262,6 +272,7 @@ struct SerTestContext: public ContextGlobalObject { JSG_METHOD(foreignToSerializer); JSG_METHOD(serializerToForeign); JSG_METHOD(structuredCloneWithTransfer); + JSG_METHOD(transferredBufferCount); JSG_METHOD(deserializeBytes); JSG_METHOD(serializeBytes); JSG_METHOD(deserializeWithoutSharedArrayBuffers); @@ -540,6 +551,28 @@ KJ_TEST("malformed native errors throw") { "string", "DataCloneError: Deserialization failed: error message is not a string"); } +KJ_TEST("transfer transfers each buffer once") { + Evaluator e(v8System); + + // A view and its buffer, and two views of one buffer, each transfer the buffer once. + e.expectEval("const b = new ArrayBuffer(8);\n" + "const u = new Uint8Array(b); u[0] = 7;\n" + "transferredBufferCount({ u, b }, [u, b])", + "number", "1"); + e.expectEval("const b = new ArrayBuffer(8);\n" + "transferredBufferCount([], [new Uint8Array(b), new Uint8Array(b, 4)])", + "number", "1"); + e.expectEval( + "transferredBufferCount([], [new ArrayBuffer(1), new ArrayBuffer(1)])", "number", "2"); + + // The clone keeps the view on the buffer. + e.expectEval("const b = new ArrayBuffer(8);\n" + "const u = new Uint8Array(b, 4); u[0] = 7;\n" + "const out = structuredCloneWithTransfer({ u, b }, [u, b]);\n" + "`${b.byteLength},${out.u[0]},${out.u.buffer === out.b}`", + "string", "0,7,true"); +} + KJ_TEST("a SharedArrayBuffer beyond the given list is corrupt data") { Evaluator e(v8System); KJ_EXPECT_LOG(ERROR, "deserialization failure, possible corruption"); @@ -549,5 +582,6 @@ KJ_TEST("a SharedArrayBuffer beyond the given list is corrupt data") { "}", "boolean", "true"); } + } // namespace } // namespace workerd::jsg::test diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index 1f4990e599d..701ac9d648c 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -324,13 +324,15 @@ void Serializer::transfer(Lock& js, const JsValue& value) { JSG_REQUIRE(arrayBuffer->IsDetachable(), DOMDataCloneError, "Object is not transferable."); uint32_t n; + // Each buffer is transferred once, however many times it or views of it are listed: V8 must + // be told about a buffer only once. + JsValue buffer(arrayBuffer); for (n = 0; n < arrayBuffers.size(); n++) { - // If the ArrayBuffer has already been added, we do not want to try adding it again. - if (arrayBuffers[n].getHandle(js) == value) { + if (arrayBuffers[n].getHandle(js) == buffer) { return; } } - arrayBuffers.add(jsg::JsRef(js, value)); + arrayBuffers.add(jsg::JsRef(js, buffer)); backingStores.add(arrayBuffer->GetBackingStore()); ser.TransferArrayBuffer(n, arrayBuffer); diff --git a/src/workerd/jsg/ser.h b/src/workerd/jsg/ser.h index be2f05d2f3e..e91695658d1 100644 --- a/src/workerd/jsg/ser.h +++ b/src/workerd/jsg/ser.h @@ -223,6 +223,7 @@ class Serializer final: v8::ValueSerializer::Delegate { kj::Maybe externalHandler; kj::Vector> sharedArrayBuffers; + // The buffers passed to transfer(), or of the views passed to it, in the order they are numbered. kj::Vector> arrayBuffers; // ArrayBuffers passed to transfer() that still need to be detached. We defer detaching until // release() (i.e. after write()) -- see transfer() for why. From ac365097321687efb60055cc09eb8203c616198a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 28 Oct 2026 10:32:36 +0100 Subject: [PATCH 09/10] Copy native errors' own properties as data properties when serializing writeNativeError() copies an error's own properties, other than name and message, to a fresh object that it writes, and copied them with Set(). That ran a setter that Object.prototype has for a key instead of copying the property, which was then missing from the clone, and a setter that threw made serialization throw. An own `__proto__` property replaced the object's prototype instead and was lost. Define them as data properties, as readNativeError() does when reading them back, so that each property is copied as it is and no JavaScript runs for it. Compatibility: this changes what Workers can observe with enhanced_error_serialization, which is on by default from 2026-04-21, for structuredClone and everything else that serializes errors: a setter on Object.prototype for one of an error's own property names no longer runs, and the property, like an own `__proto__`, now reaches the clone. 2e1193033 kept Set() here when it moved deserialization to data properties. We accept the risk without a compatibility flag: it reaches only Workers that install such setters and clone errors with matching own properties, and the new behavior is what the clone of any other error already gets. The data stays readable by every version of jsg::Deserializer, which reads the properties as an ordinary object. error-deser-prototype-setter-test now expects the setter not to run and the property to round-trip, and the Rust JSG tests expect the same of both serializers. Suggested in review. --- src/rust/jsg-test/tests/serialization.rs | 22 ++++++++++++------- .../error-deser-prototype-setter-test.js | 16 +++++++++----- src/workerd/jsg/ser.c++ | 5 ++++- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index a4f853e0111..a6cf64c1422 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -183,14 +183,21 @@ fn errors_lose_what_cpp_jsg_loses() { "Object.assign(new Error('m'), { name: 'Error\\0custom' })", "v.name === 'Error' && !Object.hasOwn(v, 'name') && v.message === 'm'", ); + Ok(()) + }); +} - // The other own properties are copied with an assignment, which runs a setter that - // `Object.prototype` has for the property instead of copying it. +/// An error's other own properties are copied as data properties, so no setter that +/// `Object.prototype` has for one runs, and an own `__proto__` is copied as it is. +#[test] +fn error_properties_are_copied_as_they_are() { + let harness = Harness::with_flags(|mut flags| flags.set_enhanced_error_serialization(true)); + harness.run_in_context(|lock, ctx| { ctx.eval_raw( lock, "globalThis.setterRuns = 0; Object.defineProperty(Object.prototype, 'x', { - set(value) { globalThis.setterRuns++; }, + set(value) { globalThis.setterRuns++; throw new Error('setter'); }, configurable: true, });", ) @@ -199,18 +206,17 @@ fn errors_lose_what_cpp_jsg_loses() { lock, ctx, "Object.defineProperty(new Error('e'), 'x', { value: 1, enumerable: true })", - "!Object.hasOwn(v, 'x')", + "Object.hasOwn(v, 'x') && v.x === 1", ); - // Once for each serializer. - assert!(ctx.eval::(lock, "globalThis.setterRuns === 2").unwrap()); + assert!(ctx.eval::(lock, "globalThis.setterRuns === 0").unwrap()); ctx.eval_raw(lock, "delete Object.prototype.x").unwrap(); - // An own `__proto__` sets the prototype of the object it is copied to, and is lost. assert_round_trip( lock, ctx, "Object.defineProperty(new Error('e'), '__proto__', { value: { y: 1 }, enumerable: true })", - "!Object.hasOwn(v, '__proto__') && v.y === undefined && v.message === 'e'", + "Object.getOwnPropertyDescriptor(v, '__proto__').value.y === 1 && \ + Object.getPrototypeOf(v) === Error.prototype && v.message === 'e'", ); Ok(()) }); diff --git a/src/workerd/api/tests/error-deser-prototype-setter-test.js b/src/workerd/api/tests/error-deser-prototype-setter-test.js index 0428a72a65b..a5c0bfe567a 100644 --- a/src/workerd/api/tests/error-deser-prototype-setter-test.js +++ b/src/workerd/api/tests/error-deser-prototype-setter-test.js @@ -62,7 +62,8 @@ export const errorDeserPrototypeSetterRegression = { }; // Also verify the serialization side: when the serializer copies own -// properties into a temporary plain object, it can safely use Set() +// properties into a temporary plain object, it defines them as data +// properties, so no Object.prototype setter runs and each property is kept. export const errorSerPrototypeSetterRegression = { test() { let setterInvoked = false; @@ -85,13 +86,18 @@ export const errorSerPrototypeSetterRegression = { configurable: true, }); - // This exercises the serialization path (ser.c++:286) where own + // This exercises the serialization path (writeNativeError()) where own // properties are copied to a temporary plain object. - const _clone = structuredClone(err); + const clone = structuredClone(err); + strictEqual( + Object.getOwnPropertyDescriptor(clone, 'serprop')?.value, + 99, + 'own data property serprop should round-trip with value 99' + ); ok( - setterInvoked, - 'Object.prototype setter must be invoked during serialization' + !setterInvoked, + 'Object.prototype setter must not be invoked during serialization' ); } finally { delete Object.prototype.serprop; diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index 701ac9d648c..39f78f160b1 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -585,9 +585,12 @@ v8::Maybe writeNativeError( if (!keys->Get(context, i).ToLocal(&key)) return v8::Nothing(); // The name and message are written above. if (key->StrictEquals(nameKey) || key->StrictEquals(messageKey)) continue; + // A data property rather than Set(), as when reading, so that each property is copied as it + // is: Set() would run a setter that Object.prototype has for the key instead, and an own + // `__proto__` would replace the object's prototype. v8::Local value; if (!error->Get(context, key).ToLocal(&value) || - properties->Set(context, key, value).IsNothing()) { + properties->CreateDataProperty(context, key.As(), value).IsNothing()) { return v8::Nothing(); } } From 62c289a9a43b23fe899fbcbfd535016307c357e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 28 Oct 2026 16:58:42 +0100 Subject: [PATCH 10/10] Test a transferred buffer that serialization detaches A getter that serialization runs can detach a buffer that is being transferred, as a nested structuredClone() that transfers it does. Both serializers' release() then skip it rather than detaching it again, and the data still reads with either deserializer. Test that, with ArrayBuffer.prototype.transfer() in the getter, since the test harness has no structuredClone(). Removing the WasDetached() check in release() doesn't make this test fail: V8's JSArrayBuffer::Detach() returns early for a detached buffer, so detaching twice is a no-op today. The check guards against V8 changing that, as jsg::Serializer::release()'s does. Suggested in review. --- src/rust/jsg-test/tests/serialization.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/rust/jsg-test/tests/serialization.rs b/src/rust/jsg-test/tests/serialization.rs index a6cf64c1422..a49b3f646ca 100644 --- a/src/rust/jsg-test/tests/serialization.rs +++ b/src/rust/jsg-test/tests/serialization.rs @@ -807,6 +807,20 @@ fn transferred_array_buffers_move_to_the_deserializer() { "b.byteLength === 0", "v.size === 8 && v.b.byteLength === 8", ); + // A getter that serialization runs can detach a transferred buffer itself, as a nested + // structuredClone() that transfers it does. release() skips it rather than detaching it + // again, which V8 happens to allow today. + assert_transfer( + lock, + ctx, + "globalThis.b = new ArrayBuffer(8); new Uint8Array(b)[0] = 42; + globalThis.value = { get moved() { return new Uint8Array(b.transfer()); } }; + globalThis.transfer = [b];", + 1, + 1, + "b.detached", + "v.moved[0] === 42 && v.moved.length === 8", + ); // A buffer that the value doesn't hold is still detached, and passed on. assert_transfer( lock,