Skip to content

archive: Resolve extraction parents through the open root - #129

Open
vvoland wants to merge 1 commit into
moby:mainfrom
vvoland:parents-root
Open

vvoland wants to merge 1 commit into
moby:mainfrom
vvoland:parents-root

Conversation

@vvoland

@vvoland vvoland commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Extraction opens its destination as an os.Root, but resolved parents of missing or escaping paths through root.Name().
If the destination was renamed and replaced during extraction, that fallback resolved symlinks in the replacement directory.

resolveRootPath now walks symlinks through the os.Root handle and returns root-relative paths.

fsRootPath now takes the caller's *os.Root as well, so xattr, opaque whiteout, and darwin mknod paths also resolve through the open handle. It still joins the result with root.Name() though because those operations need host paths.

@vvoland vvoland self-assigned this Oct 13, 2026
@codecov-commenter

codecov-commenter commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.80%. Comparing base (953c7df) to head (5a27407).
⚠️ Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
rootpath.go 90.47% 2 Missing ⚠️
dev_darwin.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   65.48%   74.80%   +9.32%     
==========================================
  Files          46       46              
  Lines        2393     2366      -27     
==========================================
+ Hits         1567     1770     +203     
+ Misses        605      594      -11     
+ Partials      221        2     -219     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Host-path fallbacks can still target a replacement tree after the opened destination is renamed.

2 open findings
What changed in this PR

Updates archive extraction to resolve symlinks through an open os.Root handle using root-relative paths.

Changes:

  • Reworks root-path resolution around *os.Root.
  • Updates extraction, xattr, whiteout, and Darwin device paths.
  • Adapts Unix path-resolution tests.
File Description
rootpath.go Implements handle-relative resolution.
archive.go Uses resolved root-relative archive paths.
diff.go Passes the open root for whiteouts.
dev_darwin.go Passes the open root for mknod.
archive_unix_test.go Updates resolver tests for os.Root.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rootpath.go Resolved
Comment thread archive_unix_test.go Resolved
@vvoland
vvoland force-pushed the parents-root branch 2 times, most recently from 5de6ce2 to 7563da8 Compare October 13, 2026 14:21
@vvoland
vvoland requested a balanced review from Copilot October 13, 2026 14:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new Lstat identity check falsely rejects destinations opened through symlinks and can accept a raced symlink parent.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

Extraction opens its destination as an os.Root, but resolved parents
of missing or escaping paths through root.Name().
If the destination was renamed and replaced during extraction, that
fallback resolved symlinks in the replacement directory.

resolveRootPath now walks symlinks through the os.Root handle and
returns root-relative paths.

The opaque whiteout walk now uses fs.WalkDir over root.FS(), so it no
longer needs a host path.

xattrs and darwin mknod have no portable handle-relative API, so they
still use a host path from fsRootPath.
fsRootPath now takes the caller's *os.Root, resolves through it, and
returns a breakout error if the host path's parent is not the same
file as the parent resolved through the handle.
The remaining window between that check and the host-path operation
is the same one that existed before.

Signed-off-by: Paweł Gronowski <git@grono.dev>
@vvoland
vvoland force-pushed the parents-root branch 2 times, most recently from 7563da8 to 5a27407 Compare October 13, 2026 14:35
@vvoland
vvoland requested a balanced review from Copilot October 13, 2026 14:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Security-sensitive filesystem race handling and platform-specific path behavior warrant final human review.

0 open findings

🧠 Review effort: Balanced

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.

3 participants