Repository navigation
Conversation
…s API In overlay mode the files API reads a session's workspace through MergedView, which serves the upper layer and falls back to the lower one unless it finds a ".wh.<name>" marker for the exact path. That marker is only what MergedView itself writes when a file is deleted through the API, and it was only checked on the last path element. The upper layer is also the kernel overlayfs upperdir of the session's bwrap mount, and the kernel records a process's deletions differently: a deleted lower entry becomes a 0/0 character device, and a directory created in place of a deleted one gets an opaque xattr (user.overlay.opaque inside bwrap's user namespace, trusted.overlay.* without one). MergedView knew neither, so after `rm f` or `rm -r d` in the session, the API kept serving the old contents from lower, listed the whiteout device as a file, and showed the old lower files again in a recreated directory. The same happened under a directory deleted through the API. Treat the lower entry at a path as hidden when upper has, at that path or at any parent, a ".wh." marker, a non-directory (which includes the kernel whiteout) or an opaque directory, and never show a kernel whiteout itself. Stat, Open, ReadFile, ReadDir and Search all use this. rejectSymlink now stops at ENOTDIR, since nothing exists below a file or whiteout in that layer, so a path under a deleted directory reads as not found instead of failing with ENOTDIR.
github-actions
Bot
added
component/execd
size/L
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae34438541
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if m.lowerHidden(rel) { | ||
| return nil, &os.PathError{Op: "open", Path: path, Err: os.ErrNotExist} |
There was a problem hiding this comment.
When execd cannot open an existing upper file (for example, a session-owned mode-000 file in an unprivileged/userns deployment), this branch discards the EACCES from os.Open and lowerHidden(rel) is true simply because that ordinary upper file is non-directory, so Open (and the same pattern in ReadFile) synthesizes ENOENT. The download handler maps that to a 404, and RW mode now reports an existing file as missing instead of surfacing its permission or I/O error; preserve the upper read error when the path itself is a non-whiteout entry while retaining not-found for actual whiteouts or paths hidden by an ancestor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. Fixed in the latest commit. When upper holds an entry at the path that is not a kernel whiteout, Open and ReadFile now return the result of reading that entry, error included, as Stat already does. Not-found is left for real whiteouts and for paths hidden by a parent.
This also covers overlay mode, where the old code fell back to the lower copy and served stale contents for an upper file it could not open. TestMergedView_UnreadableUpperFileKeepsItsError checks both modes with a mode 000 upper file: rw with upper and lower as the same directory, and overlay with a lower copy present.
bwrap mounts its overlay with the userxattr option whether or not it runs in a user namespace, so the kernel always records an opaque directory as user.overlay.opaque. Running bwrap as root without --unshare-user confirms it: the mount options include userxattr and the recreated directory carries user.overlay.opaque. The trusted.overlay.opaque lookup never matched and cost a second getxattr for every directory in upper that is not opaque.
Open and ReadFile fell through to the hidden-lower check whenever the upper read failed. An upper file that exists but cannot be read, such as a mode 000 file when execd is not root, is a non-directory in upper, so the lower entry counted as hidden and the read reported the file as missing (404) instead of its permission error. In rw mode, where upper and lower are the same directory, that was a regression from the previous EACCES. When upper holds an entry at the path that is not a kernel whiteout, return the result of reading that entry, error included, as Stat already does. This also stops overlay mode from serving the stale lower copy of a file whose upper copy cannot be read.
hidesLower and Search each spelled out which upper entries hide the lower one. Name that rule once, upperEntryHidesLower, and use it in both. Rename hidesLower to hidesLowerAt so it reads apart from lowerHidden, which also walks the parent directories, and let ReadDir use inUpper instead of repeating its check.
In overlay mode, the isolated session files API reads the workspace through
MergedView. It serves the upper layer first and falls back to the lower one unless it finds a.wh.<name>marker for that exact path. That marker is whatMergedViewitself writes when a file is deleted through the API, and only the last path element was checked.The upper directory is also the kernel overlayfs upperdir of the session's bwrap mount, and the kernel records a process's deletions differently:
user.overlay.opaquexattr. bwrap always mounts its overlay withuserxattr, with or without a user namespace.MergedViewunderstood neither. I checked this against a real bwrap overlay: after a session ranrm f && rm -r d && mkdir d && echo new > d/n && rm -r eon a lower tree withf,d/xande/y, the files API on current main still read the deletedfandd/xwith their old contents, listede/yand the oldd/x, and Search returned all of them. The same thing happens with the API's own markers under a deleted directory: afterRemoveAll("d"),ditself is gone butd/xcan still be read, listed and found, and recreatingdbrings the old files back.Summary
.wh.marker, a non-directory (which covers the kernel whiteout and a file that replaced a directory), or an opaque directory. This is the usual overlay rule.Stat,Open,ReadFile,ReadDirandSearchall use it, and none of them shows a kernel whiteout itself.OpenandReadFilereturn the result of reading it, error included, asStatalready does. An unreadable upper file therefore keeps its permission error, instead of reading as missing or falling back to a stale lower copy.ReadDirreports a deleted directory as not found.Searchskips hidden lower subtrees and no longer needs itsseenmap, since every upper file now hides the lower file at the same path.rejectSymlinkstops atENOTDIR: nothing exists in a layer below a file or whiteout, so there is nothing to check, and a path under a deleted directory now reads as not found (404) instead of failing withENOTDIR(500).whiteout_linux.go. Other platforms have no overlayfs and get stubs that return false.rwmode the runtime passes the workspace as both lower and upper. Reads there behave as before, which a new test covers.Out of scope: the write paths (
Remove,RemoveAll,Rename,Chmod,WriteFile) still do not know about kernel whiteouts, and the.wh.markers the API writes are still not understood by the kernel overlay (the existing documented limitation of overlay mode). I'd rather handle the copy-up and delete paths separately.Testing
Not run (explain why)
Unit tests
Integration tests
e2e / manual verification
New tests, all failing on current main:
TestMergedView_DeletedDirectoryHidesLowerContents,TestMergedView_RecreatedDirectoryHidesOldLowerContentsandTestMergedView_UpperFileHidesLowerDirectory(portable).TestMergedView_KernelWhiteoutHidesLowerFile,TestMergedView_KernelWhiteoutHidesLowerDirectoryandTestMergedView_OpaqueDirectoryHidesLowerContents(Linux). These create the real kernel markers: an unprivileged 0/0mknod, allowed since Linux 5.8, and auser.overlay.opaquexattr. They skip if the filesystem does not allow either.Stat,ReadFile,Open,ReadDirandSearchfor each hidden path.TestMergedView_UnreadableUpperFileKeepsItsErrorchecks a mode 000 upper file in both modes:rwwith upper and lower as the same directory, and overlay with a lower copy present.TestMergedView_RWModeSameUpperAndLowerchecks that reads inrwmode are unchanged, including that listing an existing file is not reported as missing.Manual check against a real
bwrap --overlaymount (bubblewrap 0.11.1, as above), both unprivileged with--unshare-userand as root without a user namespace (in a privileged container, on tmpfs). In both modes the kernel left 0/0 whiteouts anduser.overlay.opaque. Before the fix the view served the deleted contents. After it, the view matches what the session sees: onlyd/nremains.Mutation checks: removing each part of the fix makes at least one test fail. The parts are the opaque check, the non-directory check, the parent walk, the whiteout filter in
ReadDirandSearch, theReadDirnot-found case, theENOTDIRstop inrejectSymlink, and theSearchsubtree skip and root check.go test ./...,go vet ./...and golangci-lint for execd are clean.go buildandgo vetalso pass forGOOS=windowsandGOOS=darwin. Thebwrap-tagged integration tests need root, so I could not run them here.Breaking Changes
Checklist