From 3dc3627a4604c694cf046504ca9e2c9ad63eb392 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sun, 17 Jul 2022 02:44:24 +0900 Subject: [PATCH 1/5] stream: improve `respondWithNewView()` This fixes validating an ArrayBufferView given to ReadableStreamBYOBRequest.respondWithNewView() to improve the web streams compatibility. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 12 +++++ src/node_util.cc | 8 ++++ ...eadablebytestream-bad-buffers-and-views.js | 46 +++++++++++++++++++ .../test-whatwg-readablebytestream.js | 4 +- 4 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 0e7d33e9b71eb3..1d96470269154c 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -28,6 +28,8 @@ const { Uint8Array, } = primordials; +const { arrayBufferIsDetached } = internalBinding('util'); + const { codes: { ERR_ILLEGAL_CONSTRUCTOR, @@ -681,6 +683,16 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } + const viewedBuffer = ArrayBufferViewGetBuffer(view); + const viewedBufferByteLength = ArrayBufferGetByteLength(viewedBuffer); + const isDetached = arrayBufferIsDetached(viewedBuffer); + + if (viewedBufferByteLength === 0 && isDetached) { + throw new ERR_INVALID_STATE.TypeError( + 'Viewed ArrayBuffer is detached', + ); + } + readableByteStreamControllerRespondWithNewView(controller, view); } diff --git a/src/node_util.cc b/src/node_util.cc index 5b5dab36f08fbf..c979f5ea2ca882 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -207,6 +207,12 @@ void ArrayBufferViewHasBuffer(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(args[0].As()->HasBuffer()); } +void ArrayBufferIsDetached(const FunctionCallbackInfo& args) { + CHECK(args[0]->IsArrayBuffer()); + v8::Local ab = args[0].As(); + args.GetReturnValue().Set(ab->GetBackingStore()->Data() == nullptr); +} + class WeakReference : public BaseObject { public: WeakReference(Environment* env, Local object, Local target) @@ -334,6 +340,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetExternalValue); registry->Register(Sleep); registry->Register(ArrayBufferViewHasBuffer); + registry->Register(ArrayBufferIsDetached); registry->Register(WeakReference::New); registry->Register(WeakReference::Get); registry->Register(WeakReference::IncRef); @@ -380,6 +387,7 @@ void Initialize(Local target, env->SetMethod(target, "sleep", Sleep); env->SetMethod(target, "arrayBufferViewHasBuffer", ArrayBufferViewHasBuffer); + env->SetMethod(target, "arrayBufferIsDetached", ArrayBufferIsDetached); Local constants = Object::New(env->isolate()); NODE_DEFINE_CONSTANT(constants, ALL_PROPERTIES); NODE_DEFINE_CONSTANT(constants, ONLY_WRITABLE); diff --git a/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js new file mode 100644 index 00000000000000..056aaa3eb85344 --- /dev/null +++ b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); + +{ + // ReadableStream with byte source: respondWithNewView() throws if the + // supplied view's buffer has a different length (in the closed state) + const stream = new ReadableStream({ + pull: common.mustCall(async (c) => { + const view = new Uint8Array(new ArrayBuffer(10), 0, 0); + + c.close(); + + assert.throws(() => { + c.byobRequest.respondWithNewView(view); + }, RangeError); + }), + type: 'bytes', + }); + + const reader = stream.getReader({ mode: 'byob' }); + reader.read(new Uint8Array([4, 5, 6])); +} + +{ + // ReadableStream with byte source: respondWithNewView() throws if the + // supplied view's buffer has been detached (in the closed state) + const stream = new ReadableStream({ + pull: common.mustCall((c) => { + c.close(); + + // Detach it by reading into it + const view = new Uint8Array([1, 2, 3]); + reader.read(view); + + assert.throws(() => { + c.byobRequest.respondWithNewView(view); + }, TypeError); + }), + type: 'bytes', + }); + + const reader = stream.getReader({ mode: 'byob' }); + reader.read(new Uint8Array([4, 5, 6])); +} diff --git a/test/parallel/test-whatwg-readablebytestream.js b/test/parallel/test-whatwg-readablebytestream.js index 6b305a0d8365f2..45b6a34d7af631 100644 --- a/test/parallel/test-whatwg-readablebytestream.js +++ b/test/parallel/test-whatwg-readablebytestream.js @@ -98,9 +98,7 @@ class Source { this.controller.close(); } - assert.throws(() => byobRequest.respondWithNewView({}), { - code: 'ERR_INVALID_ARG_TYPE', - }); + assert.throws(() => byobRequest.respondWithNewView({}), TypeError); byobRequest.respond(bytesRead); From 958ee56bc25d2028b4439c8a50a07f8ef055c9e6 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 21 Jul 2022 21:10:39 +0900 Subject: [PATCH 2/5] fixup: add a counter for passed tests Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- ...eadablebytestream-bad-buffers-and-views.js | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) 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 056aaa3eb85344..545a0cd2db5128 100644 --- a/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js +++ b/test/parallel/test-whatwg-readablebytestream-bad-buffers-and-views.js @@ -3,6 +3,8 @@ const common = require('../common'); const assert = require('node:assert'); +let pass = 0; + { // ReadableStream with byte source: respondWithNewView() throws if the // supplied view's buffer has a different length (in the closed state) @@ -12,9 +14,11 @@ const assert = require('node:assert'); c.close(); - assert.throws(() => { - c.byobRequest.respondWithNewView(view); - }, RangeError); + assert.throws(() => c.byobRequest.respondWithNewView(view), { + code: 'ERR_INVALID_ARG_VALUE', + name: 'RangeError', + }); + pass++; }), type: 'bytes', }); @@ -34,9 +38,11 @@ const assert = require('node:assert'); const view = new Uint8Array([1, 2, 3]); reader.read(view); - assert.throws(() => { - c.byobRequest.respondWithNewView(view); - }, TypeError); + assert.throws(() => c.byobRequest.respondWithNewView(view), { + code: 'ERR_INVALID_STATE', + name: 'TypeError', + }); + pass++; }), type: 'bytes', }); @@ -44,3 +50,5 @@ const assert = require('node:assert'); const reader = stream.getReader({ mode: 'byob' }); reader.read(new Uint8Array([4, 5, 6])); } + +process.on('exit', () => assert.strictEqual(pass, 2)); From 593536fd4ce68e90910aef4c1e46583887bdf550 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 21 Jul 2022 21:11:32 +0900 Subject: [PATCH 3/5] fixup: add `isViewedArrayBufferDetached` in JS Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 11 +++-------- lib/internal/webstreams/util.js | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 1d96470269154c..cb38b00a4b6c86 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -104,6 +104,7 @@ const { extractHighWaterMark, extractSizeAlgorithm, lazyTransfer, + isViewedArrayBufferDetached, isBrandCheck, resetQueue, setPromiseHandled, @@ -683,14 +684,8 @@ class ReadableStreamBYOBRequest { 'This BYOB request has been invalidated'); } - const viewedBuffer = ArrayBufferViewGetBuffer(view); - const viewedBufferByteLength = ArrayBufferGetByteLength(viewedBuffer); - const isDetached = arrayBufferIsDetached(viewedBuffer); - - if (viewedBufferByteLength === 0 && isDetached) { - throw new ERR_INVALID_STATE.TypeError( - 'Viewed ArrayBuffer is detached', - ); + if (isViewedArrayBufferDetached(view)) { + throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached'); } readableByteStreamControllerRespondWithNewView(controller, view); diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js index 524093dc74df66..ed74a10801f8ff 100644 --- a/lib/internal/webstreams/util.js +++ b/lib/internal/webstreams/util.js @@ -14,6 +14,7 @@ const { PromiseReject, ReflectGet, Symbol, + Uint8Array, } = primordials; const { @@ -128,6 +129,24 @@ function transferArrayBuffer(buffer) { return res; } +function isArrayBufferDetached(buffer) { + if (ArrayBufferGetByteLength(buffer) === 0) { + try { + new Uint8Array(buffer); + } catch { + return true; + } + } + return false; +} + +function isViewedArrayBufferDetached(view) { + return ( + ArrayBufferViewGetByteLength(view) === 0 && + isArrayBufferDetached(ArrayBufferViewGetBuffer(view)) + ); +} + function dequeueValue(controller) { assert(controller[kState].queue !== undefined); assert(controller[kState].queueTotalSize !== undefined); @@ -225,6 +244,7 @@ module.exports = { lazyTransfer, isBrandCheck, isPromisePending, + isViewedArrayBufferDetached, peekQueueValue, resetQueue, setPromiseHandled, From 97408ed68e77e4ed29a30916d4edf257a9af0a30 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Thu, 21 Jul 2022 21:15:42 +0900 Subject: [PATCH 4/5] fixup: remove the c++ part Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- lib/internal/webstreams/readablestream.js | 2 -- src/node_util.cc | 8 -------- 2 files changed, 10 deletions(-) diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index cb38b00a4b6c86..724710fdb1749d 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -28,8 +28,6 @@ const { Uint8Array, } = primordials; -const { arrayBufferIsDetached } = internalBinding('util'); - const { codes: { ERR_ILLEGAL_CONSTRUCTOR, diff --git a/src/node_util.cc b/src/node_util.cc index c979f5ea2ca882..5b5dab36f08fbf 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -207,12 +207,6 @@ void ArrayBufferViewHasBuffer(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(args[0].As()->HasBuffer()); } -void ArrayBufferIsDetached(const FunctionCallbackInfo& args) { - CHECK(args[0]->IsArrayBuffer()); - v8::Local ab = args[0].As(); - args.GetReturnValue().Set(ab->GetBackingStore()->Data() == nullptr); -} - class WeakReference : public BaseObject { public: WeakReference(Environment* env, Local object, Local target) @@ -340,7 +334,6 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetExternalValue); registry->Register(Sleep); registry->Register(ArrayBufferViewHasBuffer); - registry->Register(ArrayBufferIsDetached); registry->Register(WeakReference::New); registry->Register(WeakReference::Get); registry->Register(WeakReference::IncRef); @@ -387,7 +380,6 @@ void Initialize(Local target, env->SetMethod(target, "sleep", Sleep); env->SetMethod(target, "arrayBufferViewHasBuffer", ArrayBufferViewHasBuffer); - env->SetMethod(target, "arrayBufferIsDetached", ArrayBufferIsDetached); Local constants = Object::New(env->isolate()); NODE_DEFINE_CONSTANT(constants, ALL_PROPERTIES); NODE_DEFINE_CONSTANT(constants, ONLY_WRITABLE); From ec99eb956011c6a88e1129a4aa463491a386257b Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Fri, 22 Jul 2022 00:52:17 +0900 Subject: [PATCH 5/5] fixup: revert unnecessary test changes Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com --- test/parallel/test-whatwg-readablebytestream.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-whatwg-readablebytestream.js b/test/parallel/test-whatwg-readablebytestream.js index 45b6a34d7af631..6b305a0d8365f2 100644 --- a/test/parallel/test-whatwg-readablebytestream.js +++ b/test/parallel/test-whatwg-readablebytestream.js @@ -98,7 +98,9 @@ class Source { this.controller.close(); } - assert.throws(() => byobRequest.respondWithNewView({}), TypeError); + assert.throws(() => byobRequest.respondWithNewView({}), { + code: 'ERR_INVALID_ARG_TYPE', + }); byobRequest.respond(bytesRead);