Skip to content

vfs: apply open(2) effects to ZipProvider handles - #65853

Open
pipobscure wants to merge 3 commits into
nodejs:mainfrom
pipobscure:vfs-zip-handle-commit
Open

vfs: apply open(2) effects to ZipProvider handles#65853
pipobscure wants to merge 3 commits into
nodejs:mainfrom
pipobscure:vfs-zip-handle-commit

Conversation

@pipobscure

@pipobscure pipobscure commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

vfs: apply open(2) effects to ZipProvider handles

A ZipProvider handle keeps its content in memory and adds the entry to
the archive when it is closed, and only if something was written. The
effects a real open(2) has at open time are therefore lost, and so is
the metadata the entry already carried:

  • open(path, 'w') followed by close() neither truncates an existing
    entry nor creates a missing one; the same holds for "a" on a missing
    file. Tools that touch or truncate by open-then-close do nothing.
  • Rewriting an entry (append, or an in-place write through "r+") re-adds
    it with the mode argument open() received (fs's default 0o666),
    not the mode the entry had, so a 0o755 script silently loses its
    executable bit.
  • fstat on a handle reports that same open() mode and the current
    time instead of the entry's mode and modification time.
  • Renaming a file onto an existing directory succeeds and leaves a name
    that is both a file and a directory; real file systems refuse with
    EISDIR.

This adds a test for each of these against a mounted ZipBuffer, stating
the real-fs outcome as the expectation.

Proposed solution: mark the handle dirty at open time when the flags
imply creation or truncation, so close always commits; carry the
existing entry's mode and modification time on the handle, use them
for fstat and for the re-added entry, and only fall back to the
open() mode for a newly created entry; and reject rename onto an
existing directory with EISDIR before touching the archive.

Note: Since these are gaps/defects in existing functionality, I decided to create the failing tests first (first commit) and then add the fix/solution as a second commit. That way whoever wants to review this can first prove out the issue, before applying the solution.

This goes with the VFS work by @mcollina and the bug-fix PRs by @trivikr.

A ZipProvider handle keeps its content in memory and adds the entry to
the archive when it is closed, and only if something was written. The
effects a real `open(2)` has at open time are therefore lost, and so is
the metadata the entry already carried:

* `open(path, 'w')` followed by `close()` neither truncates an existing
  entry nor creates a missing one; the same holds for "a" on a missing
  file. Tools that touch or truncate by open-then-close do nothing.
* Rewriting an entry (append, or an in-place write through "r+") re-adds
  it with the `mode` argument `open()` received (fs's default 0o666),
  not the mode the entry had, so a 0o755 script silently loses its
  executable bit.
* `fstat` on a handle reports that same `open()` mode and the current
  time instead of the entry's mode and modification time.
* Renaming a file onto an existing directory succeeds and leaves a name
  that is both a file and a directory; real file systems refuse with
  EISDIR.

This adds a test for each of these against a mounted ZipBuffer, stating
the real-fs outcome as the expectation.

Proposed solution: mark the handle dirty at open time when the flags
imply creation or truncation, so close always commits; carry the
existing entry's mode and modification time on the handle, use them
for `fstat` and for the re-added entry, and only fall back to the
`open()` mode for a newly created entry; and reject `rename` onto an
existing directory with EISDIR before touching the archive.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
A ZipProvider handle adds its entry to the archive when it is closed.
Give that model the effects a real open(2) has up front, and keep the
metadata an entry already carries:

* A handle whose flags create or truncate the file starts out dirty,
  so closing it without a write still creates the missing entry or
  truncates the existing one.
* The handle remembers the entry's own mode and modification time.
  The re-added entry keeps that mode instead of taking the `mode`
  argument `open()` was given (fs's default 0o666), so a 0o755 script
  survives an append or an in-place write; only a newly created file
  takes the mode from `open()`.
* `fstat` reports that mode and, until the handle has changed the
  file, that modification time.
* `rename` refuses to move a file onto an existing directory with
  EISDIR before touching the archive.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. vfs Issues and PRs related to the virtual filesystem subsystem. labels Sep 6, 2026
@pipobscure pipobscure changed the title Vfs zip handle commit vfs: apply open(2) effects to ZipProvider handles Sep 6, 2026
Comment thread lib/internal/vfs/providers/ziparchive.js Outdated
The options passed to `createFileStats()` and to the archive's `add()`
and `addSync()` are plain literals, so a property added to
`Object.prototype` would reach those callees as if it had been passed
on purpose. Create them with a null prototype so only the fields set
here are visible.

Refs: nodejs#65853
Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
@pipobscure
pipobscure marked this pull request as ready for review September 6, 2026 14:33
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2026
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.18%. Comparing base (7991140) to head (a88f5b8).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/vfs/providers/ziparchive.js 93.10% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65853      +/-   ##
==========================================
- Coverage   90.19%   90.18%   -0.01%     
==========================================
  Files         771      771              
  Lines      264622   264645      +23     
  Branches    50223    50242      +19     
==========================================
- Hits       238663   238661       -2     
- Misses      16965    16970       +5     
- Partials     8994     9014      +20     
Files with missing lines Coverage Δ
lib/internal/vfs/providers/ziparchive.js 97.67% <93.10%> (-0.26%) ⬇️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. vfs Issues and PRs related to the virtual filesystem subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants