From add8bf4790cc62a84005ee2a4990c9496f9972e2 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sat, 18 Jan 2025 00:43:11 +1300 Subject: [PATCH] LibWeb/Streams: Ensure pending pull into's objects are visited by GC While PendingPullIntos are typically visted by their controller there were some cases that we were removing those references from the controller and storing them in a SinglyLinkedList on the stack which is not safe. Instead, make PendingPullInto a GC::Cell type, which also allows us to remove an awkward copy of the struct where the underlying reference was previously being destroyed. --- .../LibWeb/Streams/AbstractOperations.cpp | 70 +++++++++---------- Libraries/LibWeb/Streams/AbstractOperations.h | 6 +- .../Streams/ReadableByteStreamController.cpp | 41 ++++++----- .../Streams/ReadableByteStreamController.h | 30 ++++++-- 4 files changed, 86 insertions(+), 61 deletions(-) diff --git a/Libraries/LibWeb/Streams/AbstractOperations.cpp b/Libraries/LibWeb/Streams/AbstractOperations.cpp index 33d8b08b8b1..f341986c9fb 100644 --- a/Libraries/LibWeb/Streams/AbstractOperations.cpp +++ b/Libraries/LibWeb/Streams/AbstractOperations.cpp @@ -1,7 +1,7 @@ /* * Copyright (c) 2022, Linus Groh * Copyright (c) 2023, Matthew Olsson - * Copyright (c) 2023-2024, Shannon Booth + * Copyright (c) 2023-2025, Shannon Booth * Copyright (c) 2023-2024, Kenneth Myhra * * SPDX-License-Identifier: BSD-2-Clause @@ -1686,7 +1686,7 @@ void readable_stream_byob_reader_error_read_into_requests(ReadableStreamBYOBRead void readable_byte_stream_controller_fill_head_pull_into_descriptor(ReadableByteStreamController const& controller, u64 size, PullIntoDescriptor& pull_into_descriptor) { // 1. Assert: either controller.[[pendingPullIntos]] is empty, or controller.[[pendingPullIntos]][0] is pullIntoDescriptor. - VERIFY(controller.pending_pull_intos().is_empty() || &controller.pending_pull_intos().first() == &pull_into_descriptor); + VERIFY(controller.pending_pull_intos().is_empty() || controller.pending_pull_intos().first().ptr() == &pull_into_descriptor); // 2. Assert: controller.[[byobRequest]] is null. VERIFY(!controller.raw_byob_request()); @@ -1915,17 +1915,16 @@ void readable_byte_stream_controller_pull_into(ReadableByteStreamController& con // 10. Let pullIntoDescriptor be a new pull-into descriptor with buffer buffer, buffer byte length buffer.[[ArrayBufferByteLength]], // byte offset byteOffset, byte length byteLength, bytes filled 0, element size elementSize, view constructor ctor, and reader type "byob". - PullIntoDescriptor pull_into_descriptor { - .buffer = buffer, - .buffer_byte_length = buffer->byte_length(), - .byte_offset = byte_offset, - .byte_length = byte_length, - .bytes_filled = 0, - .minimum_fill = minimum_fill, - .element_size = element_size, - .view_constructor = *ctor, - .reader_type = ReaderType::Byob, - }; + auto pull_into_descriptor = vm.heap().allocate( + buffer, + buffer->byte_length(), + byte_offset, + byte_length, + 0, + minimum_fill, + element_size, + *ctor, + ReaderType::Byob); // 11. If controller.[[pendingPullIntos]] is not empty, if (!controller.pending_pull_intos().is_empty()) { @@ -1942,7 +1941,7 @@ void readable_byte_stream_controller_pull_into(ReadableByteStreamController& con // 12. If stream.[[state]] is "closed", if (stream->is_closed()) { // 1. Let emptyView be ! Construct(ctor, « pullIntoDescriptor’s buffer, pullIntoDescriptor’s byte offset, 0 »). - auto empty_view = MUST(JS::construct(vm, *ctor, pull_into_descriptor.buffer, JS::Value(pull_into_descriptor.byte_offset), JS::Value(0))); + auto empty_view = MUST(JS::construct(vm, *ctor, pull_into_descriptor->buffer, JS::Value(pull_into_descriptor->byte_offset), JS::Value(0))); // 2. Perform readIntoRequest’s close steps, given emptyView. read_into_request.on_close(empty_view); @@ -2263,7 +2262,7 @@ GC::Ptr readable_byte_stream_controller_get_byob_requ // 1. If controller.[[byobRequest]] is null and controller.[[pendingPullIntos]] is not empty, if (!controller->raw_byob_request() && !controller->pending_pull_intos().is_empty()) { // 1. Let firstDescriptor be controller.[[pendingPullIntos]][0]. - auto const& first_descriptor = controller->pending_pull_intos().first(); + auto const& first_descriptor = *controller->pending_pull_intos().first(); // 2. Let view be ! Construct(%Uint8Array%, « firstDescriptor’s buffer, firstDescriptor’s byte offset + firstDescriptor’s bytes filled, firstDescriptor’s byte length − firstDescriptor’s bytes filled »). auto view = MUST(JS::construct(vm, *realm.intrinsics().uint8_array_constructor(), first_descriptor.buffer, JS::Value(first_descriptor.byte_offset + first_descriptor.bytes_filled), JS::Value(first_descriptor.byte_length - first_descriptor.bytes_filled))); @@ -2319,7 +2318,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_respond_in_readable_st // 3. For each filledPullInto of filledPullIntos, for (auto& filled_pull_into : filled_pulled_intos) { // 1. Perform ! ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], filledPullInto). - readable_byte_stream_controller_commit_pull_into_descriptor(*controller.stream(), filled_pull_into); + readable_byte_stream_controller_commit_pull_into_descriptor(*controller.stream(), *filled_pull_into); } // 4. Return. @@ -2333,34 +2332,33 @@ WebIDL::ExceptionOr readable_byte_stream_controller_respond_in_readable_st // NOTE: A descriptor for a read() request that is not yet filled up to its minimum length will stay at the head of the queue, so the underlying source can keep filling it. // 5. Perform ! ReadableByteStreamControllerShiftPendingPullInto(controller). - // NOTE: We need to take a copy of pull_into_descriptor here as the shift destroys the pull into descriptor we are given. - auto pull_into_descriptor_copy = readable_byte_stream_controller_shift_pending_pull_into(controller); + readable_byte_stream_controller_shift_pending_pull_into(controller); // 6. Let remainderSize be the remainder after dividing pullIntoDescriptor’s bytes filled by pullIntoDescriptor’s element size. - auto remainder_size = pull_into_descriptor_copy.bytes_filled % pull_into_descriptor_copy.element_size; + auto remainder_size = pull_into_descriptor.bytes_filled % pull_into_descriptor.element_size; // 7. If remainderSize > 0, if (remainder_size > 0) { // 1. Let end be pullIntoDescriptor’s byte offset + pullIntoDescriptor’s bytes filled. - auto end = pull_into_descriptor_copy.byte_offset + pull_into_descriptor_copy.bytes_filled; + auto end = pull_into_descriptor.byte_offset + pull_into_descriptor.bytes_filled; // 2. Perform ? ReadableByteStreamControllerEnqueueClonedChunkToQueue(controller, pullIntoDescriptor’s buffer, end − remainderSize, remainderSize). - TRY(readable_byte_stream_controller_enqueue_cloned_chunk_to_queue(controller, *pull_into_descriptor_copy.buffer, end - remainder_size, remainder_size)); + TRY(readable_byte_stream_controller_enqueue_cloned_chunk_to_queue(controller, *pull_into_descriptor.buffer, end - remainder_size, remainder_size)); } // 8. Set pullIntoDescriptor’s bytes filled to pullIntoDescriptor’s bytes filled − remainderSize. - pull_into_descriptor_copy.bytes_filled -= remainder_size; + pull_into_descriptor.bytes_filled -= remainder_size; // 9. Let filledPullIntos be the result of performing ! ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller). auto filled_pulled_intos = readable_byte_stream_controller_process_pull_into_descriptors_using_queue(controller); // 10. Perform ! ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], pullIntoDescriptor). - readable_byte_stream_controller_commit_pull_into_descriptor(*controller.stream(), pull_into_descriptor_copy); + readable_byte_stream_controller_commit_pull_into_descriptor(*controller.stream(), pull_into_descriptor); // 11. For each filledPullInto of filledPullIntos, for (auto& filled_pull_into : filled_pulled_intos) { // 1. Perform ! ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], filledPullInto). - readable_byte_stream_controller_commit_pull_into_descriptor(*controller.stream(), filled_pull_into); + readable_byte_stream_controller_commit_pull_into_descriptor(*controller.stream(), *filled_pull_into); } return {}; } @@ -2381,7 +2379,7 @@ void readable_byte_stream_controller_respond_in_closed_state(ReadableByteStreamC // 4. If ! ReadableStreamHasBYOBReader(stream) is true, if (readable_stream_has_byob_reader(stream)) { // 1. Let filledPullIntos be a new empty list. - SinglyLinkedList filled_pull_intos; + SinglyLinkedList> filled_pull_intos; // 2. Let i be 0. u64 i = 0; @@ -2400,7 +2398,7 @@ void readable_byte_stream_controller_respond_in_closed_state(ReadableByteStreamC // 4. For each filledPullInto of filledPullIntos, for (auto& filled_pull_into : filled_pull_intos) { // 1. Perform ! ReadableByteStreamControllerCommitPullIntoDescriptor(stream, filledPullInto). - readable_byte_stream_controller_commit_pull_into_descriptor(stream, filled_pull_into); + readable_byte_stream_controller_commit_pull_into_descriptor(stream, *filled_pull_into); } } } @@ -2410,7 +2408,7 @@ void readable_byte_stream_controller_respond_in_closed_state(ReadableByteStreamC WebIDL::ExceptionOr readable_byte_stream_controller_respond_internal(ReadableByteStreamController& controller, u64 bytes_written) { // 1. Let firstDescriptor be controller.[[pendingPullIntos]][0]. - auto& first_descriptor = controller.pending_pull_intos().first(); + auto& first_descriptor = *controller.pending_pull_intos().first(); // 2. Assert: ! CanTransferArrayBuffer(firstDescriptor’s buffer) is true. VERIFY(can_transfer_array_buffer(*first_descriptor.buffer)); @@ -2456,7 +2454,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_respond(ReadableByteSt VERIFY(!controller.pending_pull_intos().is_empty()); // 2. Let firstDescriptor be controller.[[pendingPullIntos]][0]. - auto& first_descriptor = controller.pending_pull_intos().first(); + auto& first_descriptor = *controller.pending_pull_intos().first(); // 3. Let state be controller.[[stream]].[[state]]. auto state = controller.stream()->state(); @@ -2498,7 +2496,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_respond_with_new_view( VERIFY(!view.viewed_array_buffer()->is_detached()); // 3. Let firstDescriptor be controller.[[pendingPullIntos]][0]. - auto& first_descriptor = controller.pending_pull_intos().first(); + auto& first_descriptor = *controller.pending_pull_intos().first(); // 4. Let state be controller.[[stream]].[[state]]. auto state = controller.stream()->state(); @@ -2809,7 +2807,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_close(ReadableByteStre // 4. If controller.[[pendingPullIntos]] is not empty, if (!controller.pending_pull_intos().is_empty()) { // 1. Let firstPendingPullInto be controller.[[pendingPullIntos]][0]. - auto& first_pending_pull_into = controller.pending_pull_intos().first(); + auto& first_pending_pull_into = *controller.pending_pull_intos().first(); // 2. If the remainder after dividing firstPendingPullInto’s bytes filled by firstPendingPullInto’s element size is not 0, if (first_pending_pull_into.bytes_filled % first_pending_pull_into.element_size != 0) { @@ -3312,7 +3310,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_enqueue(ReadableByteSt // 8. If controller.[[pendingPullIntos]] is not empty, if (!controller.pending_pull_intos().is_empty()) { // 1. Let firstPendingPullInto be controller.[[pendingPullIntos]][0]. - auto& first_pending_pull_into = controller.pending_pull_intos().first(); + auto& first_pending_pull_into = *controller.pending_pull_intos().first(); // 2. If ! IsDetachedBuffer(firstPendingPullInto’s buffer) is true, throw a TypeError exception. if (first_pending_pull_into.buffer->is_detached()) { @@ -3352,7 +3350,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_enqueue(ReadableByteSt // 2. If controller.[[pendingPullIntos]] is not empty, if (!controller.pending_pull_intos().is_empty()) { // 1. Assert: controller.[[pendingPullIntos]][0]'s reader type is "default". - VERIFY(controller.pending_pull_intos().first().reader_type == ReaderType::Default); + VERIFY(controller.pending_pull_intos().first()->reader_type == ReaderType::Default); // 2. Perform ! ReadableByteStreamControllerShiftPendingPullInto(controller). readable_byte_stream_controller_shift_pending_pull_into(controller); @@ -3376,7 +3374,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_enqueue(ReadableByteSt // 3. For each filledPullInto of filledPullIntos, for (auto& filled_pull_into : filled_pull_intos) { // 1. Perform ! ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], filledPullInto). - readable_byte_stream_controller_commit_pull_into_descriptor(*stream, filled_pull_into); + readable_byte_stream_controller_commit_pull_into_descriptor(*stream, *filled_pull_into); } } // 11. Otherwise, @@ -3468,13 +3466,13 @@ void readable_byte_stream_controller_commit_pull_into_descriptor(ReadableStream& } // https://streams.spec.whatwg.org/#readable-byte-stream-controller-process-pull-into-descriptors-using-queue -SinglyLinkedList readable_byte_stream_controller_process_pull_into_descriptors_using_queue(ReadableByteStreamController& controller) +SinglyLinkedList> readable_byte_stream_controller_process_pull_into_descriptors_using_queue(ReadableByteStreamController& controller) { // 1. Assert: controller.[[closeRequested]] is false. VERIFY(!controller.close_requested()); // 2. Let filledPullIntos be a new empty list. - SinglyLinkedList filled_pull_intos; + SinglyLinkedList> filled_pull_intos; // 3. While controller.[[pendingPullIntos]] is not empty, while (!controller.pending_pull_intos().is_empty()) { @@ -3568,7 +3566,7 @@ WebIDL::ExceptionOr readable_byte_stream_controller_enqueue_cloned_chunk_t } // https://streams.spec.whatwg.org/#readable-byte-stream-controller-shift-pending-pull-into -PullIntoDescriptor readable_byte_stream_controller_shift_pending_pull_into(ReadableByteStreamController& controller) +GC::Ref readable_byte_stream_controller_shift_pending_pull_into(ReadableByteStreamController& controller) { // 1. Assert: controller.[[byobRequest]] is null. VERIFY(!controller.raw_byob_request()); diff --git a/Libraries/LibWeb/Streams/AbstractOperations.h b/Libraries/LibWeb/Streams/AbstractOperations.h index 0e6595ab062..91e5f711646 100644 --- a/Libraries/LibWeb/Streams/AbstractOperations.h +++ b/Libraries/LibWeb/Streams/AbstractOperations.h @@ -1,7 +1,7 @@ /* * Copyright (c) 2022, Linus Groh * Copyright (c) 2023, Matthew Olsson - * Copyright (c) 2023-2024, Shannon Booth + * Copyright (c) 2023-2025, Shannon Booth * Copyright (c) 2023-2024, Kenneth Myhra * * SPDX-License-Identifier: BSD-2-Clause @@ -89,10 +89,10 @@ WebIDL::ExceptionOr> transfer_array_buffer(JS::Realm& r WebIDL::ExceptionOr readable_byte_stream_controller_enqueue_detached_pull_into_queue(ReadableByteStreamController& controller, PullIntoDescriptor& pull_into_descriptor); void readable_byte_stream_controller_commit_pull_into_descriptor(ReadableStream&, PullIntoDescriptor const&); void readable_byte_stream_controller_process_read_requests_using_queue(ReadableByteStreamController& controller); -[[nodiscard]] SinglyLinkedList readable_byte_stream_controller_process_pull_into_descriptors_using_queue(ReadableByteStreamController&); +[[nodiscard]] SinglyLinkedList> readable_byte_stream_controller_process_pull_into_descriptors_using_queue(ReadableByteStreamController&); void readable_byte_stream_controller_enqueue_chunk_to_queue(ReadableByteStreamController& controller, GC::Ref buffer, u32 byte_offset, u32 byte_length); WebIDL::ExceptionOr readable_byte_stream_controller_enqueue_cloned_chunk_to_queue(ReadableByteStreamController& controller, JS::ArrayBuffer& buffer, u64 byte_offset, u64 byte_length); -PullIntoDescriptor readable_byte_stream_controller_shift_pending_pull_into(ReadableByteStreamController& controller); +GC::Ref readable_byte_stream_controller_shift_pending_pull_into(ReadableByteStreamController& controller); void readable_byte_stream_controller_call_pull_if_needed(ReadableByteStreamController&); void readable_byte_stream_controller_clear_algorithms(ReadableByteStreamController&); diff --git a/Libraries/LibWeb/Streams/ReadableByteStreamController.cpp b/Libraries/LibWeb/Streams/ReadableByteStreamController.cpp index 7506430b484..7c8ab3c29cd 100644 --- a/Libraries/LibWeb/Streams/ReadableByteStreamController.cpp +++ b/Libraries/LibWeb/Streams/ReadableByteStreamController.cpp @@ -1,6 +1,6 @@ /* * Copyright (c) 2023, Matthew Olsson - * Copyright (c) 2023-2024, Shannon Booth + * Copyright (c) 2023-2025, Shannon Booth * * SPDX-License-Identifier: BSD-2-Clause */ @@ -17,8 +17,16 @@ namespace Web::Streams { +GC_DEFINE_ALLOCATOR(PullIntoDescriptor); GC_DEFINE_ALLOCATOR(ReadableByteStreamController); +void PullIntoDescriptor::visit_edges(Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(buffer); + visitor.visit(view_constructor); +} + // https://streams.spec.whatwg.org/#rbs-controller-desired-size Optional ReadableByteStreamController::desired_size() const { @@ -149,20 +157,19 @@ void ReadableByteStreamController::pull_steps(GC::Ref read_request) // 3. Let pullIntoDescriptor be a new pull-into descriptor with buffer buffer.[[Value]], buffer byte length autoAllocateChunkSize, byte offset 0, // byte length autoAllocateChunkSize, bytes filled 0, element size 1, view constructor %Uint8Array%, and reader type "default". - PullIntoDescriptor pull_into_descriptor { - .buffer = buffer.release_value(), - .buffer_byte_length = *m_auto_allocate_chunk_size, - .byte_offset = 0, - .byte_length = *m_auto_allocate_chunk_size, - .bytes_filled = 0, - .minimum_fill = 1, - .element_size = 1, - .view_constructor = *realm.intrinsics().uint8_array_constructor(), - .reader_type = ReaderType::Default, - }; + auto pull_into_descriptor = realm.heap().allocate( + buffer.release_value(), + *m_auto_allocate_chunk_size, + 0, + *m_auto_allocate_chunk_size, + 0, + 1, + 1, + *realm.intrinsics().uint8_array_constructor(), + ReaderType::Default); // 4. Append pullIntoDescriptor to this.[[pendingPullIntos]]. - m_pending_pull_intos.append(move(pull_into_descriptor)); + m_pending_pull_intos.append(pull_into_descriptor); } // 6. Perform ! ReadableStreamAddReadRequest(stream, readRequest). @@ -178,7 +185,7 @@ void ReadableByteStreamController::release_steps() // 1. If this.[[pendingPullIntos]] is not empty, if (!m_pending_pull_intos.is_empty()) { // 1. Let firstPendingPullInto be this.[[pendingPullIntos]][0]. - auto first_pending_pull_into = m_pending_pull_intos.first(); + auto& first_pending_pull_into = *m_pending_pull_intos.first(); // 2. Set firstPendingPullInto’s reader type to "none". first_pending_pull_into.reader_type = ReaderType::None; @@ -193,10 +200,8 @@ void ReadableByteStreamController::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); visitor.visit(m_byob_request); - for (auto const& pending_pull_into : m_pending_pull_intos) { - visitor.visit(pending_pull_into.buffer); - visitor.visit(pending_pull_into.view_constructor); - } + for (auto const& pending_pull_into : m_pending_pull_intos) + visitor.visit(pending_pull_into); for (auto const& item : m_queue) visitor.visit(item.buffer); visitor.visit(m_stream); diff --git a/Libraries/LibWeb/Streams/ReadableByteStreamController.h b/Libraries/LibWeb/Streams/ReadableByteStreamController.h index 9a9946abc37..2027db1c32f 100644 --- a/Libraries/LibWeb/Streams/ReadableByteStreamController.h +++ b/Libraries/LibWeb/Streams/ReadableByteStreamController.h @@ -1,5 +1,6 @@ /* * Copyright (c) 2023, Matthew Olsson + * Copyright (c) 2025, Shannon Booth * * SPDX-License-Identifier: BSD-2-Clause */ @@ -20,7 +21,10 @@ enum class ReaderType { }; // https://streams.spec.whatwg.org/#pull-into-descriptor -struct PullIntoDescriptor { +struct PullIntoDescriptor : public GC::Cell { + GC_CELL(PullIntoDescriptor, GC::Cell); + GC_DECLARE_ALLOCATOR(PullIntoDescriptor); + // https://streams.spec.whatwg.org/#pull-into-descriptor-buffer // An ArrayBuffer GC::Ref buffer; @@ -56,6 +60,24 @@ struct PullIntoDescriptor { // https://streams.spec.whatwg.org/#pull-into-descriptor-reader-type // Either "default" or "byob", indicating what type of readable stream reader initiated this request, or "none" if the initiating reader was released ReaderType reader_type; + +protected: + virtual void visit_edges(Cell::Visitor& visitor) override; + +private: + PullIntoDescriptor(GC::Ref buffer, u64 buffer_byte_length, u64 byte_offset, u64 byte_length, u64 bytes_filled, + u64 minimum_fill, u64 element_size, GC::Ref view_constructor, ReaderType reader_type) + : buffer(buffer) + , buffer_byte_length(buffer_byte_length) + , byte_offset(byte_offset) + , byte_length(byte_length) + , bytes_filled(bytes_filled) + , minimum_fill(minimum_fill) + , element_size(element_size) + , view_constructor(view_constructor) + , reader_type(reader_type) + { + } }; // https://streams.spec.whatwg.org/#readable-byte-stream-queue-entry @@ -113,8 +135,8 @@ public: bool pulling() const { return m_pulling; } void set_pulling(bool value) { m_pulling = value; } - SinglyLinkedList& pending_pull_intos() { return m_pending_pull_intos; } - SinglyLinkedList const& pending_pull_intos() const { return m_pending_pull_intos; } + SinglyLinkedList>& pending_pull_intos() { return m_pending_pull_intos; } + SinglyLinkedList> const& pending_pull_intos() const { return m_pending_pull_intos; } SinglyLinkedList& queue() { return m_queue; } @@ -172,7 +194,7 @@ private: // https://streams.spec.whatwg.org/#readablebytestreamcontroller-pendingpullintos // A list of pull-into descriptors - SinglyLinkedList m_pending_pull_intos; + SinglyLinkedList> m_pending_pull_intos; // https://streams.spec.whatwg.org/#readablestreamdefaultcontroller-queue // A list representing the stream’s internal queue of chunks