Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3905,6 +3905,9 @@
let buffer_value = arguments.next_eat().ok_or_else(||
// theoretically impossible, argument has been passed already
ctx.throw_invalid_arguments(format_args!("buffer is required")))?;
let buffer = Buffer::from_js(ctx, buffer_value).ok_or_else(|| {
ctx.throw_invalid_argument_type_value(b"buffer", b"TypedArray", buffer_value)
})?;

let offset_value = arguments.next_eat().unwrap_or(JSValue::NULL);
// if (offset == null) {
Expand All @@ -3931,9 +3934,10 @@
} else {
0.0
};
let buffer = Buffer::from_js(ctx, buffer_value).ok_or_else(|| {
ctx.throw_invalid_argument_type_value(b"buffer", b"TypedArray", buffer_value)
})?;
// `length.toNumber()` can re-enter JS and detach `buffer_value`; re-snapshot
// the backing store so the subsequent bounds checks and the read itself see
// the post-coercion length/pointer. The type check above cannot change.
let buffer = Buffer::from_js(ctx, buffer_value).unwrap_or(buffer);

Check warning on line 3940 in src/runtime/node/node_fs.rs

View check run for this annotation

Claude / Claude Code Review

unwrap_or(buffer) falls back to the stale pre-coercion snapshot

The `.unwrap_or(buffer)` fallback returns the *pre-coercion* snapshot — the one value (potentially stale ptr/len after detach) that the re-snapshot on this line exists to avoid. The None arm is provably unreachable (a JSCell's type is immutable; a detached view returns `Some` with `len=0`), so there's no runtime bug, but since the comment already states the invariant and continuing on a stale pointer would corrupt memory, `.expect("buffer JSCell type cannot change after coercion")` would be the
Comment thread
robobun marked this conversation as resolved.
Outdated

// if (length === 0) {
// return process.nextTick(function tick() {
Expand Down
31 changes: 31 additions & 0 deletions test/js/node/fs/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,37 @@
}
});

it("throws ERR_INVALID_ARG_TYPE for a non-buffer before validating offset", () => {
// Node's validateBuffer runs before validateInteger(offset, ...). When both
// arguments are invalid the buffer-type error must win.
let err: any;
try {
readSync(0, "not a buffer" as any, -1, 5);
} catch (e) {
err = e;
}
expect({ code: err?.code, message: err?.message }).toEqual({
code: "ERR_INVALID_ARG_TYPE",
message: expect.stringContaining('"buffer"'),
});
// Same ordering when offset is a non-numeric type.
expect(() => readSync(0, 123 as any, "bad" as any, 5)).toThrowWithCode(TypeError, "ERR_INVALID_ARG_TYPE");

Check warning on line 1279 in test/js/node/fs/fs.test.ts

View check run for this annotation

Claude / Claude Code Review

Vacuous second assertion: passes with or without the fix

This second assertion doesn't actually verify the buffer-before-offset ordering: with a string `offset`, the pre-fix code also throws `TypeError`/`ERR_INVALID_ARG_TYPE` (for `"offset"`, via `validate_integer`'s `!value.is_number()` branch), so `toThrowWithCode` passes either way. To make it load-bearing, also assert the message contains `'"buffer"'` like the first case does, or use a numeric out-of-range offset so the pre-fix code differs.
Comment thread
robobun marked this conversation as resolved.
Outdated
});

it("throws ERR_INVALID_ARG_TYPE for a non-buffer before validating offset (async)", () => {
// Node throws validation errors synchronously from fs.read().
let err: any;
try {
fs.read(0, "not a buffer" as any, -1, 5, 0, () => {});
} catch (e) {
err = e;
}
expect({ code: err?.code, message: err?.message }).toEqual({
code: "ERR_INVALID_ARG_TYPE",
message: expect.stringContaining('"buffer"'),
});
});

const firstFourBytes = new Uint32Array(new TextEncoder().encode("File").buffer)[0];

it("works on large files", () => {
Expand Down
Loading