-
Notifications
You must be signed in to change notification settings - Fork 161
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
Commit pull-into descriptors after filling from queue #1326
base: main
Are you sure you want to change the base?
Commit pull-into descriptors after filling from queue #1326
Conversation
if (SafeCopyDataBlockBytes(pullIntoDescriptor.buffer, destStart, headOfQueue.buffer, headOfQueue.byteOffset, | ||
bytesToCopy) === false) { | ||
// This should never happen. Please report an issue if it does! https://github.com/whatwg/streams/issues | ||
const e = new TypeError('Invalid buffer'); | ||
ReadableByteStreamControllerError(controller, e); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SafeCopyDataBlockBytes
check should always return true
, so we can't write a test to check if an implementation correctly handles the case where it's false
. That said, this check is our last chance to prevent a potential memory corruption bug.
Is it better to have a bit of untestable code here, or to only keep it as an assertion and hope that we don't break this again in the future? @domenic @saschanaz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is an assertion. Perhaps in the spec it's an assertion with a big <p class="warning">
suggesting that implementers really might want to consider crashing if the assertions somehow gets violated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to an assertion with a big warning.
In Chromium bug #339877167, it was discovered that a user could run JavaScript code synchronously during
ReadableStreamFulfillReadIntoRequest
by patchingObject.prototype.then
, and use this gadget to break some invariants withinReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue
.To prevent this, this PR postpones all calls to
ReadableByteStreamControllerCommitPullIntoDescriptor
until after all pull-into descriptors have been filled up byReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue
. This way, we won't trigger any patchedthen()
method until the stream is in a stable state.Fixes GHSA-p5g2-876g-95h9.
then()
sees correctbyobRequest
web-platform-tests/wpt#48085(See WHATWG Working Mode: Changes for more details.)
Preview | Diff