Skip to content

Migrate in-memory file system to new backend - #1107

Open
jaybosamiya-ms wants to merge 7 commits into
mainfrom
jayb/inmem-migration
Open

Migrate in-memory file system to new backend#1107
jaybosamiya-ms wants to merge 7 commits into
mainfrom
jayb/inmem-migration

Conversation

@jaybosamiya-ms

Copy link
Copy Markdown
Member

This PR switches our in-memory filesystem to the new core file system design (see #887). Concretely, it adds a InMem backend, migrates all old usage of in_mem::FileSystem to a resolver-backed one to use the new InMem backend, and then removes the old in_mem::FileSystem. It also revamps the in-mem backend initialization design, moving away from the ugly "temporarily change user and do operations" to an actual controlled initialization, which reduces the chances of footguns, and also makes future improvements easier :)

@jaybosamiya-ms
jaybosamiya-ms marked this pull request as ready for review July 29, 2026 22:41
@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

This PR is easiest to review one commit at a time. It is stacked on top of #1106 due to an ordering requirement but is possible to review independently.

Base automatically changed from jayb/handle-management to main July 30, 2026 13:48
@wdcui

wdcui commented Jul 30, 2026

Copy link
Copy Markdown
Member

Reported by GPT-5.6 sol. Will do my own review soon.

Found two high-confidence issues:

  1. Critical — Missing parent-directory write checks
     litebox/src/fs/resolver.rs:439-495,704-760 
    Create, mkdir, unlink, and rmdir now verify execute permission but not write permission on the parent. Unprivileged guests can modify root-owned  0755  directories. Restore  can_write  checks before backend mutations.
  2. High — Guest-triggerable panic opening directories writable
     litebox/src/fs/in_mem.rs:303-319 
    Opening a directory with  O_WRONLY  or  O_RDWR  reaches  unimplemented!() , allowing a guest to crash the runtime. Return an  EISDIR -equivalent  OpenError  instead.

@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

I don't think it is critical, but yes to the first one. There are some subtleties with the / that are already on my radar for other TODOs as more stuff migrates over, but I have added a check to handle this on all non-/ directories.

The second one, yes, I've mentioned this before too, models like to complain about panics due to unimplemented!() but that is exactly what unimplemented!() is for: to mark something as "this is not implemented right now, need to implement it in a future PR". The panic there is intentional

@wdcui

wdcui commented Jul 31, 2026

Copy link
Copy Markdown
Member

More findings by AI agents:

  1. High —  O_TRUNC  modifies files before authorization
     litebox/src/fs/in_mem.rs:357-365 
    Truncation occurs before write-permission and  O_CREAT|O_EXCL  checks. A denied open can still empty the file.
  2. High — Concurrent  O_CREAT  can replace an entry and panic
     litebox/src/fs/in_mem.rs:501-508 
    Existence checking and insertion are not atomic. Two concurrent creates can overwrite the first entry and trigger  assert!(old.is_none()) .
  3. High — Root-directory mutations bypass write permissions
     litebox/src/fs/resolver.rs:211-223 
    Non-root directories are now checked, but  /  has no permission metadata and is treated as writable. A non-root guest can modify root-owned mode- 0755   / .
  4. Medium —  O_APPEND  writes are not atomic
     litebox/src/fs/resolver.rs:616-626 
    EOF is read separately from the write operation. Concurrent appenders can use the same offset and overwrite each other.

@wdcui wdcui 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. Thanks.

Comment thread litebox/src/fs/in_mem.rs
Comment on lines +358 to +364
// Linux truncates whenever the open succeeds, regardless of the access mode (an
// `O_RDONLY|O_TRUNC` open of a writable file does truncate it); `O_PATH` opens ignore
// `O_TRUNC` entirely.
//
// TODO(jayb): Linux's `may_open` also adds `MAY_WRITE` for `O_TRUNC`, and checks
// permissions _before_ truncating; the resolver does neither, so a denied
// `O_RDONLY|O_TRUNC` open still empties the file here.

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.

Should we worry about Linux's semantics here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The Linux semantics are weird around this, and I am not particularly happy that we are still using OFlags, but changing that out is a bigger ask than the scope of this current change, thus I just wanted to document the weirdness that shows up around Linux-isms, that's all.

@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

Thanks Weidong, the 4 issues your agents mention:

  • 1, 3, and 4: agent is reporting an existing TODO/comment for this in the code.
  • 2: yep, missing an existence check after grabbing the lock, fixing it shortly.

@jaybosamiya-ms
jaybosamiya-ms enabled auto-merge July 31, 2026 21:28
@github-actions

Copy link
Copy Markdown

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details
--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/struct_missing.ron

Failed in:
  struct litebox::fs::in_mem::FileSystem, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/5272b3eb63298b1668d1288bcf55f2c2a4e06a6b/litebox/src/fs/in_mem.rs:35

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.

2 participants