feat(vm): harden against allocation-bomb DoS and document sandboxing

[davydog187]

Why

The VM is exposed publicly via the playground, which runs arbitrary
untrusted Lua. A red-team pass found that, while the escape surface was
already closed (os/io/require/load are denied by default), the VM
had no defense against resource-exhaustion DoS. The headline case:
string.rep("x", 1e15) allocates a petabyte off-heap and OOMs the entire
BEAM node — taking down every session, not just the attacker's.

What this does

Layered, defense-in-depth, with the deterministic guards doing the
real work and the BEAM heap limit as a backstop.

Library (deterministic, benefits every consumer):

• New Lua.VM.Limits — computes result size from the argument before
  allocating and raises a catchable error (~256 MiB strings, 10M elems).
• Guards on string.rep, string.format width/precision,
  table.unpack/concat/move, the load reader, and the .. operator
  (in both the interpreter and the compiled dispatcher).
• Messages mirror PUC-Lua (resulting string too large, too many results to unpack).

Playground host (website/):

• Runs each snippet in an isolated, memory-capped worker
  (max_heap_size with include_shared_binaries: true) and converts a
  memory kill into a graceful "memory limit" result. Previously the
  :killed exit propagated and crashed the LiveView.
• run/1 gains a standalone safety timeout and restores the caller's
  trap_exit; the output collector is capped against print floods.
• Examples are grouped into categories (Language, Security & limits,
  Errors) with new demos: string bomb, memory limit, infinite loop,
  output flood.

Docs:

