Skip to content

🐛 Stop a malformed zip64 record bypassing the error event - #114

Open
alecgibson wants to merge 1 commit into
antelle:masterfrom
alecgibson:fix-112-zip64-uncaught-error
Open

alecgibson wants to merge 1 commit into
antelle:masterfrom
alecgibson:fix-112-zip64-uncaught-error

Conversation

@alecgibson

Copy link
Copy Markdown

Fixes #112

At the moment, a crafted archive can make the central-directory reader throw synchronously from inside an fs.read callback, which surfaces as an uncaughtException rather than an error event. A caller with correct error handling still has its process taken down by a malformed file.

op.complete is assigned three completion handlers, and only one of them — readCentralDirectoryComplete — guards itself. Since all three are reached through the single op.complete() call in readUntilFoundCallback, guarding that one statement covers every path, and any handler added later. That leaves the try/catch in readCentralDirectoryComplete redundant, but removing it would mean re-indenting the whole function, so it stays.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Fixes antelle#112

At the moment, a crafted archive can make the central-directory reader
throw synchronously from inside an `fs.read` callback, which surfaces as
an `uncaughtException` rather than an `error` event. A caller with
correct error handling still has its process taken down by a malformed
file.

`op.complete` is assigned three completion handlers, and only one of
them — `readCentralDirectoryComplete` — guards itself. Since all three
are reached through the single `op.complete()` call in
`readUntilFoundCallback`, guarding that one statement covers every path,
and any handler added later. That leaves the try/catch in
`readCentralDirectoryComplete` redundant, but removing it would mean
re-indenting the whole function, so it stays.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@alecgibson

Copy link
Copy Markdown
Author

A few notes on what this deliberately leaves alone, so the scope is visible without
having to infer it from the diff.

readCentralDirectoryComplete's own try/catch is now redundant. The new guard wraps
op.complete(), which is the only call site of any of the three completion handlers, so
the inner catch can no longer fire on anything the outer one wouldn't. I kept it to hold
the diff to the defect — removing it means re-indenting the whole function — but it is
dead weight and I'm happy to drop it if you'd rather have one guard on that path. Worth
knowing that keeping it does have one behavioural wrinkle: a caller whose own 'error'
handler throws now has that handler re-entered once with its own exception, because the
inner catch emits inside the outer try.

readEntries() has no bounds check on the central-directory offset. The third test
(out-of-range zip64 offset) exercises a crafted zip64 record whose CD offset is past
EOF, which makes Math.min(chunkSize, fileSize - op.pos) negative and fs.read throw
its own argument-validation error. That error now reaches the caller as 'error' rather
than killing the process, which is the point of this PR, but the message a consumer sees
is Node's (ERR_INVALID_ARG_VALUE, not a library error). The non-zip64 path leaks the
same thing today, so I've treated it as pre-existing and out of scope here rather than
riding a second behaviour change along on this one. If you'd prefer a library-owned error
for an offset past EOF, that's a small follow-up — and that test's assertion would want
tightening at the same time, since it currently only asserts that an error arrives plus
that the zip64 record was consumed first.

File descriptors on the error path are untouched here. A fatal error before 'ready'
doesn't release the archive descriptor, but that predates this change and already applies
to inputs like 'Bad archive' — it's #96, with a fix in #113. #115 and #116 cover two
further descriptor problems I ran into while looking at this.

Heads up on a conflict: #113 also adds a v1.16.1 heading to the top of
release-notes.md, so whichever of the two merges second will conflict there. Happy to
rebase this one onto whatever order you prefer, or to drop the heading from this PR and
leave the version for your release commit.

Also filed #117 while tracing the reader: FileWindowBuffer's "Operation in progress"
guard can never fire, because FsRead.read() has no return value. Not reachable today
and not touched here.

The three new tests each fail against master as an uncaught exception and pass with the
fix, so they genuinely cover the patched line rather than the pre-existing catch. npm run lint, npm run check-types and npm test all pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Malformed ZIP64 record throws out of an fs.read callback, bypassing the 'error' event

1 participant