fix: extract archives with 2 GiB or more of data - #464
Open
jmrplens wants to merge 2 commits into
Open
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.