feat: add experimental Sentry Formula runtime and CRIU round trip - #195
MeteorsLiu wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Sentry runtime MVP (gVisor / CRIU round-trip)
Thorough, well-structured experimental PR (~3.5k lines across two nested modules). Error handling is careful, recover() is used appropriately around wire-graph traversal, ref-counting discipline is correct, and the READMEs are unusually accurate (sample outputs match the exact Printf format strings; the verify-linux.sh gate assertion matches the nativeResumed constant).
Trust model note: most parsing is best-effort with panic/recover, and the harness runs as container root with broad caps and seccomp=unconfined. That is acceptable while checkpoint/state images are self-generated in the same run, but the size/offset math on image-supplied values (findings below) should be hardened before this path ever ingests images it did not itself produce.
Highest-value items: two write-handle FD leaks that also risk reading back unflushed data, and missing overflow/ordering validation on untrusted page/VMA size math. Remaining findings are minor. Nothing here is wired into the main llar build.
Additional findings
experimental/criu-import/internal/sentryimport/files_linux.go:1449: [P3] Backing-file path from image opened without confinement:reg.GetName()is an absolute path taken verbatim from the CRIUfiles.imgand opened as container root (withO_RDWRwhen a writable shared mapping matches). A tampered image could direct the runtime to open/write any host file reachable in the mount namespace. Fine for self-generated images; once images can come from an untrusted source, confine backing files to an allowlisted root and reject paths outside it.
| stateFile, err := os.Create(statePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := k.SaveTo(ctx, stateFile, nil, nil, false, true); err != nil { | ||
| return err | ||
| } | ||
| k.BeforeResume(ctx) | ||
| f, err := os.Open(statePath) |
There was a problem hiding this comment.
[P2] stateFile write handle leaked and reopened before flush
stateFile (os.Create(statePath)) is written by k.SaveTo(...) but never closed. Immediately after, the same path is reopened as f for reading (os.Open(statePath)) and passed to readGraph. The write handle leaks, and the reopen relies on unflushed buffered data having reached disk. Close stateFile (checking the error) after SaveTo/BeforeResume and before the os.Open, so the data is flushed before it is read back.
| raw, err := os.Create(statePath + ".template") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| k.Pause() | ||
| if err := k.SaveTo(ctx, raw, nil, nil, false, true); err != nil { | ||
| return fmt.Errorf("save template: %w", err) | ||
| } | ||
| k.BeforeResume(ctx) | ||
| if err := rewriteIDs(statePath+".template", statePath, ids, s.process); err != nil { |
There was a problem hiding this comment.
[P2] template write handle leaked and reopened before flush
Same pattern as the sentry.state path: raw (os.Create(statePath+".template")) is written by k.SaveTo(...) but never closed, and rewriteIDs then reopens statePath+".template" for reading. The write handle leaks and the reopen depends on unflushed data reaching disk. Close raw (checking the error) after SaveTo/BeforeResume and before calling rewriteIDs.
| count := e.GetNrPages() | ||
| if e.NrPages == nil { | ||
| count = uint64(e.GetCompatNrPages()) | ||
| } // CRIU 3.17 stores field 2. | ||
| data := make([]byte, count*hostarch.PageSize) | ||
| if _, err := io.ReadFull(pages, data); err != nil { | ||
| return err | ||
| } | ||
| if n, err := m.CopyOut(ctx, hostarch.Addr(e.GetVaddr()), data, usermem.IOOpts{}); err != nil || n != len(data) { |
There was a problem hiding this comment.
[P2] Unchecked page-count math on image-supplied pagemap entry
count comes straight from the deserialized pagemap image and count*hostarch.PageSize is unchecked: a malformed/hostile entry can overflow uint64 or request an enormous allocation, and e.GetVaddr() is not validated to fall inside a mapped VMA / the user address range before the CopyOut. The VMA-range check in run_linux.go validates VMAs, not pagemap entries. Validate count against a sane maximum and confirm [vaddr, vaddr+count*PageSize) lies within a known VMA before allocating and copying. Acceptable while images are self-generated, but worth hardening before untrusted images can reach this path.
| os.Exit(0) | ||
| return nil |
There was a problem hiding this comment.
[P3] Unreachable return after os.Exit(0)
os.Exit(0) intentionally retires the supervisor and bypasses deferred cleanup, but the trailing return nil (and the stray blank line before the closing brace) is unreachable dead code. Remove it.
| } | ||
| for _, v := range s.memory.Vmas { | ||
| if v.GetStatus()&8 != 0 { // CRIU VMA_AREA_VDSO. | ||
| data := make([]byte, v.GetEnd()-v.GetStart()) |
There was a problem hiding this comment.
[P3] VMA length math lacks End >= Start validation
Start/End are read from the deserialized mm image without checking End >= Start. If End < Start, v.GetEnd()-v.GetStart() wraps to a near-2^64 length, producing a giant make([]byte, ...) (VDSO branch, line 82) or an out-of-range mmap/mprotect length (lines 38, 123). The range check in run_linux.go compares each endpoint to platform min/max independently and does not enforce ordering. Add an explicit End >= Start (and page-alignment) check when reading the checkpoint.
| return fmt.Errorf("unexpected non-private PMA") | ||
| } | ||
| pageEntries = append(pageEntries, &pagemap.PagemapEntry{Vaddr: proto.Uint64(start), CompatNrPages: proto.Uint32(uint32((end - start) / 4096)), Flags: proto.Uint32(4)}) | ||
| buf := make([]byte, 64<<10) |
There was a problem hiding this comment.
[P3] 64 KiB copy buffer reallocated per PMA segment
buf := make([]byte, 64<<10) is allocated fresh inside the per-segment export loop, though its contents are fully overwritten by each CopyIn. Hoist the allocation above the loop to drop one 64 KiB heap allocation per segment. Minor at current scale; scales with segment count.
|
Closed, see #194 |
1 participant
Add isolated MVPs for running ixgo Formulas under Sentry and preserving Go callback state across a native → Sentry → native round trip. Close #194
The implementation includes:
experimental/sentry-gate: standalone Sentry startup with DirectFS, an in-process LISAFS service, and an ixgo Formula guest.Context.Switchwrapper that inspects syscall arguments after the underlying platform returns and before Sentry dispatches the syscall; non-syscall events pass through unchanged.experimental/criu-import: conversion between native CRIU images and pinned Sentry state, including memory, registers, thread identities, signals, and the supported descriptor state.runSandbox: capture the native caller, execute its callback in Sentry, export the completed state, and restore a new native continuation without a fixed business-result ABI.Both experiments remain separate Go modules and leave production LLAR execution unchanged. The round-trip MVP currently uses Linux ARM64/ptrace; real Formula integration into that round trip, systrap migration, and production hardening remain unfinished.