From 8f17a15a223eb5a74f191f41bcc6aa44867573b0 Mon Sep 17 00:00:00 2001 From: avivkeller Date: Mon, 28 Sep 2026 11:02:46 -0400 Subject: [PATCH] buffer,fs: support immutable arraybuffers Signed-off-by: avivkeller --- lib/buffer.js | 19 +-- lib/fs.js | 11 +- lib/internal/fs/promises.js | 9 +- lib/internal/fs/utils.js | 26 +++- lib/internal/validators.js | 22 ++++ src/node_buffer.cc | 25 ++++ test/parallel/test-buffer-from-immutable.js | 65 ++++++++++ .../test-fs-immutable-arraybuffer-copy.js | 115 ++++++++++++++++++ .../parallel/test-fs-immutable-arraybuffer.js | 100 +++++++++++++++ 9 files changed, 370 insertions(+), 22 deletions(-) create mode 100644 test/parallel/test-buffer-from-immutable.js create mode 100644 test/parallel/test-fs-immutable-arraybuffer-copy.js create mode 100644 test/parallel/test-fs-immutable-arraybuffer.js diff --git a/lib/buffer.js b/lib/buffer.js index d9c64f27cf3e..f4b1a461a084 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -51,7 +51,6 @@ const { TypedArrayPrototypeGetByteOffset, TypedArrayPrototypeGetLength, TypedArrayPrototypeSet, - TypedArrayPrototypeSubarray, Uint8Array, } = primordials; @@ -632,7 +631,13 @@ function fromArrayLike(obj) { if (length > (poolSize - poolOffset)) createPool(); const b = new FastBuffer(allocPool, poolBase + poolOffset, length); - TypedArrayPrototypeSet(b, obj, 0); + if (isUint8Array(obj)) { + // The native copy reads from a source backed by an immutable ArrayBuffer, + // which %TypedArray%.prototype.set() rejects. + _copy(obj, b, 0, 0, length); + } else { + TypedArrayPrototypeSet(b, obj, 0); + } poolOffset += length; alignPool(); return b; @@ -705,7 +710,9 @@ Buffer.concat = function concat(list, length) { for (let i = 0; i < list.length; i++) { const buf = list[i]; const bufLength = TypedArrayPrototypeGetByteLength(buf); - TypedArrayPrototypeSet(buffer, buf, pos); + // The native copy reads from a source backed by an immutable ArrayBuffer, + // which %TypedArray%.prototype.set() rejects. + _copy(buf, buffer, pos, 0, bufLength); pos += bufLength; } @@ -731,13 +738,11 @@ Buffer.concat = function concat(list, length) { const buf = list[i]; const bufLength = TypedArrayPrototypeGetByteLength(buf); if (pos + bufLength > length) { - TypedArrayPrototypeSet(buffer, - TypedArrayPrototypeSubarray(buf, 0, length - pos), - pos); + _copy(buf, buffer, pos, 0, length - pos); pos = length; break; } - TypedArrayPrototypeSet(buffer, buf, pos); + _copy(buf, buffer, pos, 0, bufLength); pos += bufLength; } diff --git a/lib/fs.js b/lib/fs.js index 4436fa2df6e6..66ce5529617f 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -119,6 +119,7 @@ const { stringToSymlinkType, toUnixTimestamp, validateBufferArray, + validateWritableBufferArray, validateCpOptions, validateOffsetLengthRead, validateOffsetLengthWrite, @@ -141,7 +142,7 @@ const { parseFileMode, validateAbortSignal, validateBoolean, - validateBuffer, + validateWritableBuffer, validateEncoding, validateFunction, validateInteger, @@ -886,7 +887,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) { } = params ?? kEmptyObject); } - validateBuffer(buffer); + validateWritableBuffer(buffer); validateFunction(callback, 'cb'); if (offset == null) { @@ -955,7 +956,7 @@ ObjectDefineProperty(read, kCustomPromisifyArgsSymbol, * @returns {number} */ function readSync(fd, buffer, offsetOrOptions, length, position) { - validateBuffer(buffer); + validateWritableBuffer(buffer); let offset = offsetOrOptions; if (arguments.length <= 3 || typeof offsetOrOptions === 'object') { @@ -1023,7 +1024,7 @@ function readv(fd, buffers, position, callback) { } fd = getValidatedFd(fd); - validateBufferArray(buffers); + validateWritableBufferArray(buffers); callback ||= position; validateFunction(callback, 'cb'); @@ -1059,7 +1060,7 @@ ObjectDefineProperty(readv, kCustomPromisifyArgsSymbol, * @returns {number} */ function readvSync(fd, buffers, position) { - validateBufferArray(buffers); + validateWritableBufferArray(buffers); if (typeof position !== 'number') position = null; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index efa981c55e31..a61962f66f6e 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -78,6 +78,7 @@ const { toUnixTimestamp, handleErrorFromBinding: handleSyncErrorFromBinding, validateBufferArray, + validateWritableBufferArray, validateCpOptions, validateOffsetLengthRead, validateOffsetLengthWrite, @@ -94,7 +95,7 @@ const { parseFileMode, validateAbortSignal, validateBoolean, - validateBuffer, + validateWritableBuffer, validateEncoding, validateInteger, validateObject, @@ -1442,10 +1443,10 @@ async function read(handle, bufferOrParams, offset, length, position) { length = buffer.byteLength - offset, position = null, } = bufferOrParams ?? kEmptyObject); - - validateBuffer(buffer); } + validateWritableBuffer(buffer); + if (offset !== null && typeof offset === 'object') { // This is fh.read(buffer, options) ({ @@ -1490,7 +1491,7 @@ async function read(handle, bufferOrParams, offset, length, position) { } async function readv(handle, buffers, position) { - validateBufferArray(buffers); + validateWritableBufferArray(buffers); if (typeof position !== 'number') position = null; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 3aadc19c4732..96c08f02b539 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -58,12 +58,12 @@ const { toPathIfFileURL } = require('internal/url'); const { validateAbortSignal, validateBoolean, - validateBuffer, validateFunction, validateInt32, validateInteger, validateObject, validateUint32, + validateWritableBuffer, } = require('internal/validators'); const pathModule = require('path'); const binding = internalBinding('fs'); @@ -962,6 +962,17 @@ const validateBufferArray = hideStackFrames((buffers, propName = 'buffers') => { return buffers; }); +// Like validateBufferArray(), for buffers that are going to be written to. +const validateWritableBufferArray = hideStackFrames((buffers, propName = 'buffers') => { + validateBufferArray.withoutStackTrace(buffers, propName); + + for (let i = 0; i < buffers.length; i++) { + validateWritableBuffer.withoutStackTrace(buffers[i], `${propName}[${i}]`); + } + + return buffers; +}); + let nonPortableTemplateWarn = true; function warnOnNonPortableTemplate(template) { @@ -1139,15 +1150,17 @@ function getReadFileBufferByteLengthName(options) { const getReadFileBuffer = hideStackFrames((options, size) => { let { buffer } = options; - if (typeof buffer === 'function') { - buffer = options.buffer(size); - validateBuffer.withoutStackTrace(buffer, 'options.buffer()'); - } - if (buffer === undefined) { return undefined; } + if (typeof buffer === 'function') { + buffer = options.buffer(size); + validateWritableBuffer.withoutStackTrace(buffer, 'options.buffer()'); + } else { + validateWritableBuffer.withoutStackTrace(buffer, 'options.buffer'); + } + if (!BufferIsBuffer(buffer)) { buffer = Buffer.from(buffer.buffer, buffer.byteOffset, buffer.byteLength); } @@ -1223,6 +1236,7 @@ module.exports = { Stats: deprecate(Stats, 'fs.Stats constructor is deprecated.', 'DEP0180'), toUnixTimestamp, validateBufferArray, + validateWritableBufferArray, validateCpOptions, validateOffsetLengthRead, validateOffsetLengthWrite, diff --git a/lib/internal/validators.js b/lib/internal/validators.js index f8f1b9383d17..0ec6eb1fec50 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -39,6 +39,7 @@ const { isRegExp, } = require('internal/util/types'); const { signals } = internalBinding('constants').os; +const { isImmutable: isImmutableArrayBufferView } = internalBinding('buffer'); /** * @param {*} value @@ -411,6 +412,26 @@ const validateBuffer = hideStackFrames((buffer, name = 'buffer') => { } }); +/** + * @callback validateWritableBuffer + * @param {*} buffer + * @param {string} [name='buffer'] + * @returns {asserts buffer is ArrayBufferView} + */ + +/** + * Validates an ArrayBufferView that is going to be written to. A view backed + * by an immutable ArrayBuffer is rejected, since its bytes cannot be changed. + * @type {validateWritableBuffer} + */ +const validateWritableBuffer = hideStackFrames((buffer, name = 'buffer') => { + validateBuffer.withoutStackTrace(buffer, name); + if (isImmutableArrayBufferView(buffer)) { + throw new ERR_INVALID_ARG_VALUE( + name, buffer, 'is backed by an immutable ArrayBuffer and cannot be written'); + } +}); + /** * @param {string} data * @param {string} encoding @@ -668,6 +689,7 @@ module.exports = { validateAbortSignalArray, validateBoolean, validateBuffer, + validateWritableBuffer, validateDictionary, validateEncoding, validateFunction, diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 7ade56fd8928..56756e7a8c8f 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -989,6 +989,27 @@ int32_t FastCompare(Local, static CFunction fast_compare(CFunction::Make(FastCompare)); +bool IsImmutableImpl(Local view) { + return view.As()->Buffer()->IsImmutable(); +} + +void SlowIsImmutable(const FunctionCallbackInfo& args) { + CHECK(args[0]->IsArrayBufferView()); + args.GetReturnValue().Set(IsImmutableImpl(args[0])); +} + +bool FastIsImmutable(Local, + Local view, + // NOLINTNEXTLINE(runtime/references) + FastApiCallbackOptions& options) { + TRACK_V8_FAST_API_CALL("buffer.isImmutable"); + HandleScope scope(options.isolate); + + return IsImmutableImpl(view); +} + +static CFunction fast_is_immutable(CFunction::Make(FastIsImmutable)); + // Computes the offset for starting an indexOf or lastIndexOf search. // Returns either a valid offset in [0...], ie inside the Buffer, // or -1 to signal that there is no possible match. @@ -1927,6 +1948,8 @@ void Initialize(Local target, &fast_byte_length_utf8); SetFastMethod(context, target, "copy", SlowCopy, &fast_copy); SetFastMethodNoSideEffect(context, target, "compare", Compare, &fast_compare); + SetFastMethodNoSideEffect( + context, target, "isImmutable", SlowIsImmutable, &fast_is_immutable); SetMethodNoSideEffect(context, target, "compareOffset", CompareOffset); SetMethod(context, target, "fill", Fill); SetMethodNoSideEffect(context, target, "indexOfBuffer", IndexOfBuffer); @@ -2014,6 +2037,8 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(fast_copy); registry->Register(Compare); registry->Register(fast_compare); + registry->Register(SlowIsImmutable); + registry->Register(fast_is_immutable); registry->Register(CompareOffset); registry->Register(Fill); registry->Register(IndexOfBuffer); diff --git a/test/parallel/test-buffer-from-immutable.js b/test/parallel/test-buffer-from-immutable.js new file mode 100644 index 000000000000..d6c9a63d3a60 --- /dev/null +++ b/test/parallel/test-buffer-from-immutable.js @@ -0,0 +1,65 @@ +// Flags: --js-immutable-arraybuffer +'use strict'; + +require('../common'); +const assert = require('assert'); + +function immutable(bytes) { + const buffer = Uint8Array.from(bytes).buffer.transferToImmutable(); + assert.strictEqual(buffer.immutable, true); + return new Uint8Array(buffer); +} + +function check(actual, expected) { + assert(Buffer.isBuffer(actual)); + assert.deepStrictEqual([...actual], expected); + assert.strictEqual(actual.buffer.immutable, false); + actual.fill(0); +} + +{ + const source = immutable([1, 2, 3, 4]); + check(Buffer.from(source), [1, 2, 3, 4]); + check(Buffer.from(source.subarray(1, 3)), [2, 3]); + check(Buffer.from(Buffer.from(source.buffer)), [1, 2, 3, 4]); + assert.deepStrictEqual([...source], [1, 2, 3, 4]); +} + +{ + // Larger than half the pool size, so the copy is not pooled. + const bytes = Array.from({ length: Buffer.poolSize }, (_, i) => i & 0xff); + const source = immutable(bytes); + check(Buffer.from(source), bytes); + check(Buffer.from(source.subarray(1)), bytes.slice(1)); +} + +{ + const source = immutable([1, 2, 3, 4]); + check(Buffer.copyBytesFrom(source), [1, 2, 3, 4]); + check(Buffer.copyBytesFrom(source, 1, 2), [2, 3]); + const wide = new Uint16Array(Uint16Array.from([0x0102, 0x0304]).buffer.transferToImmutable()); + check(Buffer.copyBytesFrom(wide, 1), [...new Uint8Array(Uint16Array.from([0x0304]).buffer)]); +} + +{ + const a = immutable([1, 2]); + const b = Buffer.from([3, 4]); + const c = immutable([5, 6]); + check(Buffer.concat([a, b, c]), [1, 2, 3, 4, 5, 6]); + check(Buffer.concat([a, b, c], 6), [1, 2, 3, 4, 5, 6]); + // The last element is cut short. + check(Buffer.concat([a, b, c], 5), [1, 2, 3, 4, 5]); + // The result is longer than the sum of the elements and gets zero-filled. + check(Buffer.concat([a, c], 5), [1, 2, 5, 6, 0]); + check(Buffer.concat([a.subarray(1), c.subarray(0, 1)]), [2, 5]); + assert.deepStrictEqual([...a], [1, 2]); + assert.deepStrictEqual([...c], [5, 6]); +} + +{ + // The other direction is unchanged: the copy must not write into a view + // backed by an immutable ArrayBuffer. + const target = immutable([9, 9, 9, 9]); + assert.strictEqual(Buffer.from([1, 2, 3, 4]).copy(target), 0); + assert.deepStrictEqual([...target], [9, 9, 9, 9]); +} diff --git a/test/parallel/test-fs-immutable-arraybuffer-copy.js b/test/parallel/test-fs-immutable-arraybuffer-copy.js new file mode 100644 index 000000000000..ce950e470ae7 --- /dev/null +++ b/test/parallel/test-fs-immutable-arraybuffer-copy.js @@ -0,0 +1,115 @@ +// Flags: --js-immutable-arraybuffer +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const { once } = require('node:events'); +const { join } = require('node:path'); +const { test } = require('node:test'); +const tmpdir = require('../common/tmpdir'); + +function immutable(value) { + return Buffer.from(Uint8Array.from(Buffer.from(value)).buffer.transferToImmutable()); +} + +tmpdir.refresh(); +let index = 0; +function makeDirectory() { + const directory = tmpdir.resolve(`case-${index++}`); + fs.mkdirSync(directory); + return directory; +} + +const readdirOptions = { recursive: true, withFileTypes: true }; + +for (const method of ['readdirSync', 'readdir', 'promises.readdir']) { + test(`fs.${method} with a recursive Buffer path`, async () => { + const directory = makeDirectory(); + fs.mkdirSync(join(directory, 'nested')); + fs.writeFileSync(join(directory, 'nested', 'file'), 'contents'); + const path = immutable(directory); + let entries; + if (method === 'readdirSync') { + entries = fs.readdirSync(path, readdirOptions); + } else if (method === 'readdir') { + entries = await new Promise((resolve) => { + fs.readdir(path, readdirOptions, common.mustSucceed(resolve)); + }); + } else { + entries = await fs.promises.readdir(path, readdirOptions); + } + assert.deepStrictEqual(entries.map((entry) => entry.name).sort(), ['file', 'nested']); + const file = entries.find((entry) => entry.name === 'file'); + assert(file.isFile()); + assert.strictEqual(file.parentPath.toString(), join(directory, 'nested')); + assert.strictEqual(path.toString(), directory); + }); +} + +for (const method of ['rm', 'promises.rm']) { + test(`fs.${method} with a recursive Buffer path`, async () => { + const directory = makeDirectory(); + fs.writeFileSync(join(directory, 'file'), 'contents'); + const path = immutable(directory); + if (method === 'rm') { + await new Promise((resolve) => { + fs.rm(path, { recursive: true }, common.mustSucceed(resolve)); + }); + } else { + await fs.promises.rm(path, { recursive: true }); + } + assert.strictEqual(fs.existsSync(directory), false); + assert.strictEqual(path.toString(), directory); + }); +} + +for (const sync of [false, true]) { + test(`fs.Utf8Stream with ${sync ? 'merged synchronous' : 'asynchronous'} buffers`, async () => { + const path = join(makeDirectory(), 'output'); + const stream = new fs.Utf8Stream({ + fd: fs.openSync(path, 'w'), + contentMode: 'buffer', + sync, + // Buffer two chunks before the synchronous write so they must be merged. + minLength: sync ? 8 : 0, + }); + const closed = once(stream, 'close'); + try { + stream.write(immutable('ABCD')); + if (sync) stream.write(immutable('EFGH')); + stream.end(); + await closed; + assert.strictEqual(fs.readFileSync(path, 'utf8'), sync ? 'ABCDEFGH' : 'ABCD'); + } finally { + stream.destroy(); + await closed; + } + }); +} + +test('fs.WriteStream retries a partial writev', async () => { + const output = []; + let calls = 0; + const stream = fs.createWriteStream(null, { + // All descriptor operations are provided by the custom fs implementation. + fd: 123, + fs: { + writev: common.mustCall((fd, buffers, position, callback) => { + const bytes = buffers.flatMap((buffer) => [...buffer]); + const written = calls++ === 0 ? 1 : bytes.length; + output.push(...bytes.slice(0, written)); + process.nextTick(callback, null, written, buffers); + }, 2), + close: common.mustCall((fd, callback) => process.nextTick(callback, null)), + }, + }); + const closed = once(stream, 'close'); + stream.cork(); + stream.write(immutable('ABCD')); + stream.write(immutable('EFGH')); + stream.end(); + await closed; + assert.deepStrictEqual(output, [...Buffer.from('ABCDEFGH')]); + assert.strictEqual(stream.bytesWritten, 8); +}); diff --git a/test/parallel/test-fs-immutable-arraybuffer.js b/test/parallel/test-fs-immutable-arraybuffer.js new file mode 100644 index 000000000000..d4b3efb47258 --- /dev/null +++ b/test/parallel/test-fs-immutable-arraybuffer.js @@ -0,0 +1,100 @@ +// Flags: --js-immutable-arraybuffer +'use strict'; + +require('../common'); +const assert = require('assert'); +const fs = require('fs'); +const { test } = require('node:test'); +const tmpdir = require('../common/tmpdir'); + +tmpdir.refresh(); +const path = tmpdir.resolve('immutable-arraybuffer'); +fs.writeFileSync(path, 'test'); + +function immutable() { + return new Uint8Array(new ArrayBuffer(4).transferToImmutable()); +} + +async function checkRead(read, name = 'buffer') { + const buffer = immutable(); + // Wrapped in an async function so that a synchronous throw and a rejected + // promise are checked the same way. + await assert.rejects(async () => read(buffer), { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: `The ${name.includes('.') ? 'property' : 'argument'} '${name}' ` + + 'is backed by an immutable ArrayBuffer and cannot be written. ' + + 'Received Uint8Array(4) [ 0, 0, 0, 0 ]', + }); + assert.deepStrictEqual([...buffer], [0, 0, 0, 0]); +} + +const fdReads = [ + ['fs.readSync', (fd, buffer) => fs.readSync(fd, buffer, 0, 4, 0)], + ['fs.readSync with options', (fd, buffer) => fs.readSync(fd, buffer, {})], + ['fs.read', (fd, buffer) => new Promise((resolve, reject) => { + fs.read(fd, buffer, 0, 4, 0, (err) => (err ? reject(err) : resolve())); + })], + ['fs.read with options', (fd, buffer) => new Promise((resolve, reject) => { + fs.read(fd, { buffer }, (err) => (err ? reject(err) : resolve())); + })], + ['fs.readvSync', (fd, buffer) => fs.readvSync(fd, [buffer], 0), 'buffers[0]'], + ['fs.readv', (fd, buffer) => new Promise((resolve, reject) => { + fs.readv(fd, [buffer], 0, (err) => (err ? reject(err) : resolve())); + }), 'buffers[0]'], + ['fs.readvSync with a mutable buffer first', + (fd, buffer) => fs.readvSync(fd, [Buffer.alloc(1), buffer], 0), + 'buffers[1]'], +]; + +for (const [name, read, argName] of fdReads) { + test(name, async (t) => { + const fd = fs.openSync(path, 'r'); + t.after(() => fs.closeSync(fd)); + await checkRead((buffer) => read(fd, buffer), argName); + // The file position was not advanced by the rejected reads. + assert.strictEqual(fs.readSync(fd, Buffer.alloc(4)), 4); + }); +} + +const handleReads = [ + ['FileHandle.read', (handle, buffer) => handle.read(buffer, 0, 4, 0)], + ['FileHandle.read with options', (handle, buffer) => handle.read({ buffer })], + ['FileHandle.readv', (handle, buffer) => handle.readv([buffer], 0), 'buffers[0]'], +]; + +for (const [name, read, argName] of handleReads) { + test(name, async (t) => { + const handle = await fs.promises.open(path, 'r'); + t.after(() => handle.close()); + await checkRead((buffer) => read(handle, buffer), argName); + const { bytesRead } = await handle.read(Buffer.alloc(4)); + assert.strictEqual(bytesRead, 4); + }); +} + +const fileReads = [ + ['fs.readFileSync', (options) => fs.readFileSync(path, options)], + ['fs.readFile', (options) => new Promise((resolve, reject) => { + fs.readFile(path, options, (err) => (err ? reject(err) : resolve())); + })], + ['fs.promises.readFile', (options) => fs.promises.readFile(path, options)], +]; + +for (const factory of [false, true]) { + const name = factory ? 'buffer factory' : 'buffer'; + const argName = factory ? 'options.buffer()' : 'options.buffer'; + const options = (buffer) => ({ buffer: factory ? () => buffer : buffer }); + + for (const [method, read] of fileReads) { + test(`${method} with ${name}`, () => { + return checkRead((buffer) => read(options(buffer)), argName); + }); + } + + test(`FileHandle.readFile with ${name}`, async (t) => { + const handle = await fs.promises.open(path, 'r'); + t.after(() => handle.close()); + await checkRead((buffer) => handle.readFile(options(buffer)), argName); + }); +}