Repository navigation
Conversation
extractAll() read the whole data section into a single buffer with one
fs.readSync() call. fs.readSync() truncates its length argument to a signed
32-bit integer, so any archive whose data section reaches 2 GiB arrives as a
negative length and fails before a single file is written:
RangeError [ERR_OUT_OF_RANGE]: The value of "length" is out of range.
It must be >= 0. Received -348492367
at Object.readSync (node:fs:726:3)
at extractAll (.../@electron/asar/lib/asar.js:245:16)
asar list and asar extract-file work on the same archives, since neither goes
through that read.
I kept the descriptor open for the whole extraction, which is where the gain
over re-opening per file actually comes from, but each entry is now copied to
disk in bounded chunks instead of buffering the entire archive. Peak memory no
longer scales with archive size: a 3.9 GB archive goes from failing outright to
4.2s at 201 MB peak.
The copy loop also respects the fs.readSync() return value, which is allowed to
be shorter than requested. The single-shot read ignored it.
extractFileWithFd() takes an optional chunk size so the multi-chunk path can be
covered without materialising a 2 GiB fixture.
readFileSync() in disk.ts already resolves unpacked entries through ensureWithin(). extractAll() joined the path directly instead, so the two disagreed on the same input. This is separate from the 2 GiB read fix and can be dropped on its own.
|
Can you we not do this with read streams instead of custom chunk logic, a pipeline with readstreams should be much better |
I tried it before answering, because a pipeline was my first idea too. It does fix the 2 GiB failure, but I'd still keep the synchronous copy, for two reasons. 1. 2. It isn't better in any case I measured. I put the stream version next to main and this PR, with
Times are wall clock and memory is peak RSS, on Linux with Node 25.2.1 and a warm page cache; where there's a range, it's the spread over two or three runs. Every run's output was compared with the source tree using
So if streams are still preferred, that route means a breaking API change, or a separate async function next to the synchronous one, and it would still be slower with many small files and use more memory than the chunked copy. If an async extraction API is wanted for its own sake, I'd rather do it as a follow-up and keep this PR as the 2 GiB fix for 4.x. The chunk loop itself is the part I'd most like to keep small. It's one buffer, one |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked whether asarUnpack'd entries get the same flat-memory treatment as packed entries — they still go through fs.readFileSync (src/asar.ts:411), which is unchanged from before this PR and outside this fix's stated scope, not a regression it introduces.
Extended reasoning...
The change reworks asar extraction (src/asar.ts, src/disk.ts, test/disk-spec.ts) to stream packed entries from an open fd in bounded chunks instead of buffering the whole data section, fixing a real ERR_OUT_OF_RANGE bug on archives >= 2 GiB; it also adds offset validation and runs unpacked-file reads through ensureWithin for path-traversal hardening. The security-sensitive surface is path handling (ensureWithin) and file I/O correctness, not auth/crypto. A confirmed inline finding (unchecked fs.writeSync return value in extractFileWithFd, src/disk.ts:440) can cause silent truncation on short writes, which is enough on its own to withhold approval regardless of this note.
| destPath: string, | ||
| position: number, | ||
| size: number, | ||
| chunkSize: number = EXTRACT_CHUNK_SIZE, | ||
| ) { | ||
| const out = fs.openSync(destPath, 'w'); | ||
| try { | ||
| if (size <= 0) { | ||
| return; | ||
| } | ||
| const buffer = Buffer.alloc(Math.min(size, chunkSize)); | ||
| let copied = 0; | ||
| while (copied < size) { | ||
| const wanted = Math.min(buffer.length, size - copied); | ||
| // `readSync` is allowed to return fewer bytes than requested, so always loop on the result. | ||
| const read = fs.readSync(fd, buffer, 0, wanted, position + copied); | ||
| if (read <= 0) { | ||
| throw new Error( | ||
| `Unexpected end of archive while extracting "${destPath}" (read ${copied} of ${size} bytes)`, | ||
| ); | ||
| } | ||
| fs.writeSync(out, buffer, 0, read); |
There was a problem hiding this comment.
🔴 extractFileWithFd can silently write truncated/corrupted files on a short fs.writeSync, something the base branch's fs.writeFileSync never allowed. The loop checks fs.readSync's return value (line 434) but calls fs.writeSync(out, buffer, 0, read) at line 440 without checking how many bytes were actually written, then advances copied by the full read count regardless. Node's own fs.writeFileSync internally loops on writeSync's return exactly because a single call isn't guaranteed to write the whole buffer; this streaming path dropped that safeguard. Fix: loop writeSync like readSync is looped, accumulating bytes actually written and retrying the remainder before advancing copied/position, so partial writes on slow, quota-limited, or network-backed destinations can't silently shrink or corrupt extracted file contents.
Why this was flagged
Trigger: extracting any packed entry via extractAll() -> extractFileWithFd() (src/asar.ts new call site) when the destination filesystem's write(2) returns fewer bytes than requested for a chunk (NFS/CIFS mounts, disk-quota edge, EINTR not retried, slow device) — rare on local ext4 but real on network or constrained storage, which asar extract has no control over. At src/disk.ts:440 the code writes read bytes but ignores the return value and advances copied += read unconditionally, not by bytes actually written. The output file loses the unwritten tail of that chunk, and the next chunk's source bytes are appended right after via the file's current write offset, producing a shorter, corrupted file with no error reported — extraction appears to succeed. On base, this same code path called fs.writeFileSync, which loops internally until every byte is written, so this failure mode did not exist there.
Verification: normal (regression from base). src/disk.ts:440-441: fs.writeSync(out, buffer, 0, read); copied += read; — the write count is discarded and copied advances by bytes read, not bytes written. fs.writeSync() issues a single write(2) and may return fewer bytes than requested; it does not loop. On a short write the unwritten tail is lost and the destination file ends up silently truncated/corrupted with no error (the only guard, line 434, checks the read side, not the write side).
There was a problem hiding this comment.
Fixed in 5e24ced: each chunk is now written in a loop that advances the buffer offset and the file position by what fs.writeSync() actually returned, and a write that makes no progress throws instead of looping. The new keeps writing when writeSync writes fewer bytes than it was given test in test/disk-spec.ts caps every writeSync call at 3 bytes and checks the extracted file is complete and byte-identical (before the fix it came out 19 of 43 bytes, with no error).
extractFileWithFd() looped on fs.readSync() but wrote each chunk with a single fs.writeSync() call and ignored its return value. writeSync() is allowed to write fewer bytes than it is given, and when it did, the rest of the chunk was dropped and the next chunk was appended right after it, so the extracted file came out short and corrupted with no error. The fs.writeFileSync() call that extraction used before loops on writeSync() internally, which is why this could not happen there. Each chunk is now written in a loop that advances the buffer offset and the file position by what writeSync() actually wrote, and a write that makes no progress throws instead of looping forever.
This branch has not been deployed
2 participants
What happens
asar extractfails on any archive whose data section reaches 2 GiB, beforewriting a single file:
-348492367is3946474929 - 2^32, the size of the data section wrapped to asigned 32-bit int. The archive is fine:
asar listandasar extract-filebothwork on it, since neither goes through that read.
Cause
extractAll()reads the entire data section in one call:fs.readSync()truncateslengthto a signed 32-bit integer, so adataSizeof 2 GiB or more arrives negative and
validateOffsetLengthRead()rejects it.Buffer.alloc()is fine on 64-bit Node, so the failure is entirely in the read.Introduced in #414. Affected: 4.1.2, 4.2.0, 4.2.1 and current main. 4.1.1 and
earlier are unaffected.
Fix
The descriptor stays open for the whole extraction, which is where the gain over
re-opening per file comes from, but each entry is copied to disk in bounded
64 MiB chunks rather than buffering the whole archive first. Peak memory is now
flat instead of proportional to archive size.
Measured on a 3.9 GB archive with 1636 entries:
I verified the output byte for byte against
asar extract-filerun over everyentry in the archive.
Two smaller things came out of the same loop:
fs.readSync()return value, which is allowed tobe shorter than requested. The single-shot read ignored it.
ensureWithin(), which is whatreadFileSync()in
disk.tsalready does for the same input. That is a hardening rather thanpart of the reported bug, so it is a separate commit and can be dropped
without touching the fix.
Tests
extractFileWithFd()takes an optional chunk size, so the multi-chunk path iscovered without materialising a 2 GiB fixture. Four cases in
disk-spec.ts:multi-chunk copy, single-chunk copy, zero-length entry, and an archive that ends
early.
198 tests pass under both
node:fsand Electron'soriginal-fs, plustscoversrcandtest,oxlintandoxfmt. I could not test Windows locally, wherefollowLinkstakes the other branch, but that logic is untouched.Unrelated, but
#414 is also where #446 comes from, and that one is still open.