Skip to content

Commit

Permalink
Commit pull-into descriptors after filling from queue
Browse files Browse the repository at this point in the history 
In https://issues.chromium.org/issues/339877167, it was discovered that a user could run JavaScript code *synchronously* during ReadableStreamFulfillReadIntoRequest by patching Object.prototype.then, and use this gadget to break some invariants within ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue.

To prevent this, we now postpone all calls to ReadableByteStreamControllerCommitPullIntoDescriptor until *after* all pull-into descriptors have been filled up by ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue. This way, we won't trigger any patched then() method until the stream is in a stable state.

Fixes GHSA-p5g2-876g-95h9.
  • Loading branch information
@MattiasBuelens
MattiasBuelens authored Nov 29, 2024
1 parent 86d07e9 commit 1de47f0
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 23 deletions.
63 changes: 54 additions & 9 deletions index.bs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3348,7 +3348,12 @@ The following abstract operations support the implementation of the
1. Otherwise, if ! [$ReadableStreamHasBYOBReader$](|stream|) is true,
1. Perform ! [$ReadableByteStreamControllerEnqueueChunkToQueue$](|controller|,
|transferredBuffer|, |byteOffset|, |byteLength|).
1. Perform ! [$ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue$](|controller|).
1. Let |filledPullIntos| be the result of performing
! [$ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue$](|controller|).
1. [=list/For each=] |filledPullInto| of |filledPullIntos|,
1. Perform !
[$ReadableByteStreamControllerCommitPullIntoDescriptor$](|controller|.[=ReadableByteStreamController/[[stream]]=],
|filledPullInto|).
1. Otherwise,
1. Assert: ! [$IsReadableStreamLocked$](|stream|) is false.
1. Perform ! [$ReadableByteStreamControllerEnqueueChunkToQueue$](|controller|,
Expand Down Expand Up @@ -3433,6 +3438,7 @@ The following abstract operations support the implementation of the
|maxBytesToCopy|.
1. Let |totalBytesToCopyRemaining| be |maxBytesToCopy|.
1. Let |ready| be false.
1. Assert: ! [$IsDetachedBuffer$](|pullIntoDescriptor|'s [=pull-into descriptor/buffer=]) is false.
1. Assert: |pullIntoDescriptor|'s [=pull-into descriptor/bytes filled=] < |pullIntoDescriptor|'s
[=pull-into descriptor/minimum fill=].
1. Let |remainderBytes| be the remainder after dividing |maxBytesFilled| by |pullIntoDescriptor|'s
Expand All @@ -3452,6 +3458,14 @@ The following abstract operations support the implementation of the
queue entry/byte length=]).
1. Let |destStart| be |pullIntoDescriptor|'s [=pull-into descriptor/byte offset=] +
|pullIntoDescriptor|'s [=pull-into descriptor/bytes filled=].
1. Assert: ! [$CanCopyDataBlockBytes$](|pullIntoDescriptor|'s [=pull-into descriptor/buffer=],
|destStart|, |headOfQueue|'s [=readable byte stream queue entry/buffer=],
|headOfQueue|'s [=readable byte stream queue entry/byte offset=], |bytesToCopy|) is true.
<p class="warning">If this assertion were to fail (due to a bug in this specification or
its implementation), then the next step may read from or write to potentially invalid memory.
The user agent should always check this assertion, and stop in an [=implementation-defined=]
manner if it fails (e.g. by crashing the process, or by
<a abstract-op lt="ReadableByteStreamControllerError">erroring the stream</a>).
1. Perform ! [$CopyDataBlockBytes$](|pullIntoDescriptor|'s [=pull-into
descriptor/buffer=].\[[ArrayBufferData]], |destStart|,
|headOfQueue|'s [=readable byte stream queue entry/buffer=].\[[ArrayBufferData]],
Expand Down Expand Up @@ -3561,17 +3575,17 @@ The following abstract operations support the implementation of the
performs the following steps:

1. Assert: |controller|.[=ReadableByteStreamController/[[closeRequested]]=] is false.
1. Let |filledPullIntos| be a new empty [=list=].
1. [=While=] |controller|.[=ReadableByteStreamController/[[pendingPullIntos]]=] is not
[=list/is empty|empty=],
1. If |controller|.[=ReadableByteStreamController/[[queueTotalSize]]=] is 0, return.
1. If |controller|.[=ReadableByteStreamController/[[queueTotalSize]]=] is 0, then [=break=].
1. Let |pullIntoDescriptor| be
|controller|.[=ReadableByteStreamController/[[pendingPullIntos]]=][0].
1. If ! [$ReadableByteStreamControllerFillPullIntoDescriptorFromQueue$](|controller|,
|pullIntoDescriptor|) is true,
1. Perform ! [$ReadableByteStreamControllerShiftPendingPullInto$](|controller|).
1. Perform !
[$ReadableByteStreamControllerCommitPullIntoDescriptor$](|controller|.[=ReadableByteStreamController/[[stream]]=],
|pullIntoDescriptor|).
1. [=list/Append=] |pullIntoDescriptor| to |filledPullIntos|.
1. Return |filledPullIntos|.
</div>

<div algorithm>
Expand Down Expand Up @@ -3701,11 +3715,16 @@ The following abstract operations support the implementation of the
perform ! [$ReadableByteStreamControllerShiftPendingPullInto$](|controller|).
1. Let |stream| be |controller|.[=ReadableByteStreamController/[[stream]]=].
1. If ! [$ReadableStreamHasBYOBReader$](|stream|) is true,
1. [=While=] ! [$ReadableStreamGetNumReadIntoRequests$](|stream|) > 0,
1. Let |filledPullIntos| be a new empty [=list=].
1. Let |i| be 0.
1. [=While=] |i| < ! [$ReadableStreamGetNumReadIntoRequests$](|stream|),
1. Let |pullIntoDescriptor| be !
[$ReadableByteStreamControllerShiftPendingPullInto$](|controller|).
1. [=list/Append=] |pullIntoDescriptor| to |filledPullIntos|.
1. Set |i| to |i| + 1.
1. [=list/For each=] |filledPullInto| of |filledPullIntos|,
1. Perform ! [$ReadableByteStreamControllerCommitPullIntoDescriptor$](|stream|,
|pullIntoDescriptor|).
|filledPullInto|).
</div>

