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

stream: fix isDetachedBuffer validations in ReadableStream #44114

Merged
22 changes: 14 additions & 8 deletions lib/internal/webstreams/readablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ const {
extractHighWaterMark,
extractSizeAlgorithm,
lazyTransfer,
isDetachedBuffer,
isViewedArrayBufferDetached,
isBrandCheck,
resetQueue,
Expand Down Expand Up @@ -658,11 +659,13 @@ class ReadableStreamBYOBRequest {
const viewBuffer = ArrayBufferViewGetBuffer(view);
const viewBufferByteLength = ArrayBufferGetByteLength(viewBuffer);

if (viewByteLength === 0 || viewBufferByteLength === 0) {
throw new ERR_INVALID_STATE.TypeError(
'View ArrayBuffer is zero-length or detached');
if (isDetachedBuffer(viewBuffer)) {
throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached');
}

assert(viewByteLength > 0);
assert(viewBufferByteLength > 0);
LiviaMedeiros marked this conversation as resolved.
Show resolved Hide resolved

readableByteStreamControllerRespond(controller, bytesWritten);
}

Expand Down Expand Up @@ -894,15 +897,19 @@ class ReadableStreamBYOBReader {
],
view));
}

const viewByteLength = ArrayBufferViewGetByteLength(view);
const viewBuffer = ArrayBufferViewGetBuffer(view);
const viewBufferByteLength = ArrayBufferGetByteLength(viewBuffer);

if (viewByteLength === 0 || viewBufferByteLength === 0) {
return PromiseReject(
new ERR_INVALID_STATE.TypeError(
'View ArrayBuffer is zero-length or detached'));
'View or Viewed ArrayBuffer is zero-length',
LiviaMedeiros marked this conversation as resolved.
Show resolved Hide resolved
),
);
}

// Supposed to assert here that the view's buffer is not
// detached, but there's no API available to use to check that.
if (this[kState].stream === undefined) {
Expand Down Expand Up @@ -2298,11 +2305,10 @@ function readableByteStreamControllerEnqueue(
if (pendingPullIntos.length) {
const firstPendingPullInto = pendingPullIntos[0];

const pendingBufferByteLength =
ArrayBufferGetByteLength(firstPendingPullInto.buffer);
if (pendingBufferByteLength === 0) {
if (isDetachedBuffer(firstPendingPullInto.buffer)) {
throw new ERR_INVALID_STATE.TypeError(
'Destination ArrayBuffer is zero-length or detached');
'Destination ArrayBuffer is detached',
);
}

firstPendingPullInto.buffer =
Expand Down
20 changes: 17 additions & 3 deletions lib/internal/webstreams/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ const {
} = internalBinding('buffer');

const {
isArrayBuffer,
isArrayBufferView,
isPromise,
} = require('internal/util/types');

Expand Down Expand Up @@ -129,21 +131,32 @@ function transferArrayBuffer(buffer) {
return res;
}

function isArrayBufferDetached(buffer) {
function isDetachedBuffer(buffer) {
if (!isArrayBuffer(buffer))
throw new ERR_INVALID_ARG_TYPE('buffer', 'ArrayBuffer', buffer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking comment: this change is fine in context of this PR, but it probably should be rewritten in a follow-up.

It would be better if is* functions always returned boolean without throwing. If we want to throw, validate* function is preferable.
Since either true or false won't make sense if provided value is not a buffer, it looks like an acceptable temporal solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it looks better to follow the coding convention.

I updated the util using assert instead, since, as you mentioned, either true or false won't make sense if a provided value is not a buffer. We can guarantee a passing value is a buffer since it's internal util. PTAL.

if (ArrayBufferGetByteLength(buffer) === 0) {
LiviaMedeiros marked this conversation as resolved.
Show resolved Hide resolved
// TODO(daeyeon): Consider using C++ builtin to improve performance.
try {
new Uint8Array(buffer);
} catch {
} catch (error) {
assert(error.name === 'TypeError');
return true;
}
}
return false;
}

function isViewedArrayBufferDetached(view) {
if (!isArrayBufferView(view)) {
throw new ERR_INVALID_ARG_TYPE(
'view',
['Buffer', 'TypedArray', 'DataView'],
view,
);
}
return (
ArrayBufferViewGetByteLength(view) === 0 &&
isArrayBufferDetached(ArrayBufferViewGetBuffer(view))
isDetachedBuffer(ArrayBufferViewGetBuffer(view))
);
}

Expand Down Expand Up @@ -243,6 +256,7 @@ module.exports = {
extractSizeAlgorithm,
lazyTransfer,
isBrandCheck,
isDetachedBuffer,
isPromisePending,
isViewedArrayBufferDetached,
peekQueueValue,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,48 @@ let pass = 0;
reader.read(new Uint8Array([4, 5, 6]));
}

{
const stream = new ReadableStream({
start(c) {
c.enqueue(new Uint8Array([1, 2, 3]));
},
type: 'bytes',
});
const reader = stream.getReader({ mode: 'byob' });
const view = new Uint8Array();
reader
.read(view)
.then(common.mustNotCall())
.catch(
common.mustCall(
common.expectsError({
code: 'ERR_INVALID_STATE',
name: 'TypeError',
}),
),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reader
.read(view)
.then(common.mustNotCall())
.catch(
common.mustCall(
common.expectsError({
code: 'ERR_INVALID_STATE',
name: 'TypeError',
}),
),
);
assert.rejects(
reader.read(view),
{
code: 'ERR_INVALID_STATE',
name: 'TypeError',
}
).then(common.mustCall());

}

{
const stream = new ReadableStream({
start(c) {
c.enqueue(new Uint8Array([1, 2, 3]));
},
type: 'bytes',
});
const reader = stream.getReader({ mode: 'byob' });
const view = new Uint8Array(new ArrayBuffer(10), 0, 0);
reader
.read(view)
.then(common.mustNotCall())
.catch(
common.mustCall(
common.expectsError({
code: 'ERR_INVALID_STATE',
name: 'TypeError',
}),
),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reader
.read(view)
.then(common.mustNotCall())
.catch(
common.mustCall(
common.expectsError({
code: 'ERR_INVALID_STATE',
name: 'TypeError',
}),
),
);
assert.rejects(
reader.read(view),
{
code: 'ERR_INVALID_STATE',
name: 'TypeError',
}
).then(common.mustCall());

}

process.on('exit', () => assert.strictEqual(pass, 2));