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 6 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.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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. vfs Issues and PRs related to the virtual filesystem subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants