Skip to content

osfs: Adopt os.Root for path containment, add RootOS, refresh helpers/tests#211

Merged
pjbgf merged 23 commits into
go-git:mainfrom
pjbgf:goroot2
May 11, 2026
Merged

osfs: Adopt os.Root for path containment, add RootOS, refresh helpers/tests#211
pjbgf merged 23 commits into
go-git:mainfrom
pjbgf:goroot2

Conversation

@pjbgf
Copy link
Copy Markdown
Member

@pjbgf pjbgf commented May 9, 2026

Overhauls the osfs package, replacing the filepath-securejoin-based path containment with Go's native os.Root (introduced in Go 1.24). It also reworks the chroot and mount helpers, expands cross-implementation tests, adds a go-git integration job to CI, and tightens documentation across every filesystem implementation.

Removes any references to the deduplication logic and the previous ChrootOS FS.

Benchstat

Less memory churn and less allocations:

                             │  before  │               after              │
                             │    sec/op    │    sec/op     vs base                │
Compare/osfs.boundOS_open-24   2.412µ ± 11%   2.576µ ± 12%  +6.82% (p=0.041 n=6)
Compare/osfs.boundOS_read-24   24.20µ ±  6%   24.53µ ±  6%       ~ (p=0.310 n=6)
Compare/osfs.rootOS_open-24                   1.605µ ±  6%
Compare/osfs.rootOS_read-24                   24.44µ ±  3%
geomean                        4.668µ         7.057µ        +4.06%               ¹
¹ benchmark set differs from baseline; geomeans may not be comparable

                             │      before   │                 after               │
                             │      B/op      │    B/op     vs base                   │
Compare/osfs.boundOS_open-24    1080.0 ± 0%     376.0 ± 0%  -65.19% (p=0.002 n=6)
Compare/osfs.boundOS_read-24     0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=6) ¹
Compare/osfs.rootOS_open-24                     248.0 ± 0%
Compare/osfs.rootOS_read-24                     0.000 ± 0%
geomean                                     ²               -41.00%               ³ ²
¹ all samples are equal
² summaries must be >0 to compute geomean
³ benchmark set differs from baseline; geomeans may not be comparable

                             │  before  │               after               │
                             │  allocs/op   │ allocs/op   vs base                   │
Compare/osfs.boundOS_open-24   24.00 ± 0%     17.00 ± 0%  -29.17% (p=0.002 n=6)
Compare/osfs.boundOS_read-24   0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=6) ¹
Compare/osfs.rootOS_open-24                   12.00 ± 0%
Compare/osfs.rootOS_read-24                   0.000 ± 0%
geomean                                   ²               -15.84%               ³ ²


                             │  before  │               after             │
                             │     B/s      │     B/s       vs base              │
Compare/osfs.boundOS_read-24   1.261Gi ± 6%   1.244Gi ± 5%       ~ (p=0.310 n=6)
Compare/osfs.rootOS_read-24                   1.249Gi ± 3%
geomean                        1.261Gi        1.246Gi       -1.36%

Copilot AI review requested due to automatic review settings May 9, 2026 10:52
Comment thread .github/workflows/go-git-integration.yml Dismissed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes the osfs implementation by switching path containment from filepath-securejoin to Go’s native os.Root, and introduces a new RootOS variant for caller-managed root lifecycle. It also updates tests/benchmarks and adds CI coverage by running go-git integration tests against the local go-billy checkout.

Changes:

  • Replace filepath-securejoin containment with os.Root, adding RootOS (FromRoot) and updating BoundOS to use per-operation os.Root.
  • Refresh and expand cross-filesystem tests (especially around symlinks, chroot behavior, and escape errors).
  • Add a go-git integration GitHub Actions workflow; remove filepath-securejoin dependency from go.mod/go.sum.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
