Uh oh!
There was an error while loading. Please reload this page.
Switch to explicit file system Contexts - #1231
Conversation
Again, easiest to review one commit at a time :) |
17f0228 to
5af483dCompareWeidong Cui (wdcui)
commented
Aug 28, 2026
GPT/Opus reported the following issues:
Files: litebox/src/fs/resolver.rs:277-287,824-865 , litebox/src/fs/in_mem.rs:574-607 This PR moves ownership authorization for chmod and chown from InMem into the resolver. However, filesystem roots and Composer mount roots are represented as PermissionCheck::ByBackend , and may_change_metadata treats that value as automatically authorized. At the same time, the ownership checks were removed from InMem , leaving neither layer responsible for authorization. For example, a guest running as UID 1000 can call chmod("/") or chown("/") on a root directory owned by UID 0, and the operation reaches InMem without any ownership check. There is a related TODO in resolver.rs noting that write permission on root directories is currently unchecked, but that TODO concerns permission to add or remove directory entries. It does not recognize this chmod / chown bypass. In fact, path_handle says the backend is expected to enforce metadata permissions, which is no longer true for InMem . Suggested fix: Return resolver-checkable permission metadata for filesystem and mount roots, or preserve ownership enforcement in mutable backends for ByBackend handles.
Files: litebox/src/fs/resolver.rs:824-865 , litebox/src/fs/in_mem.rs:574-607 The resolver now authorizes chmod and chown using a copied PermissionInfo , then separately calls the backend to mutate the node. The backend acquires its metadata write lock only during the mutation, so another thread can change ownership between the authorization check and the update. For example:
Before this PR, InMem checked ownership while holding the same write lock used for the mutation, so this race did not exist there. Suggested fix: Pass the acting user into the backend metadata operation and perform authorization while holding the same lock used to update the metadata. Re-reading status in the resolver would still leave a race window.
Files: litebox/src/fs/overlay.rs:271-283,474-495 , litebox/src/fs/nine_p/mod.rs:500-539 Overlay previously created the upper node and then called chown to preserve the lower node’s ownership. This PR removes that chown , relying on NewNode.owner being applied atomically by every upper backend. The 9P backend explicitly documents that Tlcreate and Tmkdir cannot set the requested owning UID: the new node is owned by the user attached to the 9P connection. Therefore, when Overlay copies up a lower file or directory owned by another user, the operation can succeed while silently changing its ownership. The 9P protocol limitation is clearly documented in the code, so that part is known. What does not appear to be recognized is that removing Overlay’s post-create chown turns that limitation into incorrect copy-up behavior. Suggested fix: Preserve the post-create chown and rollback for backends that cannot honor NewNode.owner , or make creation fail when the requested ownership cannot be applied rather than returning a differently owned node.
File: litebox_shim_linux/src/syscalls/file.rs:1745-1753 The old implementation routed chdir through resolve_path , which explicitly rejected an empty pathname with ENOENT . The new implementation calls Context::resolve directly. Context::resolve("") ignores the empty path component and returns the current working directory. Because that path exists and is a directory, sys_chdir returns success without changing anything. Linux requires chdir("") to fail with ENOENT . Suggested fix: Explicitly reject an empty pathname with ENOENT before calling Context::resolve . |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
For the agent findings:
|
c74cc47 to
8b08877Compare🤖 SemverChecks 🤖 Click for details |
Uh oh!
There was an error while loading. Please reload this page.
This PR switches the file system resolver to explicit `Context`s, so that the underlying file system(s) and the context that they are used in are separated. Essentially, this means that nothing within the file system is itself aware of CWD (current working dir) or acting user now, and the `Context` object explicitly carries this. This means that the Linux shim no longer needs to maintain its own `cwd: String` field and manipulation of it, allowing resolution + permission decisions to live in one place. Along with this, I also updated the in-mem backend to use the resolver context rather than maintain its own user management, closing out yet another place of unnecessary duplication and potential inconsistency. Finally, as a drive-by fix: `getcwd` no longer returns a trailing `/`, making it more consistent with Linux.
This PR switches the file system resolver to explicit
Contexts, so that the underlying file system(s) and the context that they are used in are separated. Essentially, this means that nothing within the file system is itself aware of CWD (current working dir) or acting user now, and theContextobject explicitly carries this. This means that the Linux shim no longer needs to maintain its owncwd: Stringfield and manipulation of it, allowing resolution + permission decisions to live in one place.Along with this, I also updated the in-mem backend to use the resolver context rather than maintain its own user management, closing out yet another place of unnecessary duplication and potential inconsistency.
Finally, as a drive-by fix:
getcwdno longer returns a trailing/, making it more consistent with Linux.