Skip to content

node:fs: validate buffer type before offset in fs.read/readSync - #32301

Open
robobun wants to merge 3 commits into
mainfrom
claude/farm/9d47e6bb/fs-read-error-order
Open

node:fs: validate buffer type before offset in fs.read/readSync#32301
robobun wants to merge 3 commits into
mainfrom
claude/farm/9d47e6bb/fs-read-error-order

node:fs: validate buffer type before offset in fs.read/readSync

a4f3a88
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jun 15, 2026 in 13m 52s

Code review found 2 potential issues

Found 4 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/runtime/node/node_fs.rs:3937-3940 unwrap_or(buffer) falls back to the stale pre-coercion snapshot
🟡 Nit test/js/node/fs/fs.test.ts:1279 Vacuous second assertion: passes with or without the fix

Annotations

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

See this annotation in the file changed.

@claude 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 

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

See this annotation in the file changed.

@claude 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.