state: adapt gVisor object graph codec for closure transfer - #1
Conversation
There was a problem hiding this comment.
Code Review: state encode/decode refactor
Solid refactor. Splitting image_linux.go into focused state_*_linux.go files and replacing fixed-depth pointer recursion with a discovery work queue + typed object directory is a clean improvement, and the new tests (TestStateInteriorPointers both orderings, TestStateLongPointerChain, TestStateObjectDirectoryCorruption, TestOverlappingSlicesRejected) give good adversarial coverage of the decode trust boundary. Bounds/overflow discipline on the decode path is generally strong (bytes underflow-safe ordering, span overflow guard, per-copy revalidation, constrained type instantiation, function-pointer re-authorization, private-copy validation before host writeback).
Findings below are non-blocking. A few are readability/maintainability; two are worth a closer look (zero-size element amplification and walkObject cost on array-heavy graphs).
One point to confirm (no reliable inline location): the old imageWriter.copy had an explicit interior/overlap guard that the new code drops in favor of resolve/location. That looks intentional and sound, but inline-allocated interface boxes / map value storage go through w.alloc rather than the address set — please confirm an interior pointer into an inline-boxed interface value can't slip past the directory (a test or a note on why it's impossible would settle it).
Minor, not inlined: the depth parameter is now effectively dead in the encode direction of exportIxgo/exportNative after the switch to the object-directory model — consider dropping it or documenting why it's retained.
Additional findings
state_ixgo_linux.go:274: [P3] env resolved from object directory then discarded when bound: When bothr.indexedandboundare true,envis first derived from the object directory and then unconditionally overwritten withc.envhere. The directory-derived slice is discarded, so the earlier lookup is wasted work whenbound. Consider skipping the directory lookup in the bound case or adding a comment clarifying intent.
| ) | ||
|
|
||
| func (r *imageReader) span(addr, count uintptr, t reflect.Type) error { | ||
| if count > uintptr(len(r.mem)) || (t.Size() > 0 && count > uintptr(len(r.mem))/t.Size()) { |
There was a problem hiding this comment.
[P2] Zero-size element count only bounded by full image size
span skips the per-element size guard when t.Size() == 0 (only count > len(r.mem) applies). A ~24-48 byte slice/map header in the untrusted image can declare cap/n up to len(r.mem) (~16M), forcing the decode loops (MakeSlice/MakeMapWithSize and the following per-element r.copy) to run up to ~16M reflect-based iterations from a tiny input. Bounded and no memory-safety impact, but a notable decode-time amplification worth capping for zero-size element types.
|
|
||
| // walkObject indexes embedded fields before any reference is decoded. This is | ||
| // the local equivalent of pkg/state's walkChild, using offsets instead of dots. | ||
| func walkObject(v reflect.Value, addr uintptr, refs map[objectRef]reflect.Value) { |
There was a problem hiding this comment.
[P2] walkObject indexes every array element regardless of pointer targets
walkObject recurses into every struct field and every array element, inserting an objectRef into refs per element, and it runs for every object during allocate. For array-heavy graphs (e.g. [100000]int or arrays of structs) this is O(flattened element count) map inserts plus proportional w.keep growth, even though scalar array elements are never pointer targets and gain nothing from indexing. Consider skipping per-element indexing for arrays whose element type contains no pointer-addressable sub-objects.
| @@ -73,13 +73,32 @@ func (im *valueImage) inherit(from *valueImage) { | |||
|
|
|||
| // encode writes the root, followed by roots retaining every imported object. | |||
There was a problem hiding this comment.
[P3] Stale doc comment: 'encode' vs renamed 'encodeGraph'
The doc comment starts with "encode writes the root..." but the function was renamed to encodeGraph. Behavior described is still correct; only the leading symbol name is stale. Update to "encodeGraph writes the root...".
| fn, bound := r.bindings[ref] | ||
| env := make([]any, int(count)) | ||
| if r.indexed && count != 0 { | ||
| p := r.refs[objectRef{typ: reflect.ArrayOf(int(count), reflect.TypeFor[any]()), addr: address}] |
There was a problem hiding this comment.
[P3] Inner p shadows outer p with an unrelated type in restoreIxgo
The outer p := r.programs[program] (an *ixgoProgram, line 252) is shadowed by p := r.refs[...] (a reflect.Value) inside the indexed block, while the outer p is used a few lines later (e.g. p.interp in the check closure). Correctly scoped so not a bug, but reusing p for two unrelated types in one function is a readability hazard — rename the inner variable (e.g. arr/stored).
| if addr != 0 { | ||
| return fmt.Errorf("%s: unexpected non-nil %s", path, t.Kind()) | ||
| } | ||
| default: |
There was a problem hiding this comment.
[P3] Implicit invariant: default branch does a raw unsafe copy
The default case does a raw unsafe byte copy into the destination. This is memory-safe only because every pointer-bearing kind is handled by an explicit case above, so default is reached only for pointer-free scalars. That invariant is load-bearing: a future kind added without its own case would silently copy unrelocated pointers. Worth an explicit comment or a kind assertion guarding the branch.
aa862ec to
73eae58
Compare
Serialize reflect.Value contents and resolve closure environments through cached, targeted ELF instruction decoding. Remove the obsolete native type encoding and its reserved tag.
Resolve method receivers from ELF type links and represent capture-free functions with an empty environment. Add round-trip coverage and a reflectx transfer proposal.
Refactor closure transfer around gVisor's
pkg/stateobject graph model so typed interior pointers resolve independently of discovery order.The implementation includes:
pkg/statecore and generated collections from gVisord1e35511e5a4in a separate baseline commit, then adapt its address-range index, containment traversal and deferred work queue with source attribution.wire, public type registration and load callbacks. Encode withw.putand image offsets; add a private typed-object directory so decoding allocates or binds all objects before restoring references.encode/decodeentry points and remove the old codec files. The Sentry backend, interceptor, C bridge and host dependencies are unchanged.This gives closure transfer a shared object graph implementation without adding a gVisor dependency to the host module.