Skip to content

state: adapt gVisor object graph codec for closure transfer - #1

Merged
MeteorsLiu merged 12 commits into
xgo-dev:mainfrom
MeteorsLiu:codex/state-encode-decode
Sep 16, 2026
Merged

MeteorsLiu merged 12 commits into
xgo-dev:mainfrom
MeteorsLiu:codex/state-encode-decode

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Refactor closure transfer around gVisor's pkg/state object graph model so typed interior pointers resolve independently of discovery order.

The implementation includes:

  • Import the pkg/state core and generated collections from gVisor d1e35511e5a4 in a separate baseline commit, then adapt its address-range index, containment traversal and deferred work queue with source attribution.
  • Include native closure storage and ixgo environments in the object graph while retaining the existing ELF/DWARF and interpreter adapters, alias preservation and validated host writeback.
  • Remove wire, public type registration and load callbacks. Encode with w.put and image offsets; add a private typed-object directory so decoding allocates or binds all objects before restoring references.
  • Route the host and guest through private encode / decode entry points and remove the old codec files. The Sentry backend, interceptor, C bridge and host dependencies are unchanged.
  • Verify Linux ARM64 and emulated AMD64 transfer suites, the real ARM64 Sentry smoke and macOS package tests. New regressions cover struct/array interior pointers, a 1024-link pointer chain, zero-sized values and malformed object directories. Native AMD64 Sentry execution remains unverified.

This gives closure transfer a shared object graph implementation without adding a gVisor dependency to the host module.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 both r.indexed and bound are true, env is first derived from the object directory and then unconditionally overwritten with c.env here. The directory-derived slice is discarded, so the earlier lookup is wasted work when bound. Consider skipping the directory lookup in the bound case or adding a comment clarifying intent.

Comment thread state_decode_linux.go Outdated
)

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()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Comment thread state_objects_linux.go Outdated

// 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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Comment thread state_transfer_linux.go Outdated
@@ -73,13 +73,32 @@ func (im *valueImage) inherit(from *valueImage) {

// encode writes the root, followed by roots retaining every imported object.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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...".

Comment thread state_ixgo_linux.go Outdated
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}]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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).

Comment thread state_decode_linux.go Outdated
if addr != 0 {
return fmt.Errorf("%s: unexpected non-nil %s", path, t.Kind())
}
default:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

@MeteorsLiu
MeteorsLiu merged commit 7bedc37 into xgo-dev:main Sep 16, 2026
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.

1 participant