Skip to content

feat(stdlib): route os/require file IO through a virtual filesystem#302

Open
davydog187 wants to merge 6 commits into
mainfrom
feat/vfs-sandbox
Open

feat(stdlib): route os/require file IO through a virtual filesystem#302
davydog187 wants to merge 6 commits into
mainfrom
feat/vfs-sandbox

Conversation

@davydog187
Copy link
Copy Markdown
Contributor

VFS sandbox — route os/require file IO through a virtual filesystem

Plan: .agents/plans/A48-vfs-sandbox.md
Closes #297

Goal

Make filesystem-touching os/require operations safe by default by running them against an in-memory virtual filesystem instead of refusing or reaching the host disk. Integrates ivarvong/vfs (a VFS.Mountable protocol with pluggable backends), defaulting to VFS.Memory. Gives embedding hosts an API to seed files and mount backends, and uses /lua/deps as the dependency root for require.

This PR ships the smallest coherent green slice (dep + State.vfs + os ops + require searcher + populate/mount API). The io.* rewire is deferred — see Out of scope.

Success criteria

  • vfs added to mix.exs as a git dep pinned to ref: 32d2ab618ec12c16fe4f675b5ee8b563c660dd69, matching mix.lock entry, mix deps.get succeeds.
  • Lua.VM.State has a vfs field defaulting to an in-memory VFS (VFS.new/0 + VFS.mount/3 with VFS.Memory.new(%{})), @type t updated, seeded by State.new/0. (verified: test/lua/vfs_test.exs "the default VM has an empty in-memory VFS")
  • Public Lua API to write a file (write_file/3, put_dep/3) and mount a backend (mount/3), each returning an updated %Lua{}. (verified: doctests + test/lua/vfs_test.exs)
  • os.tmpname, os.remove, os.rename operate against state.vfs and thread the updated struct back; none touch the host. (verified: test/lua/vm/stdlib/os_test.exs)
  • require resolves modules from the VFS (resolved path + /lua/deps-anchored), with host File.read fallback so existing set_lua_paths/package.path workflows keep working; a module seeded via the new API is loadable. (verified: test/lua/vfs_test.exs + existing require/luassert tests stay green)
  • No source file or test references the plan id.
  • mix format clean; mix compile --warnings-as-errors passes.
  • mix test green (2105 passed, 19 skipped, 1 excluded; 0 failed).
  • mix test --only lua53 shows no regression (17 passed, 12 skipped, 0 failed).

Changes

```
lib/lua.ex | 55 +++++++++++++++++++++++++++
lib/lua/vm/state.ex | 81 ++++++++++++++++++++++++++++++++++++++--
lib/lua/vm/stdlib.ex | 61 ++++++++++++++++++++++++++-----
lib/lua/vm/stdlib/os.ex | 65 ++++++++++++++++++++++++++++----
mix.exs | 1 +
mix.lock | 1 +
test/lua/vm/stdlib/os_test.exs | 45 ++++++++++++++++++++++
test/lua/vfs_test.exs | new
```

Verification output (tail)

```
mix test -> Result: 2105 passed (59 doctests, 51 properties, 1995 tests), 19 skipped, 1 excluded
mix test --only lua53 -> Result: 17 passed, 12 skipped, 2096 excluded
```

Out of scope

  • Rewiring the io.* library (still a stub) — deferred follow-up.
  • Changing the @default_sandbox deny-list (os.remove/rename/tmpname/require/package stay sandboxed by default; hosts opt out via sandboxed:/exclude:).
  • Flipping deferred Lua 5.3 suite files (files.lua, attrib.lua, verybig.lua, main.lua) green.

🤖 Generated with Claude Code

Filesystem-touching stdlib functions now operate against an in-memory
virtual filesystem carried on Lua.VM.State instead of refusing or
reaching the host disk. Integrates the ivarvong/vfs VFS.Mountable
protocol with the in-memory VFS.Memory backend as the default.

- State.new/0 seeds an empty in-memory VFS; vfs_read/write/rm/exists?/
  mount helpers thread the returned backend struct forward.
- os.tmpname returns a virtual path; new os.remove and os.rename operate
  against state.vfs and never touch the host.
- The require searcher consults the VFS first (the resolved path plus a
  /lua/deps-anchored path), then falls back to host File.read so existing
  set_lua_paths/package.path workflows keep working.
- Lua.write_file/3, Lua.put_dep/3, and Lua.mount/3 let embedding hosts
  seed files and mount backends.