test/symlink_test.go Normalizes Readlink assertions across platforms (slash vs host separators).
test/fs_test.go Updates expected error for containment escapes to osfs.ErrPathEscapesParent.
osfs/os.go Updates OS FS factory docs/behavior to always return BoundOS backed by os.Root.
osfs/os_rootfs.go Adds RootOS, FromRoot, and shared containment/error-translation helpers.
osfs/os_options.go Moves Option type behind !js build tag for consistent builds.
osfs/os_js.go Keeps js/wasm option/types available; adds Option type definition for js builds.
osfs/os_bound.go Rewrites BoundOS to open/close os.Root per operation; refactors containment logic.
osfs/os_bound_windows_test.go Adds Windows-specific path normalization tests (drive + slash-prefixed forms).
osfs/os_bound_test.go Overhauls tests for new os.Root containment behavior and new RootOS.
osfs/os_bench_test.go Expands benchmarks to compare BoundOS vs RootOS vs stdlib.
helper/chroot/chroot.go Updates chroot helper docs and refines symlink target rewrite/readlink translation.
helper/chroot/chroot_test.go Adds tests for readlink normalization/preservation across edge cases (incl. Windows forms).
go.mod Drops filepath-securejoin dependency.
go.sum Removes checksums for dropped dependency.
embedfs/embed.go Tightens documentation for embed-backed read-only filesystem semantics.
.github/workflows/go-git-integration.yml Adds CI job to test go-git against the PR’s local go-billy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread osfs/os_bench_test.go
Comment thread helper/chroot/chroot.go Outdated
Comment thread osfs/os_rootfs.go Outdated
Comment thread osfs/os_rootfs.go
Comment thread osfs/os_bound.go
Comment thread osfs/os_rootfs.go
Comment thread osfs/os.go
pjbgf and others added 20 commits May 9, 2026 15:45
Signed-off-by: Paulo Gomes <paulo@entire.io>
This change removes the previous on-demand costly evaluation of paths
with the Go's traversal resistent primitive os.Root.

The benchmarks in /test/ indicate that in most scenarios this represent
a positive performance:

                                 │   base.txt    │                pr.txt                 │
                                 │    sec/op     │     sec/op      vs base               │
Compare/osfs.boundOS_open-4        15.78µ ±   4%    13.38µ ±   2%  -15.22% (p=0.002 n=6)
Compare/osfs.boundOS_read-4        88.45µ ±   1%    91.08µ ±   0%   +2.97% (p=0.002 n=6)
Compare/osfs.boundOS_write-4       924.4µ ±  23%    867.2µ ±  18%        ~ (p=0.180 n=6)
Compare/osfs.boundOS_create-4      31.39µ ±  51%    23.10µ ±  72%        ~ (p=0.065 n=6)
Compare/osfs.boundOS_stat-4        9.493µ ±   2%    4.244µ ±   2%  -55.30% (p=0.002 n=6)
Compare/osfs.boundOS_rename-4      68.14µ ±   1%    42.76µ ±   3%  -37.26% (p=0.002 n=6)
Compare/osfs.boundOS_remove-4      30.14µ ±   2%    24.31µ ±   3%  -19.34% (p=0.002 n=6)
Compare/osfs.boundOS_mkdirall-4    18.25µ ± 351%    17.74µ ± 303%        ~ (p=0.699 n=6)
Compare/osfs.boundOS_tempfile-4    39.63µ ±   5%    34.35µ ±   2%  -13.31% (p=0.002 n=6)

                                 │     base.txt     │                 pr.txt                  │
                                 │       B/op       │      B/op       vs base                 │
Compare/osfs.boundOS_open-4         1032.0 ±   0%       424.0 ±   0%  -58.91% (p=0.002 n=6)
Compare/osfs.boundOS_read-4          0.000 ±   0%       0.000 ±   0%        ~ (p=1.000 n=6) ¹
Compare/osfs.boundOS_write-4        1507.0 ±   3%       261.0 ±   0%  -82.68% (p=0.002 n=6)
Compare/osfs.boundOS_create-4       1536.5 ±   3%       278.0 ±   1%  -81.91% (p=0.002 n=6)
Compare/osfs.boundOS_stat-4         1120.0 ±   0%       240.0 ±   0%  -78.57% (p=0.002 n=6)
Compare/osfs.boundOS_rename-4       4845.0 ±   0%       122.0 ±   1%  -97.48% (p=0.002 n=6)
Compare/osfs.boundOS_remove-4      1055.00 ±   0%       63.00 ±   0%  -94.03% (p=0.002 n=6)
Compare/osfs.boundOS_mkdirall-4     2123.5 ±  20%       199.0 ±   8%  -90.63% (p=0.002 n=6)
Compare/osfs.boundOS_tempfile-4      183.0 ±   1%       336.0 ±   0%  +83.61% (p=0.002 n=6)

                                 │    base.txt    │                 pr.txt                 │
                                 │   allocs/op    │  allocs/op    vs base                  │
Compare/osfs.boundOS_open-4        21.000 ±  0%      9.000 ±  0%   -57.14% (p=0.002 n=6)
Compare/osfs.boundOS_read-4         0.000 ±  0%      0.000 ±  0%         ~ (p=1.000 n=6) ¹
Compare/osfs.boundOS_write-4       25.000 ±  4%      8.000 ±  0%   -68.00% (p=0.002 n=6)
Compare/osfs.boundOS_create-4      26.000 ±  4%      8.000 ±  0%   -69.23% (p=0.002 n=6)
Compare/osfs.boundOS_stat-4        19.000 ±  0%      3.000 ±  0%   -84.21% (p=0.002 n=6)
Compare/osfs.boundOS_rename-4      68.000 ±  0%      8.000 ±  0%   -88.24% (p=0.002 n=6)
Compare/osfs.boundOS_remove-4      19.000 ±  0%      3.000 ±  0%   -84.21% (p=0.002 n=6)
Compare/osfs.boundOS_mkdirall-4    32.500 ± 14%      8.000 ± 12%   -75.38% (p=0.002 n=6)
Compare/osfs.boundOS_tempfile-4     6.000 ±  0%     14.000 ±  0%  +133.33% (p=0.002 n=6)

A new ErrPathEscapesParent was introduced to represent when an operation is
attempting to escape the root/bound dir used for the bound OS.

Signed-off-by: Paulo Gomes <pjbgf@linux.com>
Capabilities() used bitwise AND instead of OR, reporting no
capabilities. translateError() swallowed errors when Unwrap returned
nil and relied on exact string matching. Stat() created the base dir
outside os.Root as a side-effect. Abs-to-relative conversion was
duplicated across six methods. MkdirAll() silently discarded the
caller's permission mode. Chroot() downgraded FromRoot instances to
per-operation mode.

Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Cache os.Root in newBoundOS to eliminate per-operation OpenRoot/Close
syscall pairs. Defer symlink resolution in OpenFile to a retry path
that only triggers on path-escape errors, removing an Lstat from every
non-create open. Simplify fsRoot and Chroot now that os.Root is always
cached.

Add benchmarks for Create, Stat, Lstat, Rename and Remove. Fix
WalkDir benchmark to use fs.WalkDir via iofs adapter for all
implementations. Exclude setup cost from Rename and Remove benchmarks.

Add TestOpenAbsSymlinkInsideRoot to prove the symlink fallback in
OpenFile is needed: os.Root rejects absolute symlink targets that
point inside the root, and BoundOS resolves them to relative paths.

Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Introduce RootOS, a high-performance filesystem backed by a
caller-managed os.Root that is reused across all operations. BoundOS
becomes a thin wrapper that opens and closes an os.Root per operation,
avoiding leaked directory handles on Windows.

FromRoot now returns (*RootOS, error), validating the root upfront
rather than checking on every operation. RootOS.Chroot returns a
child RootOS; BoundOS.Chroot returns a child BoundOS.

Add rootOS sub-benchmarks alongside the existing implementations.

Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Chroot previously opened a new os.Root for the child path that was never
closed, leaking a file descriptor on every call. Wrap the existing
RootOS via helper/chroot instead, reusing the caller-managed parent
os.Root for all operations.

Document that containment remains anchored at the parent root rather
than the chroot path; callers needing a tighter boundary should open a
new os.Root and wrap it with FromRoot.

Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 3c52307e49ba
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Adjusts ChrootHelper.Readlink doc to match the implementation
(relative targets are returned unchanged), enriches the
"not a dir" and Rename error messages with the offending paths,
documents RootOS.Chroot's auto-create side effect, reintroduces
WithChrootOS as a deprecated no-op for API compatibility with
go-git, and expands TestStatBaseFile to cover ".", "./" and "./.".

Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
pjbgf added 2 commits May 10, 2026 05:57
ChrootOS no longer exists; drop the WithChrootOS option, the
ChrootOSFS type constant, and the js/wasm newChrootOS shim, along with
the tests and API-compat assertions that referenced them.

Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
The option was a no-op in both build tags: on non-js the value was
never read, and on js it was stored on BoundOS and propagated through
Chroot but never consulted by path resolution. Containment is now
handled by os.Root (non-js) and the chroot helper (js), making
deduplication moot.

Drops the option, the deduplicatePath fields, and the variadic bool on
newBoundOS, along with the related test and API-compat assertion.

Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
@pjbgf pjbgf added the breaking label May 10, 2026
Signed-off-by: Paulo Gomes <paulo@entire.io>
@pjbgf pjbgf merged commit 13ded9e into go-git:main May 11, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants