Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: OOB access in ReadableStream::Close #22435

Merged
merged 3 commits into from Feb 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions patches/chromium/.patches
Expand Up @@ -95,3 +95,5 @@ never_let_a_non-zero-size_pixel_snap_to_zero_size.patch
fix_hi-dpi_transitions_on_catalina.patch
typemap.patch
allow_restricted_clock_nanosleep_in_linux_sandbox.patch
move_readablestream_requests_onto_the_stack_before_iteration.patch
streams_convert_state_dchecks_to_checks.patch
@@ -0,0 +1,55 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <nornagon@nornagon.net>
Date: Thu, 27 Feb 2020 12:40:02 -0800
Subject: Move ReadableStream requests onto the stack before iteration.

This might be subject to concurrent modification by script.

Cherry-picked from 12310ed05f15fea5fa6824c6a6b5d86f81532e25.

diff --git a/third_party/blink/renderer/core/streams/readable_stream_native.cc b/third_party/blink/renderer/core/streams/readable_stream_native.cc
index 632ea367d6861cc45625d3ff305f21249ccd6db6..9f2f7b8cbef30e8c69cb3d4ca1c5b74d6b0073e4 100644
--- a/third_party/blink/renderer/core/streams/readable_stream_native.cc
+++ b/third_party/blink/renderer/core/streams/readable_stream_native.cc
@@ -708,7 +708,9 @@ class ReadableStreamNative::TeeEngine::PullAlgorithm final
// b. Perform ! ReadableStreamDefaultControllerClose(branch2.
// [[readableStreamController]]).
for (int branch = 0; branch < 2; ++branch) {
- if (!engine_->canceled_[branch]) {
+ if (!engine_->canceled_[branch] &&
+ ReadableStreamDefaultController::CanCloseOrEnqueue(
+ engine_->controller_[branch])) {
ReadableStreamDefaultController::Close(
script_state, engine_->controller_[branch]);
}
@@ -734,7 +736,9 @@ class ReadableStreamNative::TeeEngine::PullAlgorithm final
// ReadableStreamDefaultControllerEnqueue(branch2.
// [[readableStreamController]], value2).
for (int branch = 0; branch < 2; ++branch) {
- if (!engine_->canceled_[branch]) {
+ if (!engine_->canceled_[branch] &&
+ ReadableStreamDefaultController::CanCloseOrEnqueue(
+ engine_->controller_[branch])) {
ReadableStreamDefaultController::Enqueue(script_state,
engine_->controller_[branch],
value, exception_state);
@@ -1532,7 +1536,9 @@ void ReadableStreamNative::Close(ScriptState* script_state,
// 5. If ! IsReadableStreamDefaultReader(reader) is true,
// a. Repeat for each readRequest that is an element of reader.
// [[readRequests]],
- for (StreamPromiseResolver* promise : reader->read_requests_) {
+ HeapDeque<Member<StreamPromiseResolver>> requests;
+ requests.Swap(reader->read_requests_);
+ for (StreamPromiseResolver* promise : requests) {
// i. Resolve readRequest.[[promise]] with !
// ReadableStreamCreateReadResult(undefined, true, reader.
// [[forAuthorCode]]).
@@ -1543,7 +1549,7 @@ void ReadableStreamNative::Close(ScriptState* script_state,
}

// b. Set reader.[[readRequests]] to an empty List.
- reader->read_requests_.clear();
+ // This is not required since we've already called Swap().

// 6. Resolve reader.[[closedPromise]] with undefined.
reader->closed_promise_->ResolveWithUndefined(script_state);
259 changes: 259 additions & 0 deletions patches/chromium/streams_convert_state_dchecks_to_checks.patch
@@ -0,0 +1,259 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <nornagon@nornagon.net>
Date: Thu, 27 Feb 2020 12:53:52 -0800
Subject: Streams: Convert state DCHECKs to CHECKs

For "cheap" checks of state in the streams implementation, use CHECKs
instead of DCHECKs. This will improve robustness against logic errors.

Cherry-picked from 122b074f0354079f3d9044cc14890dcfd2d72918.

diff --git a/third_party/blink/renderer/core/streams/readable_stream_default_controller.cc b/third_party/blink/renderer/core/streams/readable_stream_default_controller.cc
index 653cab070a3e995ea62d1ca5d604d6b674089efe..d9e69a72280f4f098ebe25d99f90806a07ca71f4 100644
--- a/third_party/blink/renderer/core/streams/readable_stream_default_controller.cc
+++ b/third_party/blink/renderer/core/streams/readable_stream_default_controller.cc
@@ -110,7 +110,7 @@ void ReadableStreamDefaultController::Close(

// 2. Assert: ! ReadableStreamDefaultControllerCanCloseOrEnqueue(controller)
// is true.
- DCHECK(CanCloseOrEnqueue(controller));
+ CHECK(CanCloseOrEnqueue(controller));

// 3. Set controller.[[closeRequested]] to true.
controller->is_close_requested_ = true;
@@ -136,7 +136,7 @@ void ReadableStreamDefaultController::Enqueue(

// 2. Assert: ! ReadableStreamDefaultControllerCanCloseOrEnqueue(controller)
// is true.
- DCHECK(CanCloseOrEnqueue(controller));
+ CHECK(CanCloseOrEnqueue(controller));

// 3. If ! IsReadableStreamLocked(stream) is true and !
// ReadableStreamGetNumReadRequests(stream) > 0, perform !
@@ -261,7 +261,7 @@ const char* ReadableStreamDefaultController::EnqueueExceptionMessage(
if (state == ReadableStreamNative::kErrored) {
return "Cannot enqueue a chunk into an errored readable stream";
}
- DCHECK(state == ReadableStreamNative::kClosed);
+ CHECK(state == ReadableStreamNative::kClosed);
return "Cannot enqueue a chunk into a closed readable stream";
}

diff --git a/third_party/blink/renderer/core/streams/readable_stream_native.cc b/third_party/blink/renderer/core/streams/readable_stream_native.cc
index 9f2f7b8cbef30e8c69cb3d4ca1c5b74d6b0073e4..0c5bbb02ccbb2ac7b60acb13117ea43880c621cf 100644
--- a/third_party/blink/renderer/core/streams/readable_stream_native.cc
+++ b/third_party/blink/renderer/core/streams/readable_stream_native.cc
@@ -1333,7 +1333,7 @@ void ReadableStreamNative::Initialize(ReadableStreamNative* stream) {
// initialised correctly.
// https://streams.spec.whatwg.org/#initialize-readable-stream
// 1. Set stream.[[state]] to "readable".
- DCHECK_EQ(stream->state_, kReadable);
+ CHECK_EQ(stream->state_, kReadable);
// 2. Set stream.[[reader]] and stream.[[storedError]] to undefined.
DCHECK(!stream->reader_);
DCHECK(stream->stored_error_.IsEmpty());
@@ -1452,7 +1452,7 @@ StreamPromiseResolver* ReadableStreamNative::AddReadRequest(
DCHECK(stream->reader_);

// 2. Assert: stream.[[state]] is "readable".
- DCHECK_EQ(stream->state_, kReadable);
+ CHECK_EQ(stream->state_, kReadable);

// 3. Let promise be a new promise.
auto* promise = MakeGarbageCollected<StreamPromiseResolver>(script_state);
@@ -1519,7 +1519,7 @@ void ReadableStreamNative::Close(ScriptState* script_state,
ReadableStreamNative* stream) {
// https://streams.spec.whatwg.org/#readable-stream-close
// 1. Assert: stream.[[state]] is "readable".
- DCHECK_EQ(stream->state_, kReadable);
+ CHECK_EQ(stream->state_, kReadable);

// 2. Set stream.[[state]] to "closed".
stream->state_ = kClosed;
@@ -1606,7 +1606,7 @@ void ReadableStreamNative::Error(ScriptState* script_state,
v8::Local<v8::Value> e) {
// https://streams.spec.whatwg.org/#readable-stream-error
// 2. Assert: stream.[[state]] is "readable".
- DCHECK_EQ(stream->state_, kReadable);
+ CHECK_EQ(stream->state_, kReadable);
auto* isolate = script_state->GetIsolate();

// 3. Set stream.[[state]] to "errored".
diff --git a/third_party/blink/renderer/core/streams/transform_stream_native.cc b/third_party/blink/renderer/core/streams/transform_stream_native.cc
index 073efffc0ccad31384a29cd7a0d4863dc22540b2..955e1f21559e8c7a51da0692f05f90e1203023ad 100644
--- a/third_party/blink/renderer/core/streams/transform_stream_native.cc
+++ b/third_party/blink/renderer/core/streams/transform_stream_native.cc
@@ -489,7 +489,7 @@ class TransformStreamNative::DefaultSinkWriteAlgorithm final
}

// 4. Assert: state is "writable".
- DCHECK(writable->IsWritable());
+ CHECK(writable->IsWritable());

// 5. Return ! TransformStreamDefaultControllerPerformTransform(
// controller, chunk).
diff --git a/third_party/blink/renderer/core/streams/writable_stream_default_controller.cc b/third_party/blink/renderer/core/streams/writable_stream_default_controller.cc
index 08652a65c0a142031890a7a0005babf08162f41a..afded82d46da46a156a87be2379c428824b53e66 100644
--- a/third_party/blink/renderer/core/streams/writable_stream_default_controller.cc
+++ b/third_party/blink/renderer/core/streams/writable_stream_default_controller.cc
@@ -146,8 +146,8 @@ void WritableStreamDefaultController::SetUp(
// 16. Upon fulfillment of startPromise
// a. Assert: stream.[[state]] is "writable" or "erroring".
const auto state = stream_->GetState();
- DCHECK(state == WritableStreamNative::kWritable ||
- state == WritableStreamNative::kErroring);
+ CHECK(state == WritableStreamNative::kWritable ||
+ state == WritableStreamNative::kErroring);

// b. Set controller.[[started]] to true.
WritableStreamDefaultController* controller = stream_->Controller();
@@ -178,8 +178,8 @@ void WritableStreamDefaultController::SetUp(
// 17. Upon rejection of startPromise with reason r,
// a. Assert: stream.[[state]] is "writable" or "erroring".
const auto state = stream_->GetState();
- DCHECK(state == WritableStreamNative::kWritable ||
- state == WritableStreamNative::kErroring);
+ CHECK(state == WritableStreamNative::kWritable ||
+ state == WritableStreamNative::kErroring);

// b. Set controller.[[started]] to true.
WritableStreamDefaultController* controller = stream_->Controller();
@@ -578,8 +578,8 @@ void WritableStreamDefaultController::ProcessWrite(
const auto state = stream_->GetState();

// c. Assert: state is "writable" or "erroring".
- DCHECK(state == WritableStreamNative::kWritable ||
- state == WritableStreamNative::kErroring);
+ CHECK(state == WritableStreamNative::kWritable ||
+ state == WritableStreamNative::kErroring);

// d. Perform ! DequeueValue(controller).
controller_->queue_->DequeueValue(script_state->GetIsolate());
diff --git a/third_party/blink/renderer/core/streams/writable_stream_default_writer.cc b/third_party/blink/renderer/core/streams/writable_stream_default_writer.cc
index 0d78ec2429f6be6c38e4f88e8b4d146257791918..13511cda376d642b0fc7036de0f715903106350d 100644
--- a/third_party/blink/renderer/core/streams/writable_stream_default_writer.cc
+++ b/third_party/blink/renderer/core/streams/writable_stream_default_writer.cc
@@ -347,8 +347,8 @@ v8::Local<v8::Promise> WritableStreamDefaultWriter::CloseWithErrorPropagation(
}

// 6. Assert: state is "writable" or "erroring".
- DCHECK(state == WritableStreamNative::kWritable ||
- state == WritableStreamNative::kErroring);
+ CHECK(state == WritableStreamNative::kWritable ||
+ state == WritableStreamNative::kErroring);

// 7. Return ! WritableStreamDefaultWriterClose(writer).
return Close(script_state, writer);
@@ -539,11 +539,11 @@ v8::Local<v8::Promise> WritableStreamDefaultWriter::Close(
}

// 5. Assert: state is "writable" or "erroring".
- DCHECK(state == WritableStreamNative::kWritable ||
- state == WritableStreamNative::kErroring);
+ CHECK(state == WritableStreamNative::kWritable ||
+ state == WritableStreamNative::kErroring);

// 6. Assert: ! WritableStreamCloseQueuedOrInFlight(stream) is false.
- DCHECK(!WritableStreamNative::CloseQueuedOrInFlight(stream));
+ CHECK(!WritableStreamNative::CloseQueuedOrInFlight(stream));

// 7. Let promise be a new promise.
auto* promise = MakeGarbageCollected<StreamPromiseResolver>(script_state);
diff --git a/third_party/blink/renderer/core/streams/writable_stream_native.cc b/third_party/blink/renderer/core/streams/writable_stream_native.cc
index 913ddadae5ac981115397db59ced04e847b9a037..9f272dcd791a32590e931141db9ebb6eda103113 100644
--- a/third_party/blink/renderer/core/streams/writable_stream_native.cc
+++ b/third_party/blink/renderer/core/streams/writable_stream_native.cc
@@ -319,7 +319,7 @@ v8::Local<v8::Promise> WritableStreamNative::Abort(
}

// 4. Assert: state is "writable" or "erroring".
- DCHECK(state == kWritable || state == kErroring);
+ CHECK(state == kWritable || state == kErroring);

// 5. Let wasAlreadyErroring be false.
// 6. If state is "erroring",
@@ -358,7 +358,7 @@ v8::Local<v8::Promise> WritableStreamNative::AddWriteRequest(
DCHECK(IsLocked(stream));

// 2. Assert: stream.[[state]] is "writable".
- DCHECK_EQ(stream->state_, kWritable);
+ CHECK_EQ(stream->state_, kWritable);

// 3. Let promise be a new promise.
auto* promise = MakeGarbageCollected<StreamPromiseResolver>(script_state);
@@ -396,7 +396,7 @@ void WritableStreamNative::DealWithRejection(ScriptState* script_state,
}

// 3. Assert: state is "erroring".
- DCHECK_EQ(state, kErroring);
+ CHECK_EQ(state, kErroring);

// 4. Perform ! WritableStreamFinishErroring(stream).
FinishErroring(script_state, stream);
@@ -410,7 +410,7 @@ void WritableStreamNative::StartErroring(ScriptState* script_state,
DCHECK(stream->stored_error_.IsEmpty());

// 2. Assert: stream.[[state]] is "writable".
- DCHECK_EQ(stream->state_, kWritable);
+ CHECK_EQ(stream->state_, kWritable);

// 3. Let controller be stream.[[writableStreamController]].
WritableStreamDefaultController* controller =
@@ -447,7 +447,7 @@ void WritableStreamNative::FinishErroring(ScriptState* script_state,
WritableStreamNative* stream) {
// https://streams.spec.whatwg.org/#writable-stream-finish-erroring
// 1. Assert: stream.[[state]] is "erroring".
- DCHECK_EQ(stream->state_, kErroring);
+ CHECK_EQ(stream->state_, kErroring);

// 2. Assert: ! WritableStreamHasOperationMarkedInFlight(stream) is false.
DCHECK(!HasOperationMarkedInFlight(stream));
@@ -596,7 +596,7 @@ void WritableStreamNative::FinishInFlightWriteWithError(

// 4. Assert: stream.[[state]] is "writable" or "erroring".
const auto state = stream->state_;
- DCHECK(state == kWritable || state == kErroring);
+ CHECK(state == kWritable || state == kErroring);

// 5. Perform ! WritableStreamDealWithRejection(stream, error).
DealWithRejection(script_state, stream, error);
@@ -618,7 +618,7 @@ void WritableStreamNative::FinishInFlightClose(ScriptState* script_state,
const auto state = stream->state_;

// 5. Assert: stream.[[state]] is "writable" or "erroring".
- DCHECK(state == kWritable || state == kErroring);
+ CHECK(state == kWritable || state == kErroring);

// 6. If state is "erroring",
if (state == kErroring) {
@@ -672,7 +672,7 @@ void WritableStreamNative::FinishInFlightCloseWithError(

// 4. Assert: stream.[[state]] is "writable" or "erroring".
const auto state = stream->state_;
- DCHECK(state == kWritable || state == kErroring);
+ CHECK(state == kWritable || state == kErroring);

// 5. If stream.[[pendingAbortRequest]] is not undefined,
if (stream->pending_abort_request_) {
@@ -728,10 +728,10 @@ void WritableStreamNative::UpdateBackpressure(ScriptState* script_state,
bool backpressure) {
// https://streams.spec.whatwg.org/#writable-stream-update-backpressure
// 1. Assert: stream.[[state]] is "writable".
- DCHECK_EQ(stream->state_, kWritable);
+ CHECK_EQ(stream->state_, kWritable);

// 2. Assert: ! WritableStreamCloseQueuedOrInFlight(stream) is false.
- DCHECK(!CloseQueuedOrInFlight(stream));
+ CHECK(!CloseQueuedOrInFlight(stream));

// 3. Let writer be stream.[[writer]].
WritableStreamDefaultWriter* writer = stream->writer_;
@@ -803,7 +803,7 @@ void WritableStreamNative::RejectCloseAndClosedPromiseIfNeeded(
WritableStreamNative* stream) {
// https://streams.spec.whatwg.org/#writable-stream-reject-close-and-closed-promise-if-needed
// // 1. Assert: stream.[[state]] is "errored".
- DCHECK_EQ(stream->state_, kErrored);
+ CHECK_EQ(stream->state_, kErrored);

auto* isolate = script_state->GetIsolate();