From c5f0f120f1f1c67a8fde1e843d396bee4bb9210f Mon Sep 17 00:00:00 2001 From: Till Schneidereit Date: Tue, 18 Jun 2024 15:13:45 +0200 Subject: [PATCH] Improve handling of GC managed array buffers Specifically, some of the `noGC` guards had issues that could lead to panics during exception throwing, and some of the loops over multiple buffers where overly complex. --- builtins/web/base64.cpp | 38 ++++++----- builtins/web/crypto/crypto.cpp | 1 + builtins/web/fetch/request-response.cpp | 71 +++++++-------------- builtins/web/streams/compression-stream.cpp | 2 +- 4 files changed, 42 insertions(+), 70 deletions(-) diff --git a/builtins/web/base64.cpp b/builtins/web/base64.cpp index 5b34dd2..956ad57 100644 --- a/builtins/web/base64.cpp +++ b/builtins/web/base64.cpp @@ -20,35 +20,33 @@ JS::Result convertJSValueToByteString(JSContext *cx, JS::Handle(JS::Error()); + } + + for (size_t i = 0; i < length; i++) { + if (chars[i] > 255) { + // Reset the nogc guard, since otherwise we can't throw errors. + nogc.reset(); JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_INVALID_CHARACTER_ERROR); return JS::Result(JS::Error()); } - - for (size_t i = 0; i < length; i++) { - if (chars[i] > 255) { - foundBadChar = true; - break; - } - } - } - - if (foundBadChar) { - JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_INVALID_CHARACTER_ERROR); - return JS::Result(JS::Error()); } + auto latin1_z = + JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, chars, length); + result = UniqueChars(reinterpret_cast(latin1_z.get())); } else { length = JS::GetStringLength(s); + result = JS_EncodeStringToLatin1(cx, s); } - UniqueChars result = JS_EncodeStringToLatin1(cx, s); if (!result) { return JS::Result(JS::Error()); } diff --git a/builtins/web/crypto/crypto.cpp b/builtins/web/crypto/crypto.cpp index 773f9bc..3504f9b 100644 --- a/builtins/web/crypto/crypto.cpp +++ b/builtins/web/crypto/crypto.cpp @@ -54,6 +54,7 @@ bool Crypto::get_random_values(JSContext *cx, unsigned argc, JS::Value *vp) { auto res = host_api::Random::get_bytes(byte_length); if (auto *err = res.to_err()) { + noGC.reset(); HANDLE_ERROR(cx, *err); return false; } diff --git a/builtins/web/fetch/request-response.cpp b/builtins/web/fetch/request-response.cpp index 42a9ebc..8bc34ac 100644 --- a/builtins/web/fetch/request-response.cpp +++ b/builtins/web/fetch/request-response.cpp @@ -621,71 +621,44 @@ bool RequestOrResponse::content_stream_read_then_handler(JSContext *cx, JS::Hand if (!JS::GetArrayLength(cx, contents, &contentsLength)) { return false; } - // TODO(performance): investigate whether we can infer the size directly from `contents` - size_t buf_size = HANDLE_READ_CHUNK_SIZE; - // TODO(performance): make use of malloc slack. - // https://github.com/fastly/js-compute-runtime/issues/217 - size_t offset = 0; - // In this loop we are finding the length of each entry in `contents` and resizing the `buf` - // until it is large enough to fit all the entries in `contents` - for (uint32_t index = 0; index < contentsLength; index++) { - JS::RootedValue val(cx); + + size_t total_length = 0; + RootedValue val(cx); + + for (size_t index = 0; index < contentsLength; index++) { if (!JS_GetElement(cx, contents, index, &val)) { return false; } - { - JS::AutoCheckCannotGC nogc; - MOZ_ASSERT(val.isObject()); - JSObject *array = &val.toObject(); - MOZ_ASSERT(JS_IsUint8Array(array)); - size_t length = JS_GetTypedArrayByteLength(array); - if (length) { - offset += length; - // if buf is not big enough to fit the next uint8array's bytes then resize - if (offset > buf_size) { - buf_size = - buf_size + (HANDLE_READ_CHUNK_SIZE * ((length / HANDLE_READ_CHUNK_SIZE) + 1)); - } - } - } + JSObject *array = &val.toObject(); + size_t length = JS_GetTypedArrayByteLength(array); + total_length += length; } - JS::UniqueChars buf{static_cast(JS_malloc(cx, buf_size + 1))}; + JS::UniqueChars buf{static_cast(JS_malloc(cx, total_length))}; if (!buf) { JS_ReportOutOfMemory(cx); return false; } - // reset the offset for the next loop - offset = 0; + + size_t offset = 0; // In this loop we are inserting each entry in `contents` into `buf` for (uint32_t index = 0; index < contentsLength; index++) { - JS::RootedValue val(cx); if (!JS_GetElement(cx, contents, index, &val)) { return false; } - { - JS::AutoCheckCannotGC nogc; - MOZ_ASSERT(val.isObject()); - JSObject *array = &val.toObject(); - MOZ_ASSERT(JS_IsUint8Array(array)); - bool is_shared; - size_t length = JS_GetTypedArrayByteLength(array); - if (length) { - static_assert(CHAR_BIT == 8, "Strange char"); - auto bytes = reinterpret_cast(JS_GetUint8ArrayData(array, &is_shared, nogc)); - memcpy(buf.get() + offset, bytes, length); - offset += length; - } - } - } - buf[offset] = '\0'; -#ifdef DEBUG - bool foundBodyParser; - if (!JS_HasElement(cx, catch_handler, 2, &foundBodyParser)) { - return false; + JSObject *array = &val.toObject(); + bool is_shared; + size_t length = JS_GetTypedArrayByteLength(array); + JS::AutoCheckCannotGC nogc(cx); + auto bytes = reinterpret_cast(JS_GetUint8ArrayData(array, &is_shared, nogc)); + memcpy(buf.get() + offset, bytes, length); + offset += length; } + + mozilla::DebugOnly foundBodyParser = false; + MOZ_ASSERT(JS_HasElement(cx, catch_handler, 2, &foundBodyParser)); MOZ_ASSERT(foundBodyParser); -#endif + // Now we can call parse_body on the result JS::RootedValue body_parser(cx); if (!JS_GetElement(cx, catch_handler, 2, &body_parser)) { diff --git a/builtins/web/streams/compression-stream.cpp b/builtins/web/streams/compression-stream.cpp index 7774f28..90fc328 100644 --- a/builtins/web/streams/compression-stream.cpp +++ b/builtins/web/streams/compression-stream.cpp @@ -129,7 +129,7 @@ bool deflate_chunk(JSContext *cx, JS::HandleObject self, JS::HandleValue chunk, { bool is_shared; - JS::AutoCheckCannotGC nogc; + JS::AutoCheckCannotGC nogc(cx); uint8_t *out_buffer = JS_GetUint8ArrayData(out_obj, &is_shared, nogc); memcpy(out_buffer, buffer, bytes); }