From e5e32c096fa3e1885c8f1cc9fb7cb97c8250c63f Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 3 Aug 2022 20:59:24 +0900 Subject: [PATCH 1/9] stream: fix `isDetachedBuffer` validations Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 26 +++++++---------------- lib/internal/webstreams/util.js | 5 +++-- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 5c5c8da724ace9..1aadfb100f1589 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -101,6 +101,7 @@ const { extractHighWaterMark, extractSizeAlgorithm, lazyTransfer, + isDetachedBuffer, isViewedArrayBufferDetached, isBrandCheck, resetQueue, @@ -654,13 +655,8 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } - const viewByteLength = ArrayBufferViewGetByteLength(view); - 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 (isViewedArrayBufferDetached(view)) { + throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } readableByteStreamControllerRespond(controller, bytesWritten); @@ -894,14 +890,9 @@ 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')); + if (isViewedArrayBufferDetached(view)) { + throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } // Supposed to assert here that the view's buffer is not // detached, but there's no API available to use to check that. @@ -2298,11 +2289,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 = diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index ed74a10801f8ff..eef8201c9402ea 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -129,7 +129,7 @@ function transferArrayBuffer(buffer) { return res; } -function isArrayBufferDetached(buffer) { +function isDetachedBuffer(buffer) { if (ArrayBufferGetByteLength(buffer) === 0) { try { new Uint8Array(buffer); @@ -143,7 +143,7 @@ function isArrayBufferDetached(buffer) { function isViewedArrayBufferDetached(view) { return ( ArrayBufferViewGetByteLength(view) === 0 && - isArrayBufferDetached(ArrayBufferViewGetBuffer(view)) + isDetachedBuffer(ArrayBufferViewGetBuffer(view)) ); } @@ -243,6 +243,7 @@ module.exports = { extractSizeAlgorithm, lazyTransfer, isBrandCheck, + isDetachedBuffer, isPromisePending, isViewedArrayBufferDetached, peekQueueValue, From 50c77284299cea657cd328f2f467ca902562318a Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 4 Aug 2022 16:56:58 +0900 Subject: [PATCH 2/9] fixup: check zero and add asserts Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 27 ++++++++++-- ...eadablebytestream-bad-buffers-and-views.js | 44 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 1aadfb100f1589..2b883c1b2ccdca 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -655,10 +655,17 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } - if (isViewedArrayBufferDetached(view)) { + const viewByteLength = ArrayBufferViewGetByteLength(view); + const viewBuffer = ArrayBufferViewGetBuffer(view); + const viewBufferByteLength = ArrayBufferGetByteLength(viewBuffer); + + if (isDetachedBuffer(viewBuffer)) { throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } + assert(viewByteLength > 0); + assert(viewBufferByteLength > 0); + readableByteStreamControllerRespond(controller, bytesWritten); } @@ -891,8 +898,22 @@ class ReadableStreamBYOBReader { view)); } - if (isViewedArrayBufferDetached(view)) { - throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); + 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 or Viewed ArrayBuffer is zero-length', + ), + ); + } + + if (isDetachedBuffer(viewBuffer)) { + return PromiseReject( + new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'), + ); } // Supposed to assert here that the view's buffer is not // detached, but there's no API available to use to check that. diff --git a/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js index 545a0cd2db5128..4f208ae6995864 100644 --- a/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js +++ b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js @@ -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', + }), + ), + ); +} + +{ + 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', + }), + ), + ); +} + process.on('exit', () => assert.strictEqual(pass, 2)); From 6f6ba3e72dceadcbf14e869f24dc2f1ceefe07b3 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 4 Aug 2022 17:13:38 +0900 Subject: [PATCH 3/9] fixup: use isDetachedBuffer only Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 3 +-- lib/internal/webstreams/util.js | 18 ++++-------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 2b883c1b2ccdca..94d57ddf64c677 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -102,7 +102,6 @@ const { extractSizeAlgorithm, lazyTransfer, isDetachedBuffer, - isViewedArrayBufferDetached, isBrandCheck, resetQueue, setPromiseHandled, @@ -684,7 +683,7 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } - if (isViewedArrayBufferDetached(view)) { + if (isDetachedBuffer(ArrayBufferViewGetBuffer(view))) { throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index eef8201c9402ea..b298b730c0f8f8 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -130,23 +130,14 @@ function transferArrayBuffer(buffer) { } function isDetachedBuffer(buffer) { - if (ArrayBufferGetByteLength(buffer) === 0) { - try { - new Uint8Array(buffer); - } catch { - return true; - } + try { + new Uint8Array(buffer); + } catch { + return true; } return false; } -function isViewedArrayBufferDetached(view) { - return ( - ArrayBufferViewGetByteLength(view) === 0 && - isDetachedBuffer(ArrayBufferViewGetBuffer(view)) - ); -} - function dequeueValue(controller) { assert(controller[kState].queue !== undefined); assert(controller[kState].queueTotalSize !== undefined); @@ -245,7 +236,6 @@ module.exports = { isBrandCheck, isDetachedBuffer, isPromisePending, - isViewedArrayBufferDetached, peekQueueValue, resetQueue, setPromiseHandled, From 782c71aaf13f650b0131dde81e2901d8fa89b503 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 4 Aug 2022 20:39:47 +0900 Subject: [PATCH 4/9] revert "fixup: use isDetachedBuffer only" This reverts commit 6f6ba3e72dceadcbf14e869f24dc2f1ceefe07b3. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 3 ++- lib/internal/webstreams/util.js | 18 ++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 94d57ddf64c677..2b883c1b2ccdca 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -102,6 +102,7 @@ const { extractSizeAlgorithm, lazyTransfer, isDetachedBuffer, + isViewedArrayBufferDetached, isBrandCheck, resetQueue, setPromiseHandled, @@ -683,7 +684,7 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } - if (isDetachedBuffer(ArrayBufferViewGetBuffer(view))) { + if (isViewedArrayBufferDetached(view)) { throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index b298b730c0f8f8..eef8201c9402ea 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -130,14 +130,23 @@ function transferArrayBuffer(buffer) { } function isDetachedBuffer(buffer) { - try { - new Uint8Array(buffer); - } catch { - return true; + if (ArrayBufferGetByteLength(buffer) === 0) { + try { + new Uint8Array(buffer); + } catch { + return true; + } } return false; } +function isViewedArrayBufferDetached(view) { + return ( + ArrayBufferViewGetByteLength(view) === 0 && + isDetachedBuffer(ArrayBufferViewGetBuffer(view)) + ); +} + function dequeueValue(controller) { assert(controller[kState].queue !== undefined); assert(controller[kState].queueTotalSize !== undefined); @@ -236,6 +245,7 @@ module.exports = { isBrandCheck, isDetachedBuffer, isPromisePending, + isViewedArrayBufferDetached, peekQueueValue, resetQueue, setPromiseHandled, From e44effab83b8e9e1a80f2edfde16a4a8d8729959 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sun, 7 Aug 2022 00:57:45 +0900 Subject: [PATCH 5/9] fixup: remove redundant isDetachedBuffer Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 2b883c1b2ccdca..dc963e5096f7bf 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -910,11 +910,6 @@ class ReadableStreamBYOBReader { ); } - if (isDetachedBuffer(viewBuffer)) { - return PromiseReject( - new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'), - ); - } // 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) { From 6fd3ea1945472e708e017d2a3b6e22cd5637f105 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sun, 7 Aug 2022 01:00:25 +0900 Subject: [PATCH 6/9] fixup: improve the checker Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/util.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index eef8201c9402ea..85e54d68b8afa2 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -31,6 +31,8 @@ const { } = internalBinding('buffer'); const { + isArrayBuffer, + isArrayBufferView, isPromise, } = require('internal/util/types'); @@ -130,10 +132,14 @@ function transferArrayBuffer(buffer) { } function isDetachedBuffer(buffer) { + if (!isArrayBuffer(buffer)) + throw new ERR_INVALID_ARG_TYPE('buffer', 'ArrayBuffer', buffer); if (ArrayBufferGetByteLength(buffer) === 0) { + // TODO(daeyeon): Consider using C++ builtin to improve performance. try { new Uint8Array(buffer); - } catch { + } catch (error) { + assert(error.name === 'TypeError'); return true; } } @@ -141,6 +147,13 @@ function isDetachedBuffer(buffer) { } function isViewedArrayBufferDetached(view) { + if (!isArrayBufferView(view)) { + throw new ERR_INVALID_ARG_TYPE( + 'view', + ['Buffer', 'TypedArray', 'DataView'], + view, + ); + } return ( ArrayBufferViewGetByteLength(view) === 0 && isDetachedBuffer(ArrayBufferViewGetBuffer(view)) From 0cc34f4d8f4467089da7ddc6a01a5dc7804678eb Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sun, 7 Aug 2022 17:42:15 +0900 Subject: [PATCH 7/9] fixup: change error message properly Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index dc963e5096f7bf..db8e34deadd0a9 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -905,7 +905,7 @@ class ReadableStreamBYOBReader { if (viewByteLength === 0 || viewBufferByteLength === 0) { return PromiseReject( new ERR_INVALID_STATE.TypeError( - 'View or Viewed ArrayBuffer is zero-length', + 'View or Viewed ArrayBuffer is zero-length or detached', ), ); } From fe816cdff06c68be3b17e6b87fc6f41346e7f406 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sun, 7 Aug 2022 17:45:06 +0900 Subject: [PATCH 8/9] fixup: use assert for isDetachedBuffer Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 18 ++++++++---------- lib/internal/webstreams/util.js | 11 ++--------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index db8e34deadd0a9..a25e7adf0bfc0f 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -684,6 +684,14 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } + if (!isArrayBufferView(view)) { + throw new ERR_INVALID_ARG_TYPE( + 'view', + ['Buffer', 'TypedArray', 'DataView'], + view, + ); + } + if (isViewedArrayBufferDetached(view)) { throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } @@ -2507,16 +2515,6 @@ function readableByteStreamControllerRespondWithNewView(controller, view) { const desc = pendingPullIntos[0]; assert(stream[kState].state !== 'errored'); - if (!isArrayBufferView(view)) { - throw new ERR_INVALID_ARG_TYPE( - 'view', - [ - 'Buffer', - 'TypedArray', - 'DataView', - ], - view); - } const viewByteLength = ArrayBufferViewGetByteLength(view); const viewByteOffset = ArrayBufferViewGetByteOffset(view); const viewBuffer = ArrayBufferViewGetBuffer(view); diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index 85e54d68b8afa2..87c86ee07bc211 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -132,8 +132,7 @@ function transferArrayBuffer(buffer) { } function isDetachedBuffer(buffer) { - if (!isArrayBuffer(buffer)) - throw new ERR_INVALID_ARG_TYPE('buffer', 'ArrayBuffer', buffer); + assert(isArrayBuffer(buffer)); if (ArrayBufferGetByteLength(buffer) === 0) { // TODO(daeyeon): Consider using C++ builtin to improve performance. try { @@ -147,13 +146,7 @@ function isDetachedBuffer(buffer) { } function isViewedArrayBufferDetached(view) { - if (!isArrayBufferView(view)) { - throw new ERR_INVALID_ARG_TYPE( - 'view', - ['Buffer', 'TypedArray', 'DataView'], - view, - ); - } + assert(isArrayBufferView(view)); return ( ArrayBufferViewGetByteLength(view) === 0 && isDetachedBuffer(ArrayBufferViewGetBuffer(view)) From 58da0d892bd3e1ac38a811d441a1ad8db4507c32 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 18 Aug 2022 09:27:01 +0900 Subject: [PATCH 9/9] fixup: apply suggestions Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 9 ++--- lib/internal/webstreams/util.js | 4 --- ...eadablebytestream-bad-buffers-and-views.js | 34 +++++++------------ 3 files changed, 14 insertions(+), 33 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index a25e7adf0bfc0f..d663e9a692a811 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -59,6 +59,7 @@ const { } = require('v8'); const { + validateBuffer, validateObject, } = require('internal/validators'); @@ -684,13 +685,7 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } - if (!isArrayBufferView(view)) { - throw new ERR_INVALID_ARG_TYPE( - 'view', - ['Buffer', 'TypedArray', 'DataView'], - view, - ); - } + validateBuffer(view, 'view'); if (isViewedArrayBufferDetached(view)) { throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index 87c86ee07bc211..d8e63b5faaa280 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -31,8 +31,6 @@ const { } = internalBinding('buffer'); const { - isArrayBuffer, - isArrayBufferView, isPromise, } = require('internal/util/types'); @@ -132,7 +130,6 @@ function transferArrayBuffer(buffer) { } function isDetachedBuffer(buffer) { - assert(isArrayBuffer(buffer)); if (ArrayBufferGetByteLength(buffer) === 0) { // TODO(daeyeon): Consider using C++ builtin to improve performance. try { @@ -146,7 +143,6 @@ function isDetachedBuffer(buffer) { } function isViewedArrayBufferDetached(view) { - assert(isArrayBufferView(view)); return ( ArrayBufferViewGetByteLength(view) === 0 && isDetachedBuffer(ArrayBufferViewGetBuffer(view)) diff --git a/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js index 4f208ae6995864..a8f6ffc7535e96 100644 --- a/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js +++ b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js @@ -60,17 +60,12 @@ let pass = 0; }); 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', - }), - ), - ); + assert + .rejects(reader.read(view), { + code: 'ERR_INVALID_STATE', + name: 'TypeError', + }) + .then(common.mustCall()); } { @@ -82,17 +77,12 @@ let pass = 0; }); 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', - }), - ), - ); + assert + .rejects(reader.read(view), { + code: 'ERR_INVALID_STATE', + name: 'TypeError', + }) + .then(common.mustCall()); } process.on('exit', () => assert.strictEqual(pass, 2));