From 1f2d48cd975b719f0248e471f3b503cb01398dfb Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Wed, 14 Aug 2024 09:42:31 -0700 Subject: [PATCH] fix(node/fs): node:fs.read and write should accept typed arrays other than Uint8Array (#25030) Part of #25028. Our underlying read/write operations in `io` assume the buffer is a Uint8Array, but we were passing in other typed arrays (in the case above it was `Int8Array`). --- ext/node/polyfills/_fs/_fs_common.ts | 2 +- ext/node/polyfills/_fs/_fs_read.ts | 26 ++++++++++------ ext/node/polyfills/_fs/_fs_write.mjs | 9 ++---- ext/node/polyfills/internal/fs/utils.mjs | 10 ++++++ tests/unit_node/_fs/_fs_read_test.ts | 39 ++++++++++++++++++++++-- tests/unit_node/_fs/_fs_write_test.ts | 39 ++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 19 deletions(-) diff --git a/ext/node/polyfills/_fs/_fs_common.ts b/ext/node/polyfills/_fs/_fs_common.ts index 069f9a3a11..ac0bf5a551 100644 --- a/ext/node/polyfills/_fs/_fs_common.ts +++ b/ext/node/polyfills/_fs/_fs_common.ts @@ -38,7 +38,7 @@ export type BinaryOptionsArgument = export type FileOptionsArgument = Encodings | FileOptions; export type ReadOptions = { - buffer: Buffer | Uint8Array; + buffer: Buffer | ArrayBufferView; offset: number; length: number; position: number | null; diff --git a/ext/node/polyfills/_fs/_fs_read.ts b/ext/node/polyfills/_fs/_fs_read.ts index e25f41e764..7b3d9fccdb 100644 --- a/ext/node/polyfills/_fs/_fs_read.ts +++ b/ext/node/polyfills/_fs/_fs_read.ts @@ -9,6 +9,7 @@ import * as io from "ext:deno_io/12_io.js"; import * as fs from "ext:deno_fs/30_fs.js"; import { ReadOptions } from "ext:deno_node/_fs/_fs_common.ts"; import { + arrayBufferViewToUint8Array, validateOffsetLengthRead, validatePosition, } from "ext:deno_node/internal/fs/utils.mjs"; @@ -16,6 +17,7 @@ import { validateBuffer, validateInteger, } from "ext:deno_node/internal/validators.mjs"; +import { isArrayBufferView } from "ext:deno_node/internal/util/types.ts"; type readSyncOptions = { offset: number; @@ -38,7 +40,7 @@ export function read( ): void; export function read( fd: number, - buffer: Buffer | Uint8Array, + buffer: ArrayBufferView, offset: number, length: number, position: number | null, @@ -46,7 +48,7 @@ export function read( ): void; export function read( fd: number, - optOrBufferOrCb?: Buffer | Uint8Array | ReadOptions | Callback, + optOrBufferOrCb?: ArrayBufferView | ReadOptions | Callback, offsetOrCallback?: number | Callback, length?: number, position?: number | null, @@ -75,9 +77,9 @@ export function read( } if ( - optOrBufferOrCb instanceof Buffer || optOrBufferOrCb instanceof Uint8Array + isArrayBufferView(optOrBufferOrCb) ) { - buffer = optOrBufferOrCb; + buffer = arrayBufferViewToUint8Array(optOrBufferOrCb); } else if (typeof optOrBufferOrCb === "function") { offset = 0; buffer = Buffer.alloc(16384); @@ -86,7 +88,7 @@ export function read( } else { const opt = optOrBufferOrCb as ReadOptions; if ( - !(opt.buffer instanceof Buffer) && !(opt.buffer instanceof Uint8Array) + !isArrayBufferView(opt.buffer) ) { throw new ERR_INVALID_ARG_TYPE("buffer", [ "Buffer", @@ -94,8 +96,12 @@ export function read( "DataView", ], optOrBufferOrCb); } + if (opt.buffer === undefined) { + buffer = Buffer.alloc(16384); + } else { + buffer = arrayBufferViewToUint8Array(opt.buffer); + } offset = opt.offset ?? 0; - buffer = opt.buffer ?? Buffer.alloc(16384); length = opt.length ?? buffer.byteLength; position = opt.position ?? null; } @@ -131,19 +137,19 @@ export function read( export function readSync( fd: number, - buffer: Buffer | Uint8Array, + buffer: ArrayBufferView, offset: number, length: number, position: number | null, ): number; export function readSync( fd: number, - buffer: Buffer | Uint8Array, + buffer: ArrayBufferView, opt: readSyncOptions, ): number; export function readSync( fd: number, - buffer: Buffer | Uint8Array, + buffer: ArrayBufferView, offsetOrOpt?: number | readSyncOptions, length?: number, position?: number | null, @@ -156,6 +162,8 @@ export function readSync( validateBuffer(buffer); + buffer = arrayBufferViewToUint8Array(buffer); + if (length == null) { length = 0; } diff --git a/ext/node/polyfills/_fs/_fs_write.mjs b/ext/node/polyfills/_fs/_fs_write.mjs index c0ae129d37..b4b133222f 100644 --- a/ext/node/polyfills/_fs/_fs_write.mjs +++ b/ext/node/polyfills/_fs/_fs_write.mjs @@ -12,6 +12,7 @@ import { import * as io from "ext:deno_io/12_io.js"; import * as fs from "ext:deno_fs/30_fs.js"; import { + arrayBufferViewToUint8Array, getValidatedFd, validateOffsetLengthWrite, validateStringAfterArrayBufferView, @@ -23,9 +24,7 @@ export function writeSync(fd, buffer, offset, length, position) { fd = getValidatedFd(fd); const innerWriteSync = (fd, buffer, offset, length, position) => { - if (buffer instanceof DataView) { - buffer = new Uint8Array(buffer.buffer); - } + buffer = arrayBufferViewToUint8Array(buffer); if (typeof position === "number") { fs.seekSync(fd, position, io.SeekMode.Start); } @@ -69,9 +68,7 @@ export function write(fd, buffer, offset, length, position, callback) { fd = getValidatedFd(fd); const innerWrite = async (fd, buffer, offset, length, position) => { - if (buffer instanceof DataView) { - buffer = new Uint8Array(buffer.buffer); - } + buffer = arrayBufferViewToUint8Array(buffer); if (typeof position === "number") { await fs.seek(fd, position, io.SeekMode.Start); } diff --git a/ext/node/polyfills/internal/fs/utils.mjs b/ext/node/polyfills/internal/fs/utils.mjs index 9f3322baee..53184b9c30 100644 --- a/ext/node/polyfills/internal/fs/utils.mjs +++ b/ext/node/polyfills/internal/fs/utils.mjs @@ -986,6 +986,16 @@ export const validatePosition = hideStackFrames((position) => { } }); +/** @type {(buffer: ArrayBufferView) => Uint8Array} */ +export const arrayBufferViewToUint8Array = hideStackFrames( + (buffer) => { + if (!(buffer instanceof Uint8Array)) { + return new Uint8Array(buffer.buffer); + } + return buffer; + }, +); + export const realpathCacheKey = Symbol("realpathCacheKey"); export const constants = { kIoMaxLength, diff --git a/tests/unit_node/_fs/_fs_read_test.ts b/tests/unit_node/_fs/_fs_read_test.ts index 42e8fed731..288e4a57c5 100644 --- a/tests/unit_node/_fs/_fs_read_test.ts +++ b/tests/unit_node/_fs/_fs_read_test.ts @@ -1,4 +1,5 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +/// import { assertEquals, assertFalse, @@ -13,21 +14,21 @@ import { closeSync } from "node:fs"; async function readTest( testData: string, - buffer: Buffer, + buffer: NodeJS.ArrayBufferView, offset: number, length: number, position: number | null = null, expected: ( fd: number, bytesRead: number | null, - data: Buffer | undefined, + data: ArrayBufferView | undefined, ) => void, ) { let fd1 = 0; await new Promise<{ fd: number; bytesRead: number | null; - data: Buffer | undefined; + data: ArrayBufferView | undefined; }>((resolve, reject) => { open(testData, "r", (err, fd) => { if (err) reject(err); @@ -320,3 +321,35 @@ Deno.test({ closeSync(fd); }, }); + +Deno.test({ + name: "accepts non Uint8Array buffer", + async fn() { + const moduleDir = path.dirname(path.fromFileUrl(import.meta.url)); + const testData = path.resolve(moduleDir, "testdata", "hello.txt"); + const buffer = new ArrayBuffer(1024); + const buf = new Int8Array(buffer); + await readTest( + testData, + buf, + buf.byteOffset, + buf.byteLength, + null, + (_fd, bytesRead, data) => { + assertStrictEquals(bytesRead, 11); + assertEquals(data instanceof Buffer, true); + assertMatch((data as Buffer).toString(), /hello world/); + }, + ); + const fd = openSync(testData, "r"); + + try { + const nRead = readSync(fd, buf); + const expected = new TextEncoder().encode("hello world"); + + assertEquals(buf.slice(0, nRead), new Int8Array(expected.buffer)); + } finally { + closeSync(fd); + } + }, +}); diff --git a/tests/unit_node/_fs/_fs_write_test.ts b/tests/unit_node/_fs/_fs_write_test.ts index ab82d4f5bf..3ce030bc60 100644 --- a/tests/unit_node/_fs/_fs_write_test.ts +++ b/tests/unit_node/_fs/_fs_write_test.ts @@ -73,3 +73,42 @@ Deno.test({ assertEquals(decoder.decode(data), "\x00\x00\x00\x00hello world"); }, }); + +Deno.test({ + name: "Accepts non Uint8Array buffer", + async fn() { + const tempFile: string = Deno.makeTempFileSync(); + using file = Deno.openSync(tempFile, { + create: true, + write: true, + read: true, + }); + const bytes = [0, 1, 2, 3, 4]; + const buffer = new Int8Array(bytes); + for (let i = 0; i < buffer.length; i++) { + buffer[i] = i; + } + let nWritten = writeSync(file.rid, buffer); + + const data = Deno.readFileSync(tempFile); + + assertEquals(nWritten, 5); + assertEquals(data, new Uint8Array(bytes)); + + nWritten = await new Promise((resolve, reject) => + write( + file.rid, + buffer, + 0, + 5, + (err: unknown, nwritten: number) => { + if (err) return reject(err); + resolve(nwritten); + }, + ) + ); + + assertEquals(nWritten, 5); + assertEquals(data, new Uint8Array(bytes)); + }, +});