fix(node/fs): Use correct offset and length in node:fs.read and write (#25049)

My fix in #25030 was buggy, I forgot to pass the `byteOffset` and
`byteLength`. Whoops.

I also discovered that fs.read was not respecting the `offset` argument,
and we were constructing a new `Buffer` for the callback instead of just
passing the original one (which is what node does, and the @types/node
definitions also indicate the callback should get the same type).
Fixes #25028.

ext/node/polyfills/_fs/_fs_read.ts

@@ -28,7 +28,7 @@ type readSyncOptions = {
 type BinaryCallback = (
   err: Error | null,
   bytesRead: number | null,
-  data?: Buffer,
+  data?: ArrayBufferView,
 ) => void;
 type Callback = BinaryCallback;
 
@@ -56,7 +56,7 @@ export function read(
 ) {
   let cb: Callback | undefined;
   let offset = 0,
-    buffer: Buffer | Uint8Array;
+    buffer: ArrayBufferView;
 
   if (typeof fd !== "number") {
     throw new ERR_INVALID_ARG_TYPE("fd", "number", fd);
@@ -79,7 +79,7 @@ export function read(
   if (
     isArrayBufferView(optOrBufferOrCb)
   ) {
-    buffer = arrayBufferViewToUint8Array(optOrBufferOrCb);
+    buffer = optOrBufferOrCb;
   } else if (typeof optOrBufferOrCb === "function") {
     offset = 0;
     buffer = Buffer.alloc(16384);
@@ -99,10 +99,10 @@ export function read(
     if (opt.buffer === undefined) {
       buffer = Buffer.alloc(16384);
     } else {
-      buffer = arrayBufferViewToUint8Array(opt.buffer);
+      buffer = opt.buffer;
     }
     offset = opt.offset ?? 0;
-    length = opt.length ?? buffer.byteLength;
+    length = opt.length ?? buffer.byteLength - offset;
     position = opt.position ?? null;
   }
 
@@ -123,12 +123,18 @@ export function read(
         // We use sync calls below to avoid being affected by others during
         // these calls.
         fs.seekSync(fd, position, io.SeekMode.Start);
-        nread = io.readSync(fd, buffer);
+        nread = io.readSync(
+          fd,
+          arrayBufferViewToUint8Array(buffer).subarray(offset, offset + length),
+        );
         fs.seekSync(fd, currentPosition, io.SeekMode.Start);
       } else {
-        nread = await io.read(fd, buffer);
+        nread = await io.read(
+          fd,
+          arrayBufferViewToUint8Array(buffer).subarray(offset, offset + length),
+        );
       }
-      cb(null, nread ?? 0, Buffer.from(buffer.buffer, offset, length));
+      cb(null, nread ?? 0, buffer);
     } catch (error) {
       cb(error as Error, null);
     }
@@ -162,8 +168,6 @@ export function readSync(
 
   validateBuffer(buffer);
 
-  buffer = arrayBufferViewToUint8Array(buffer);
-
   if (length == null) {
     length = 0;
   }
@@ -174,7 +178,7 @@ export function readSync(
   } else if (offsetOrOpt !== undefined) {
     const opt = offsetOrOpt as readSyncOptions;
     offset = opt.offset ?? 0;
-    length = opt.length ?? buffer.byteLength;
+    length = opt.length ?? buffer.byteLength - offset;
     position = opt.position ?? null;
   }
 
@@ -191,7 +195,10 @@ export function readSync(
     fs.seekSync(fd, position, io.SeekMode.Start);
   }
 
-  const numberOfBytesRead = io.readSync(fd, buffer);
+  const numberOfBytesRead = io.readSync(
+    fd,
+    arrayBufferViewToUint8Array(buffer).subarray(offset, offset + length),
+  );
 
   if (typeof position === "number" && position >= 0) {
     fs.seekSync(fd, currentPosition, io.SeekMode.Start);

ext/node/polyfills/internal/fs/utils.mjs

@@ -990,7 +990,11 @@ export const validatePosition = hideStackFrames((position) => {
 export const arrayBufferViewToUint8Array = hideStackFrames(
   (buffer) => {
     if (!(buffer instanceof Uint8Array)) {
-      return new Uint8Array(buffer.buffer);
+      return new Uint8Array(
+        buffer.buffer,
+        buffer.byteOffset,
+        buffer.byteLength,
+      );
     }
     return buffer;
   },

tests/unit_node/_fs/_fs_handle_test.ts

@@ -38,11 +38,15 @@ Deno.test("read specify opt", async function () {
     buffer: new Buffer(byteLength),
     offset: 6,
     length: 5,
+    position: 6,
   };
   let res = await fileHandle.read(opt);
 
-  assertEquals(res.bytesRead, byteLength);
-  assertEquals(new TextDecoder().decode(res.buffer as Uint8Array), "world");
+  assertEquals(res.bytesRead, 5);
+  assertEquals(
+    new TextDecoder().decode(res.buffer.subarray(6) as Uint8Array),
+    "world",
+  );
 
   const opt2 = {
     buffer: new Buffer(byteLength),
@@ -51,8 +55,11 @@ Deno.test("read specify opt", async function () {
   };
   res = await fileHandle.read(opt2);
 
-  assertEquals(res.bytesRead, byteLength);
-  assertEquals(decoder.decode(res.buffer as Uint8Array), "hello");
+  assertEquals(res.bytesRead, 5);
+  assertEquals(
+    decoder.decode(res.buffer.subarray(0, 5) as Uint8Array),
+    "hello",
+  );
 
   await fileHandle.close();
 });

tests/unit_node/_fs/_fs_read_test.ts

@@ -1,6 +1,7 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 /// <reference types="npm:@types/node" />
 import {
+  assert,
   assertEquals,
   assertFalse,
   assertMatch,
@@ -12,23 +13,23 @@ import { Buffer } from "node:buffer";
 import * as path from "@std/path";
 import { closeSync } from "node:fs";
 
-async function readTest(
+async function readTest<T extends NodeJS.ArrayBufferView>(
   testData: string,
-  buffer: NodeJS.ArrayBufferView,
+  buffer: T,
   offset: number,
   length: number,
   position: number | null = null,
   expected: (
     fd: number,
     bytesRead: number | null,
-    data: ArrayBufferView | undefined,
+    data: T | undefined,
   ) => void,
 ) {
   let fd1 = 0;
   await new Promise<{
     fd: number;
     bytesRead: number | null;
-    data: ArrayBufferView | undefined;
+    data: T | undefined;
   }>((resolve, reject) => {
     open(testData, "r", (err, fd) => {
       if (err) reject(err);
@@ -323,33 +324,82 @@ Deno.test({
 });
 
 Deno.test({
-  name: "accepts non Uint8Array buffer",
+  name: "read with offset TypedArray buffers",
   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");
+    const bufConstructors = [
+      Int8Array,
+      Uint8Array,
+    ];
+    const offsets = [0, 24, 48];
 
-      assertEquals(buf.slice(0, nRead), new Int8Array(expected.buffer));
-    } finally {
-      closeSync(fd);
+    const resetBuffer = () => {
+      new Uint8Array(buffer).fill(0);
+    };
+    const decoder = new TextDecoder();
+
+    for (const constr of bufConstructors) {
+      // test combinations of buffers internally offset from their backing array buffer,
+      // and also offset in the read call
+      for (const innerOffset of offsets) {
+        for (const offset of offsets) {
+          // test read
+          resetBuffer();
+          const buf = new constr(
+            buffer,
+            innerOffset,
+          );
+          await readTest(
+            testData,
+            buf,
+            offset,
+            buf.byteLength - offset,
+            null,
+            (_fd, bytesRead, data) => {
+              assert(data);
+              assert(bytesRead);
+              assertStrictEquals(bytesRead, 11);
+              assertEquals(data == buf, true);
+              const got = decoder.decode(
+                data.subarray(
+                  offset,
+                  offset + bytesRead,
+                ),
+              );
+              const want = "hello world";
+              assertEquals(got.length, want.length);
+              assertEquals(
+                got,
+                want,
+              );
+            },
+          );
+
+          // test readSync
+          resetBuffer();
+          const fd = openSync(testData, "r");
+          try {
+            const bytesRead = readSync(
+              fd,
+              buf,
+              offset,
+              buf.byteLength - offset,
+              null,
+            );
+
+            assertStrictEquals(bytesRead, 11);
+            assertEquals(
+              decoder.decode(buf.subarray(offset, offset + bytesRead)),
+              "hello world",
+            );
+          } finally {
+            closeSync(fd);
+          }
+        }
+      }
     }
   },
 });

tests/unit_node/_fs/_fs_write_test.ts

@@ -75,7 +75,7 @@ Deno.test({
 });
 
 Deno.test({
-  name: "Accepts non Uint8Array buffer",
+  name: "write with offset TypedArray buffers",
   async fn() {
     const tempFile: string = Deno.makeTempFileSync();
     using file = Deno.openSync(tempFile, {
@@ -83,32 +83,56 @@ Deno.test({
       write: true,
       read: true,
     });
+    const arrayBuffer = new ArrayBuffer(128);
+    const resetBuffer = () => {
+      new Uint8Array(arrayBuffer).fill(0);
+    };
+    const bufConstructors = [
+      Int8Array,
+      Uint8Array,
+    ];
+    const offsets = [0, 24, 48];
     const bytes = [0, 1, 2, 3, 4];
-    const buffer = new Int8Array(bytes);
-    for (let i = 0; i < buffer.length; i++) {
-      buffer[i] = i;
+    for (const constr of bufConstructors) {
+      // test combinations of buffers internally offset from their backing array buffer,
+      // and also offset in the write call
+      for (const innerOffset of offsets) {
+        for (const offset of offsets) {
+          resetBuffer();
+          const buffer = new constr(
+            arrayBuffer,
+            innerOffset,
+            offset + bytes.length,
+          );
+          for (let i = 0; i < bytes.length; i++) {
+            buffer[offset + i] = i;
+          }
+          let nWritten = writeSync(file.rid, buffer, offset, bytes.length, 0);
+
+          let data = Deno.readFileSync(tempFile);
+
+          assertEquals(nWritten, bytes.length);
+          console.log(constr, innerOffset, offset);
+          assertEquals(data, new Uint8Array(bytes));
+          nWritten = await new Promise((resolve, reject) =>
+            write(
+              file.rid,
+              buffer,
+              offset,
+              bytes.length,
+              0,
+              (err: unknown, nwritten: number) => {
+                if (err) return reject(err);
+                resolve(nwritten);
+              },
+            )
+          );
+
+          data = Deno.readFileSync(tempFile);
+          assertEquals(nWritten, 5);
+          assertEquals(data, new Uint8Array(bytes));
+        }
+      }
     }
-    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));
   },
 });