• New guides/sandboxing.md covering capability sandboxing, the built-in
  limits, :max_call_depth, and the host pattern for CPU/memory — with
  the two BEAM gotchas spelled out (include_shared_binaries: true is
  mandatory; use async_nolink so a kill doesn't crash the caller).

Key finding: max_heap_size and off-heap binaries

Verified empirically on OTP 29: a plain max_heap_size does not count
off-heap refc binaries, so a binary bomb sails past it.
include_shared_binaries: true (OTP 27+, off by default) fixes that — but
it's GC-timed, so it can't preempt a single giant allocation. That's why
the per-operation library guards (which refuse before allocating) are
the primary defense and the heap limit is the backstop.

Verification

• New test/lua/vm/limits_test.exs (13 tests); lua_examples_test.exs
  gains a :limit expectation for the resource-limit demos.
• mix test green (lua 2051 / 22 skipped; website 52), mix test --only lua53 OK, mix dialyzer 0 errors, mix format --check-formatted OK.
• Every vector demonstrated live in the playground: each fails with a
  clean catchable error / timeout / truncation, and the node + LiveView
  survive (a normal program runs immediately after).

Follow-up (not in this PR)

A VM-level instruction/allocation budget (max_steps via Lua.new/1)
for library consumers who don't wrap calls in a timeout. Deferred as a
separate, benchmarked change since it touches the hot do_execute path;
infinite loops in the playground are already covered by the wall-clock
timeout.

Issues

Partially addresses #77 (execution quotas) — the allocation-bomb guards
and the host CPU/memory pattern. The VM-level step budget (max_steps)
portion is deferred to #306.

[davydog187]

Follow-up tracked in #306: a configurable VM instruction/step budget (max_steps). Deferred from this PR as a separate, benchmarked change since it touches the hot do_execute path; the playground is already protected by the wall-clock timeout.

[davydog187]

It seems like we go through a lot of effor in this module to build a sandbox using raw processes. A few thoughts

1. Would it be better to use a Task or GenServer instead?
2. Would this be useful for consumers of the library, i.e. should we have a Lua.Sandbox process that users can leverage?

[davydog187]

Good questions — taking them in turn, but proposing to keep this PR scoped to the DoS hardening and track the rest separately.

1. Task/GenServer vs. raw spawn_link: the worker is deliberately bare-bones because the two things we depend on are process-local and have to be set inside the spawned process before any user code runs: Process.flag(:max_heap_size, ...) and :trap_exit. A Task would still need the same max_heap_size flag set in its function body, so it buys little here, and the receive/{:EXIT, ^worker, :killed} handling is more legible as an explicit primitive than wrapped in Task.yield/Task.shutdown. The output collector being linked to the worker (not the caller) is also load-bearing for not mistaking its normal exit for a cancellation — that ownership is clearer with explicit spawn_link. I left a comment block on each of these so the intent survives.

2. A reusable Lua.Sandbox process for library consumers is a genuinely good idea, but it is a public-API surface (naming, supervision story, options like the heap ceiling and timeout, docs) that deserves its own design pass rather than being extracted from the playground host in this PR. This PR keeps the library additions purely deterministic (the Lua.VM.Limits guards), and the process wrapper stays a website/-local concern. I would rather ship that as a follow-up alongside the max_steps budget already tracked in Add a configurable VM instruction/step budget (max_steps) #306 — they are the same audience (consumers who do not wrap calls in their own timeout). Happy to open a tracking issue for it.

[davydog187]

Automated review — round 1 🤖

Solid, well-layered hardening with clear docs. The deterministic per-op guards are the right call, and the binary-binary concat fast-path guard in both the interpreter and dispatcher is nicely symmetric. A few things to address before merge.

Blocker

b8e1503 "fix overlfow" violates the commit convention. No conventional-commit type/scope, a typo, and the subject is misleading — the change is a CSS max-h-80 overflow-y-auto on the output <pre>, not an integer-overflow fix. Please reword to something like fix(playground): cap output pane height with a scroll.

Major

table.unpack is documented as a 10M guard but is actually bounded at INT_MAX (~2.1B), so the documented DoS guard does not exist at the stated level. The guide table and the feat(vm) commit body both claim table.unpack | more than 10M results, and the PR says "10M elems". But this PR does not touch table_unpack/2; its pre-existing guard is @unpack_max_results = 2_147_483_647, and Limits.max_element_count (10M) is only consumed by check_range_count! for concat/move. Verified empirically:

table.unpack(t, 1, 10000001)  -- returns 10,000,001 nils, no error

table.unpack({}, 1, 5e8) still builds ~500M nils and can OOM the host — the exact class this PR claims to close. Either lower the unpack guard to Limits.max_element_count() (consistent with concat/move and the docs) or correct the guide row + commit body to state the real INT_MAX bound. Lowering it is the stronger fix and would let table.unpack actually share the deterministic ceiling.

Minor

• load reader guard raises instead of returning nil, <msg>. PUC-Lua's load reports a reader error as the nil, message return; here Limits.check_string_size! raises and propagates out of load. Fine as a DoS backstop (the test relies on it via pcall), but it diverges from load's documented contract — worth a one-line note in load_from_reader.

• output-flood example has no expect and asserts :ok while running 100k prints through the real 2s run/1 timeout. If CI is slow enough to tip it into :timeout, the example test flakes. Consider a smaller count or an expect that tolerates either outcome.

• Website worker mislabels non-memory crashes as "memory limit". Any uncaught exit inside eval (not just a max_heap_size kill) lands on the {:EXIT, ^worker, _reason} arm and returns memory_result/1. Harmless for the playground, but the "Execution exceeded the memory limit" message could be wrong for an unrelated crash. Matching reason == :killed would tighten it.

Verified clean

• No plan-id references in new source/test files.
• Concat guard covers both binary-binary fast paths and the coerced/metamethod fallback (concat_checked/2 + dispatcher_concat).
• table.concat/table.move guards run after the is_integer validation, so j - i + 1 can't crash on non-integers.
• string.format field cap of 99 matches PUC's 2-digit scanformat limit.
• Sandbox/:exclude/max_call_depth doc snippets match the real defaults and "... is sandboxed" / "stack overflow" messages.

[davydog187]

Automated review — round 2 🤖

The round-1 findings landed well. table.unpack now shares Limits.max_element_count() (e0434ab), so the documented 10M ceiling is real and the new table.unpack({}, 1, 500000000) case pins it (it would have passed under the old 2.1B bound). The load-reader raise-vs-return divergence is now documented, and the website worker no longer mislabels non-:killed exits as a memory limit. The off-by-one between unpack (j - i >= max_element_count()) and check_range_count! (count <= max_element_count) lines up — both allow exactly 10M and reject above it.

Blocker

PR title is not in conventional-commit form. main is squash-merged (every recent commit is a single type(scope): … (#NNN)), so the merge commit will inherit this PR's title verbatim: Harden the VM against allocation-bomb DoS; document sandboxing. That has no type(scope): prefix and violates the repo's commit-subject convention. Please retitle, e.g. feat(vm): harden against allocation-bomb DoS and document sandboxing. (This also moots the stray fix overlfow commit, which the squash collapses — no need to reword it, but it confirms the branch wasn't tidied.)

Minor

• output-flood example still asserts strict :ok. Flagged in round 1 and unchanged: it has no :expect, so lua_examples_test requires result.status == :ok while running 20,000 prints through the real 2s run/1 backstop. In practice 20k iterations complete in single-digit ms, so the flake risk is low — but it's the only resource demo asserting a strict status (memory-bomb/timeout use the tolerant expect: :limit). A smaller count or a tolerant expect would remove the last bit of timing dependence.

Verified clean (round 2)

• table.unpack/concat/move share one deterministic 10M ceiling; boundaries consistent.
• Concat guard present on all four paths: both binary-binary fast paths (interpreter + dispatcher) and the coerced/metamethod fallback via dispatcher_concat/concat_checked.
• string.rep float path (2^3 → 8.0) routes through do_string_rep and hits check_string_size!; size math is arbitrary-precision, no overflow.
• :limit demos report status: :timeout whether the memory ceiling or the wall-clock backstop fires first, so that test is robust.
• No plan-id references in new source/tests; guide snippets match real defaults and error messages.

[davydog187]

Automated review — round 1

Solid, well-layered defense-in-depth work. The deterministic per-operation guards are the right primary defense, the thresholds are sane, and the tests would genuinely fail (hang/OOM) before the fix. Library guards cover both the interpreter and the dispatcher concat paths, and table.unpack/concat/move now share one uniform ceiling. A few items below — none block the security value, the main one is a correctness bug in the documented host pattern.

Findings

1. (major, docs) guides/sandboxing.md — SafeLua.run/2 mislabels timeouts as :memory_limit.

case Task.yield(task, @timeout_ms) || Task.shutdown(task, :brutal_kill) do
  {:ok, {result, _lua}} -> {:ok, result}
  {:exit, :killed} -> {:error, :memory_limit}
  ...
  nil -> {:error, :timeout}
end

When a CPU-bound script (while true do end) exceeds the wall-clock, Task.yield/2 returns nil, then Task.shutdown(task, :brutal_kill) kills the still-running task and returns {:exit, :killed} — which matches the :memory_limit clause. The nil -> :timeout branch is only reached if the task happens to finish in the race window between yield and shutdown. So the prescribed pattern reports a timeout as a memory limit. The website's own do_run/1 gets this right by keeping the timeout (after) and the :killed exit on separate receive arms; the guide should mirror that (own monitor + receive ... after rather than yield || shutdown), or at least call out that brutal_kill on timeout also yields :killed.

2. (minor, convention) Commit b8e1503 "fix overlfow" is not a conventional commit — no type/scope and a typo ("overlfow"). If the PR isn't squash-merged this lands on main. Reword to e.g. fix(vm): guard concat result-size overflow.

3. (nit, convention) Commit b7d847d docs: add a security and sandboxing guide is missing the subsystem scope — repo convention wants docs(<scope>): (e.g. docs(docs):).

4. (nit) PR body has no Closes #N. #77 ("Add execution quotas") is the closest open issue; this PR partially addresses it (the max_steps portion is correctly deferred to #306). Consider linking #77 with a note that step-budget is follow-up, for traceability.

Suite skip range unchanged; no plan-id leakage in source/tests/comments; no Co-Authored-By trailers. README and guide claims verified against the actual default sandbox list and the "... is sandboxed" message.

[davydog187]

Automated review — round 2 🤖 (independent re-review)

Re-reviewed the current remote tip after the address step. All prior-round blockers and majors are resolved:

• PR title is now conventional: feat(vm): harden against allocation-bomb DoS and document sandboxing — the squash-merge subject will be well-formed.
• table.unpack shares Limits.max_element_count() (10M), so the documented ceiling is real; the new table.unpack({}, 1, 500000000) case pins it (would have passed under the old 2.1B bound). Boundaries line up: unpack rejects at j - i >= max_element_count(), check_range_count! rejects at count > max_element_count — both allow exactly 10M.
• load reader raise-vs-return divergence is documented inline.
• Website worker no longer mislabels non-:killed exits as a memory limit (separate {:EXIT, ^worker, :killed} vs {:EXIT, ^worker, reason} arms).

Verified clean

• Concat guard on all four paths: both binary-binary fast paths (interpreter do_execute + dispatcher @op_concatenate) and the coerced/metamethod fallback via dispatcher_concat/concat_checked. Size math is arbitrary-precision; byte_size/1 is O(1) so the hot-path guard is cheap.
• string.rep float-count path (1e15/2^3) routes through do_string_rep → check_string_size!; the n*|str| + (n-1)*|sep| computation precedes allocation.
• string.format width/precision cap of 99 matches PUC's 2-digit scanformat; check_format_field! handles the nil (absent) case and both width and precision.
• table.concat/table.move guards run after is_integer validation, so j - i + 1 can't crash on non-integers. ArgumentError renders bad argument to 'table.concat' (range too large) — matches the test.
• examples_by_category covers every example's category (Language / Security & limits / Errors); no example is silently dropped.
• No plan-id references in new source/tests; guide and README snippets match the real default sandbox, "... is sandboxed", "stack overflow", and "resulting string too large" messages.

Minor (carried, non-blocking)

• output-flood example asserts strict :ok with no :expect. It runs 6,000 prints (cap 5,000 lines) through the real 2s run/1 backstop. 6k iterations complete in single-digit ms so flake risk is negligible, but it's the only resource demo asserting a strict status while memory-bomb/timeout use the tolerant expect: :limit. A tolerant expect would remove the last bit of timing dependence. Not a blocker.

LGTM — this is mergeable. The one minor is a pre-existing, low-risk test-timing observation.

[davydog187]

Automated review — round 3

Re-reviewed the current remote tip (bd4aea0) end to end: cloned the branch detached, mix compile --warnings-as-errors clean (only upstream usage_rules warnings), mix test + --only lua53 green (limits 71, suite 17/12 skipped), website 52. I also executed every documented snippet to confirm the prose matches reality.

Verified correct

• All documented error messages are exact: os.exit → "...is sandboxed", string.rep/.. → "resulting string too large", string.format width/precision → "invalid conversion" (boundary confirmed: %99d/%.99f ok, %100d/%.100f raise), table.concat/table.move → "range too large", max_call_depth → "stack overflow".
• The table.unpack ceiling now matches concat/move (count > 10M rejected); unpack({}, 1, 5e8) correctly raises. Boundaries are consistent across all three.
• The README and guide sandbox examples (sandboxed:, exclude:, Lua.sandbox/2) all run as written; enumerated os.*/io.* deny-list entries are genuinely blocked.
• The worker/collector concurrency in LuaSandbox.run/1 is careful: spawn_link + trap_exit with separate arms for :killed (memory) vs the after (wall-clock), collector linked to the worker, and trap_exit restored in an after block even on the exit(reason) cancel path. cancel_async's :shutdown lands on the {:EXIT, _other, reason} arm and tears the worker down. No examples are dropped by examples_by_category.

Findings (all minor/nit — none blocking)

1. guides/sandboxing.md:34 — garbled phrase "metatables, coroutity-free control flow — remains available". Reads like a botched word. Also note coroutine is actually nil by default in this VM, so a clause about coroutines here is misleading; recommend rewording (e.g. "metatables, and ordinary control flow").
2. guides/sandboxing.md — the guide states the full debug library "remains available". That's accurate (verified debug.getinfo is reachable), but exposing debug to untrusted code is itself a sandbox-escape surface in stock Lua (setupvalue/getupvalue/setmetatable). A security guide should at least caveat it. Pre-existing VM design, not introduced here — flagging for completeness.
3. Commit history nit: fix overlfow is a non-conventional, typo'd subject, and docs(docs): doubles type and scope. History-only; harmless if the PR is squashed, worth a tidy otherwise.

Verdict: solid, well-tested, defense-in-depth change with accurate docs. No correctness or convention blockers. Ship once the doc nits are addressed (or waived).