Plan: A48
Closes #297
Comment thread lib/lua/vm/stdlib.ex Outdated
Comment thread mix.exs
# Run "mix help deps" to learn about dependencies.
defp deps do
[
{:vfs, github: "ivarvong/vfs", ref: "32d2ab618ec12c16fe4f675b5ee8b563c660dd69"},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ivar will be cutting a release for me this week

The require searcher silently fell back to File.read/1, letting any VM
that un-sandboxes require reach the host disk and bypass the virtual
filesystem. Default the VFS to be the only backing store; an embedding
host now opts into the host fallback explicitly via set_lua_paths/2,
and the VFS is always consulted first so seeded modules win.

Plan: A48
@davydog187
Copy link
Copy Markdown
Contributor Author

Automated review — round 1

Solid, well-documented slice: the VFS threading discipline through State.vfs_* is clean, the host-disk fallback opt-in via set_lua_paths/2 is a nice safety property, and the tests genuinely pin the new behavior (require-from-VFS, mount, isolation, os.remove/rename). Compile/format/test/suite all green locally. A few real issues, one of which I think is a blocker for an RC hex package.

Blocker

vfs as a runtime git dep makes :lua unpublishable to hex. :lua is a packaged hex library (v1.0.0-rc.0). Hex only allows hex dependencies in a published package. mix hex.build fails outright:

** (Mix) Stopping package build due to errors.
Dependencies excluded from the package (only Hex packages can be dependencies): vfs

Because {:vfs, github: ...} is a top-level runtime dep (no only:), it blocks mix hex.publish. The CI validation (format/compile/test/suite) doesn't exercise hex.build, so this slipped through. Resolving this likely needs vfs published to hex, or vendored, before this can ship in a release.

Major

os.remove / os.rename crash on relative paths instead of returning nil, msg. VFS.Path.normalize/1 raises ArgumentError on non-absolute paths (it doesn't return {:error, %VFS.Error{}}), and the os_* clauses only rescue {:error, %VFS.Error{}}. So Lua sandbox code like local ok, err = os.remove(\"foo.txt\") crashes the whole evaluation:

return os.remove(\"relative.txt\")  ->  ** (Lua.RuntimeException) VFS paths must be absolute (start with `/`); got: \"relative.txt\"
return os.rename(\"a.txt\", \"b.txt\") -> same

Real Lua 5.3 returns nil, \"relative.txt: No such file or directory\", 2. The fix is to treat a relative/un-normalizable path as a normal failure and return the nil, message contract (the require searcher already guards this correctly by only issuing VFS reads for /-prefixed or dep-anchored paths). Worth a test pinning os.remove/os.rename on a bare relative name.

Declared elixir: \"~> 1.16\" but vfs requires ~> 1.18. Pulling vfs in as a runtime dep effectively raises the real floor to 1.18 while mix.exs still advertises 1.16 support to downstreams. Either bump the elixir: requirement to ~> 1.18 (honest), or this compat claim is misleading.

Minor

  • vfs_error_message/2 (os.ex): the %VFS.Error{message: nil} clause is dead — VFS.Error.exception/1 always builds a non-nil message (defaults to \"<kind> at <path>\"). Safe to drop that clause.
  • search_candidate/2: for an absolute pattern (e.g. /fixtures/?.lua), vfs_read_anchored and vfs_read_direct both resolve to the same absolute path, so the VFS is read twice. Harmless, but the anchor_dep_path could return early for absolute paths to avoid the duplicate read.

Automated round-1 review; a human will make the final call.

os.remove/os.rename and the require searcher funnel paths through the
VFS, which raises ArgumentError on non-absolute paths. Guard the
State.vfs_read/vfs_write/vfs_rm boundary so a relative path surfaces as
a standard {:error, %VFS.Error{}} (enoent) instead of aborting the
evaluation, matching real Lua's (nil, "<path>: No such file or
directory") contract.

Also drop the unreachable message: nil clause in vfs_error_message/2,
short-circuit absolute require patterns to a single VFS read, and bump
the declared elixir floor to ~> 1.18 to match the vfs dependency.

Plan: A48
@davydog187
Copy link
Copy Markdown
Contributor Author

Automated review — round 2

Round-1's two correctness fixes landed cleanly: relative paths now go through State.relative_path_error/1 and surface the nil, message contract (with tests pinning os.remove/os.rename on bare relative names), and elixir: is honestly bumped to ~> 1.18. The duplicate absolute-path VFS read is gone, and vfs_error_message/2 no longer carries the dead message: nil clause. Threading discipline still reads well.

Two things remain.

Blocker (carryover, unresolved)

vfs as a top-level runtime git dep still makes :lua unpublishable to hex. {:vfs, github: ...} has no only:, so it is a release dependency, and mix hex.build refuses any non-hex dep:

** (Mix) Stopping package build due to errors.
Dependencies excluded from the package (only Hex packages can be dependencies): vfs

:lua ships as v1.0.0-rc.0 on hex; this blocks mix hex.publish. CI runs format/compile/test/suite but never hex.build, so it stays green while the release path is broken. Needs vfs published to hex (or vendored) before this can ship in a release. Flagging again because it is the gating issue for the RC.

Major

os.rename(x, x) deletes the file. The read→write→remove sequence in os_rename/2 makes a self-rename destructive: vfs_read("/x") ok, vfs_write("/x", contents) ok, then vfs_rm("/x") removes it — so os.rename("/x.txt", "/x.txt") returns true but leaves no file. Reference Lua treats same-path rename as a successful no-op (POSIX rename(2) returns 0 and keeps the file). Guard from == to to return true without touching the VFS, and add a test.

Minor

  • os.rename reports the wrong path on a destination failure. The else arm always builds the message from from: {:error, error, state} -> {[nil, vfs_error_message(from, error)], state}. When the source reads fine but the destination write fails (e.g. os.rename("/from.txt", "relative")), Lua should name the offending path — here relative — but the message is prefixed "/from.txt: ...". The existing relative-path test only exercises the both-relative case, which fails at the read of from, so this mis-attribution isn't covered. Thread the failing path through instead of hardcoding from.
  • No errno third return value. Lua 5.3 os.remove/os.rename return nil, msg, errno on failure; the impl returns only nil, msg. Harmless for most code but a slight contract gap.

Automated round-2 review; a human makes the final call.

Treat os.rename(x, x) as a successful no-op so a self-rename no longer
reads/writes/removes the same path and leaves nothing behind, matching
POSIX rename(2). Attribute rename failures to the path that actually
failed (the destination on a failed write) rather than always the source,
and append the conventional POSIX errno as the third return value of
os.remove/os.rename on failure to match the Lua 5.3 contract.

Plan: A48
@davydog187
Copy link
Copy Markdown
Contributor Author

Automated review — round 3

Round-2's items all landed: self-rename is now a guarded no-op (os_rename([path, path | _]), the destination path is threaded through with_path/2 so a failing write names the right path, and vfs_errno/1 adds the third errno return. Tests pin each. Threading discipline still reads cleanly. mix format, mix compile --warnings-as-errors, mix test (2111 passed, 19 skipped), and mix test --only lua53 (17 passed, 12 skipped) are all green on bc8fdb9.

Two release-path blockers remain.

Blocker (carryover, still unresolved)

vfs as a top-level runtime git dep makes :lua unpublishable to hex. Re-confirmed on this head:

$ mix hex.build
...
** (Mix) Stopping package build due to errors.
Dependencies excluded from the package (only Hex packages can be dependencies): vfs

:lua ships as v1.0.0-rc.0 on hex; {:vfs, github: ...} has no only:, so it is a release dependency and mix hex.build refuses it. CI runs format/compile/test/suite but never hex.build, so the branch stays green while the release path is broken. This needs vfs published to hex (or vendored) before the slice can ship in a release.

Blocker (new)

.github/workflows/release.yml still pins elixir-version: '1.16.1', but this PR bumps mix.exs to elixir: \"~> 1.18\". The publish job runs mix deps.get + mix compile on 1.16.1, which now fails the version requirement before it ever reaches the git-dep error. Bump the release workflow's elixir-version (and likely otp-version, 26.2.1 is fine for 1.18) to match the new floor. CI (ci.yml) already tests 1.19/1.20-rc so it is unaffected, but the release pipeline is broken.

Minor

  • State.vfs_exists?/2 is dead code. Defined and @doc'd in lib/lua/vm/state.ex but never called from lib/ or test/. Drop it (or wire it into the searcher's existence check) — it currently just adds surface area.

Nit

  • Misleading comment in search_candidate/2 (absolute clause). The comment says reading the absolute path directly is "the same path anchor_dep_path would produce," but Path.join(\"/lua/deps\", \"/fixtures/x.lua\") yields /lua/deps/fixtures/x.lua, not /fixtures/x.lua. The single direct read is the intended behavior; the justification in the comment is just wrong.

The functional slice (State.vfs threading, os ops, require searcher, populate/mount API) is correct and well-tested. The gating issues are both about the release pipeline, not runtime behavior.

Automated round-3 review; a human makes the final call.

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.

Make os operations safe by default with VFS

1 participant