<div algorithm>
Expand All @@ -3720,7 +3739,12 @@ The following abstract operations support the implementation of the
1. If |pullIntoDescriptor|'s [=pull-into descriptor/reader type=] is "`none`",
1. Perform ? [$ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue$](|controller|,
|pullIntoDescriptor|).
1. Perform ! [$ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue$](|controller|).
1. Let |filledPullIntos| be the result of performing
! [$ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue$](|controller|).
1. [=list/For each=] |filledPullInto| of |filledPullIntos|,
1. Perform !
[$ReadableByteStreamControllerCommitPullIntoDescriptor$](|controller|.[=ReadableByteStreamController/[[stream]]=],
|filledPullInto|).
1. Return.
1. If |pullIntoDescriptor|'s [=pull-into descriptor/bytes filled=] &lt; |pullIntoDescriptor|'s
[=pull-into descriptor/minimum fill=], return.
Expand All @@ -3738,10 +3762,15 @@ The following abstract operations support the implementation of the
|remainderSize|).
1. Set |pullIntoDescriptor|'s [=pull-into descriptor/bytes filled=] to |pullIntoDescriptor|'s
[=pull-into descriptor/bytes filled=] − |remainderSize|.
1. Let |filledPullIntos| be the result of performing
! [$ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue$](|controller|).
1. Perform !
[$ReadableByteStreamControllerCommitPullIntoDescriptor$](|controller|.[=ReadableByteStreamController/[[stream]]=],
|pullIntoDescriptor|).
1. Perform ! [$ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue$](|controller|).
1. [=list/For each=] |filledPullInto| of |filledPullIntos|,
1. Perform !
[$ReadableByteStreamControllerCommitPullIntoDescriptor$](|controller|.[=ReadableByteStreamController/[[stream]]=],
|filledPullInto|).
</div>

<div algorithm>
Expand Down Expand Up @@ -6848,6 +6877,22 @@ The following abstract operations are a grab-bag of utilities.
1. Return ? [$StructuredDeserialize$](|serialized|, [=the current Realm=]).
</div>

<div algorithm>
<dfn abstract-op lt="CanCopyDataBlockBytes">CanCopyDataBlockBytes(|toBuffer|, |toIndex|,
|fromBuffer|, |fromIndex|, |count|)</dfn> performs the following steps:

1. Assert: |toBuffer| [=is an Object=].
1. Assert: |toBuffer| has an \[[ArrayBufferData]] internal slot.
1. Assert: |fromBuffer| [=is an Object=].
1. Assert: |fromBuffer| has an \[[ArrayBufferData]] internal slot.
1. If |toBuffer| is |fromBuffer|, return false.
1. If ! [$IsDetachedBuffer$](|toBuffer|) is true, return false.
1. If ! [$IsDetachedBuffer$](|fromBuffer|) is true, return false.
1. If |toIndex| + |count| > |toBuffer|.\[[ArrayBufferByteLength]], return false.
1. If |fromIndex| + |count| > |fromBuffer|.\[[ArrayBufferByteLength]], return false.
1. Return true.
</div>

<h2 id="other-specs">Using streams in other specifications</h2>

Much of this standard concerns itself with the internal machinery of streams. Other specifications
Expand Down
20 changes: 20 additions & 0 deletions reference-implementation/lib/abstract-ops/miscellaneous.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict';
const { IsDetachedBuffer } = require('./ecmascript');

exports.IsNonNegativeNumber = v => {
if (typeof v !== 'number') {
Expand All @@ -20,3 +21,22 @@ exports.CloneAsUint8Array = O => {
const buffer = O.buffer.slice(O.byteOffset, O.byteOffset + O.byteLength);
return new Uint8Array(buffer);
};

exports.CanCopyDataBlockBytes = (toBuffer, toIndex, fromBuffer, fromIndex, count) => {
if (toBuffer === fromBuffer) {
return false;
}
if (IsDetachedBuffer(toBuffer) === true) {
return false;
}
if (IsDetachedBuffer(fromBuffer) === true) {
return false;
}
if (toIndex + count > toBuffer.byteLength) {
return false;
}
if (fromIndex + count > fromBuffer.byteLength) {
return false;
}
return true;
};
44 changes: 31 additions & 13 deletions reference-implementation/lib/abstract-ops/readable-streams.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const { promiseResolvedWith, promiseRejectedWith, newPromise, resolvePromise, re
require('../helpers/webidl.js');
const { CanTransferArrayBuffer, Call, CopyDataBlockBytes, CreateArrayFromList, GetIterator, GetMethod, IsDetachedBuffer,
IteratorComplete, IteratorNext, IteratorValue, TransferArrayBuffer, typeIsObject } = require('./ecmascript.js');
const { CloneAsUint8Array, IsNonNegativeNumber } = require('./miscellaneous.js');
const { CanCopyDataBlockBytes, CloneAsUint8Array, IsNonNegativeNumber } = require('./miscellaneous.js');
const { EnqueueValueWithSize, ResetQueue } = require('./queue-with-sizes.js');
const { AcquireWritableStreamDefaultWriter, IsWritableStreamLocked, WritableStreamAbort,
WritableStreamDefaultWriterCloseWithErrorPropagation, WritableStreamDefaultWriterRelease,
Expand Down Expand Up @@ -1360,7 +1360,10 @@ function ReadableByteStreamControllerEnqueue(controller, chunk) {
} else if (ReadableStreamHasBYOBReader(stream) === true) {
// TODO: Ideally in this branch detaching should happen only if the buffer is not consumed fully.
ReadableByteStreamControllerEnqueueChunkToQueue(controller, transferredBuffer, byteOffset, byteLength);
ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);
const filledPullIntos = ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);
for (const filledPullInto of filledPullIntos) {
ReadableByteStreamControllerCommitPullIntoDescriptor(controller._stream, filledPullInto);
}
} else {
assert(IsReadableStreamLocked(stream) === false);
ReadableByteStreamControllerEnqueueChunkToQueue(controller, transferredBuffer, byteOffset, byteLength);
Expand Down Expand Up @@ -1425,6 +1428,7 @@ function ReadableByteStreamControllerFillPullIntoDescriptorFromQueue(controller,

let totalBytesToCopyRemaining = maxBytesToCopy;
let ready = false;
assert(!IsDetachedBuffer(pullIntoDescriptor.buffer));
assert(pullIntoDescriptor.bytesFilled < pullIntoDescriptor.minimumFill);
const remainderBytes = maxBytesFilled % pullIntoDescriptor.elementSize;
const maxAlignedBytes = maxBytesFilled - remainderBytes;
Expand All @@ -1443,6 +1447,9 @@ function ReadableByteStreamControllerFillPullIntoDescriptorFromQueue(controller,
const bytesToCopy = Math.min(totalBytesToCopyRemaining, headOfQueue.byteLength);

const destStart = pullIntoDescriptor.byteOffset + pullIntoDescriptor.bytesFilled;

assert(CanCopyDataBlockBytes(pullIntoDescriptor.buffer, destStart, headOfQueue.buffer, headOfQueue.byteOffset,
bytesToCopy));
CopyDataBlockBytes(pullIntoDescriptor.buffer, destStart, headOfQueue.buffer, headOfQueue.byteOffset, bytesToCopy);

if (headOfQueue.byteLength === bytesToCopy) {
Expand Down Expand Up @@ -1532,23 +1539,22 @@ function ReadableByteStreamControllerInvalidateBYOBRequest(controller) {
function ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller) {
assert(controller._closeRequested === false);

const filledPullIntos = [];
while (controller._pendingPullIntos.length > 0) {
if (controller._queueTotalSize === 0) {
return;
break;
}

const pullIntoDescriptor = controller._pendingPullIntos[0];
assert(pullIntoDescriptor.readerType !== 'none');

if (ReadableByteStreamControllerFillPullIntoDescriptorFromQueue(controller, pullIntoDescriptor) === true) {
ReadableByteStreamControllerShiftPendingPullInto(controller);

ReadableByteStreamControllerCommitPullIntoDescriptor(
controller._stream,
pullIntoDescriptor
);
filledPullIntos.push(pullIntoDescriptor);
}
}

return filledPullIntos;
}

function ReadableByteStreamControllerProcessReadRequestsUsingQueue(controller) {
Expand Down Expand Up @@ -1673,9 +1679,15 @@ function ReadableByteStreamControllerRespondInClosedState(controller, firstDescr

const stream = controller._stream;
if (ReadableStreamHasBYOBReader(stream) === true) {
while (ReadableStreamGetNumReadIntoRequests(stream) > 0) {
const filledPullIntos = [];
let i = 0;
while (i < ReadableStreamGetNumReadIntoRequests(stream)) {
const pullIntoDescriptor = ReadableByteStreamControllerShiftPendingPullInto(controller);
ReadableByteStreamControllerCommitPullIntoDescriptor(stream, pullIntoDescriptor);
filledPullIntos.push(pullIntoDescriptor);
++i;
}
for (const filledPullInto of filledPullIntos) {
ReadableByteStreamControllerCommitPullIntoDescriptor(stream, filledPullInto);
}
}
}
Expand All @@ -1687,7 +1699,10 @@ function ReadableByteStreamControllerRespondInReadableState(controller, bytesWri

if (pullIntoDescriptor.readerType === 'none') {
ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue(controller, pullIntoDescriptor);
ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);
const filledPullIntos = ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);
for (const filledPullInto of filledPullIntos) {
ReadableByteStreamControllerCommitPullIntoDescriptor(controller._stream, filledPullInto);
}
return;
}

Expand All @@ -1711,9 +1726,12 @@ function ReadableByteStreamControllerRespondInReadableState(controller, bytesWri
}

pullIntoDescriptor.bytesFilled -= remainderSize;
ReadableByteStreamControllerCommitPullIntoDescriptor(controller._stream, pullIntoDescriptor);
const filledPullIntos = ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);

ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);
ReadableByteStreamControllerCommitPullIntoDescriptor(controller._stream, pullIntoDescriptor);
for (const filledPullInto of filledPullIntos) {
ReadableByteStreamControllerCommitPullIntoDescriptor(controller._stream, filledPullInto);
}
}

function ReadableByteStreamControllerRespondInternal(controller, bytesWritten) {
Expand Down
2 changes: 1 addition & 1 deletion reference-implementation/web-platform-tests
Submodule web-platform-tests updated 11023 files

0 comments on commit 1de47f0

Please sign in to comment.