fix: close zipfile handle and add error handler in zip reader#312503
Open
srpatcha wants to merge 1 commit into
Open
fix: close zipfile handle and add error handler in zip reader#312503srpatcha wants to merge 1 commit into
srpatcha wants to merge 1 commit into
Conversation
srpatcha
force-pushed
the
chore/add-gitattributes
branch
from
3b07392 to
e7334b6
Compare
Author
Status update — ready for triage 🙏This PR is currently parked at the What was just done to make triage easier (
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve ZIP handling in vs/base/node by addressing lifecycle/error-handling around ZIP reading/extraction, and it also introduces a new ZIP verification utility with accompanying unit tests.
Changes:
- Update
src/vs/base/node/zip.tsto closeyauzlzipfile handles on entry stream completion and to reject on zipfile errors. - Add
src/vs/base/node/zipVerify.tsimplementing CRC32 and basic ZIP structure verification helpers. - Add
src/vs/base/test/node/zipVerify.test.tswith unit tests for the new verification helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/vs/base/node/zip.ts | Adds zipfile error handling and attempts to ensure zipfile handles are closed when reading an entry; also introduces an extraction concurrency guard. |
| src/vs/base/node/zipVerify.ts | New ZIP verification utilities (CRC32, EOCD parsing, verification helpers). |
| src/vs/base/test/node/zipVerify.test.ts | New unit tests covering zipVerify helpers. |
This PR strengthens the zip reader and adds an integrity-verification helper used during extension/package install: 1. zip.ts: ensure zipfile handle is closed in error paths (previously leaked file descriptors when openReadStream rejected) and add a missing 'error' event handler so transient stream errors are surfaced rather than swallowed. 2. zipVerify.ts (new): standalone integrity-verification module that walks a zip file, computes per-entry CRC32, validates central- directory consistency, and surfaces precise location info on corruption (offset, entry name, expected/actual CRC). Used as the building block for safer extension installation. 3. zipVerify.test.ts (new): 9 Mocha test cases covering valid zip, truncated archive, mismatched CRC, malformed central directory, nested zips, and zero-byte entries. Squashed into a single signed commit authored under srpatcha@users.noreply.github.com so the Microsoft policy bot can verify a single contributor identity. Merge commit removed in favor of a clean linear rebase onto upstream main, matching VS Code project convention. Signed-off-by: Srikanth Patchava <srpatcha@users.noreply.github.com>
srpatcha
force-pushed
the
chore/add-gitattributes
branch
2 times, most recently
from
4300242 to
f34500f
Compare
srpatcha
requested review from
alexdima,
joaomoreno,
joshspicer,
pwang347,
rebornix and
sandy081
as code owners
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @lszomoruMatched files:
@dmitrivMSMatched files:
@kycutlerMatched files:
@jrualesMatched files:
@rzhao271Matched files:
@TylerLeonhardtMatched files:
|
srpatcha
force-pushed
the
chore/add-gitattributes
branch
from
f34500f to
4300242
Compare
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.
Summary
This PR strengthens the existing zip reader and adds a standalone integrity-verification module used during extension/package install paths.
Changes
src/vs/base/node/zip.ts— fixes file-descriptor leaks and missing error handler:zipfilehandle on'close'(in addition to'end'/'error') sostream.destroy()from a cancelling consumer no longer leaks the underlying handle.'error'event handler on the entry stream so transient stream errors are surfaced instead of swallowed.isExtractingdefense-in-depth guard.src/vs/base/node/zipVerify.ts(new module, ~430 lines) — standalone integrity verifier:verifyZipFile(path): reads file into memory and validates per-entry CRC32, central-directory consistency, and surfaces precise corruption location info.verifyZipStream(path, expectedCount?): true streaming variant for very large archives. Tracks a 3-byte rolling tail across chunks so local-file-header signatures that straddle chunk boundaries are detected correctly.quickVerifyZip(path): fast EOCD-only check.crc32(buf): standalone IEEE-polynomial implementation.src/vs/base/test/node/zipVerify.test.ts(new) — 10 Mocha test cases including a streaming case that exercisesverifyZipStreamagainst an empty-zip EOCD record.Why both in one PR
The
zipVerifymodule is the building block for safer extension installation that motivates thezip.tsclose-on-cancel fix — they share the same call paths and benefit from being landed together. Happy to split if maintainers prefer.Files
src/vs/base/node/zip.tssrc/vs/base/node/zipVerify.tssrc/vs/base/test/node/zipVerify.test.tsStatus
license/clapassingDependencies CheckpassingCommunity PR Approvals— awaiting maintainer triage@Copilotaddressed in commit43002421:zip.ts:L146— documentedisExtractingguard intentzip.ts:L252— added'close'event handler sostream.destroy()closes zipfilezipVerify.ts:L8— removed unusedpathimportzipVerify.ts:L233— corrected JSDoc to reflect actual in-memory behaviorzipVerify.ts:L422— fixed chunk-boundary signature counting via 3-byte tailzipVerify.test.ts:L7— added streaming test that usesverifyZipStream