Skip to content

fix: memory-safety audit — container/record ownership + intern ref leak#15

Merged
lang315 merged 6 commits into
mainfrom
fix/marshal-ownership-audit
Jul 3, 2026
Merged

fix: memory-safety audit — container/record ownership + intern ref leak#15
lang315 merged 6 commits into
mainfrom
fix/marshal-ownership-audit

Conversation

@lang315

@lang315 lang315 commented Jul 3, 2026

Copy link
Copy Markdown
Owner

What

Memory-safety audit of the gotk4 Go↔C marshaling generator and the hand-written pkg/core runtime. Four confirmed reference-counting bugs fixed, each pinned by a failing-first regression test, plus baseline benchmarks for the previously-unmeasured signal-dispatch hot path. GTK4 binding coverage re-verified at 100% (677/677 bindable types).

Every fix was driven adversarially: four parallel review agents (typeconv, pkg/core, girgen, perf), then a second /simplify pass (reuse, simplification, efficiency, altitude). Findings were verified against committed generated output before fixing.

Bugs fixed

  1. Callback out-param GHashTable double-free / over-unref (cc72a81b0) — a hash table written to a vfunc out-parameter is created with g_hash_table_new_full(…destroy…), so the table owns its keys and values. The out-param-in-callback path computed ShouldFree()==true for the inners, emitting defer C.free on every key (double-free with the table's key-destroy) and skipping g_object_ref/g_variant_ref on values (over-unref when C destroys the table). New ConversionValue.OwnedByContainer forces ShouldFree false for container-owned inners. Manifested in gio MenuModel get_item_links / get_item_attributes.

  2. Transfer-full refcounted records handed to C with no ref (cc72a81b0) — GVariant/GskTransform/PangoAttrList/… passed to C with transfer-full got neither a reference nor a finalizer detach, so the C consumer's unref raced the Go finalizer's unref (use-after-free). The Go→C record path now takes its own reference when C assumes ownership, mirroring the existing Class/Interface behavior.

  3. Floating GVariant not sunk (cc72a81b0 + 3f644089f) — taking a reference on a received/passed record now prefers ref_sink over ref when available (both directions, after the /simplify altitude pass caught the Go→C side lagging). Identical to ref on non-floating instances; on a fresh g_variant_new_* result it claims the floating reference instead of leaving the flag set — matching what intern.Get already does for GObjects.

  4. intern.Get leaks a ref on transfer-full return of an interned object (f4b4332ea) — Get(take=false) (glib.AssumeOwnership, every transfer-full return) did the reference-transfer unref only on the new-box path. Both already-interned paths returned early without it, permanently leaking one ref per repeated transfer-full get of the same object (e.g. every gio.ListModel.Item(i) on the same item) and pinning it strong forever — never reaching the toggle's is_last state, never finalized.

Plus 3d1ef525d: union CastBlock now imports runtime unconditionally (a field-less union with copy() generated runtime.SetFinalizer without the import — non-manifest in the 4.22 corpus, certain from the path).

Evidence

Regression tests fail on the pre-fix tree and pass after — verified by stashing each fix and re-running:

=== new generator pin tests (in-memory GIR fixtures, no system GIR) ===
--- PASS: TestCallbackOutParamHashTableOwnership   (fails pre-fix: spurious defer C.free)
--- PASS: TestRecordTakeRefPrefersRefSink          (fails pre-fix: plain g_variant_ref)
--- PASS: TestUnionRuntimeImport                   (fails pre-fix: runtime used, not imported)

=== intern refcount regression (reads real GObject ref_count via cgo) ===
--- PASS: TestGetTransferFullAlreadyInterned       (fails pre-fix: refcount 3, want 2)
--- PASS: TestGetTransferNoneAlreadyInterned

=== root module suite ===
ok  github.com/diamondburned/gotk4/gir/girgen
ok  github.com/diamondburned/gotk4/gir/girgen/generators
ok  github.com/diamondburned/gotk4/gir/girgen/strcases
ok  github.com/diamondburned/gotk4/gir/pkgconfig

=== GTK4 coverage guard ===
TOTAL: 677/677 bindable GTK4 types covered (100.00%)
--- PASS: TestGTK4Coverage

Bindings regenerated in the canonical Docker/Nix environment (GTK 4.22.4) and verified:

go generate                     rc=0   (all diff hunks are the intended ref/ref_sink additions)
go build -C pkg ./...           rc=0   (Linux, full tree, in Docker)
goimports -l pkg gir            (empty — clean)
go generate (2nd run)           rc=0, git status clean   (idempotent — passes CI git-dirty gate)
pkg/core + cairo + gota tests   green (macOS, brew GTK)
examples/telegram build         rc=0 (against regenerated bindings)

Also in this branch

  • Baseline benchmarks for signal dispatch + GValue marshaling (f30294682) — the previously-unbenchmarked per-frame hot path. Confirms the intern layer is not a bottleneck (~23 ns/op, 0 allocs steady-state) and documents the reflect-path and G_TYPE_OBJECT double-marshal candidates as deferred behind a profile-first bar (repo's no-speculative-opt rule).
  • /simplify pass (3f644089f) — deduped the generator-test harness (generateNamespace → delegates to generateNamespaceDeps), mirrored ref_sink to the Go→C direction, and documented the container-ownership rule + the deliberately-unfixed callback flip so the next maintainer doesn't "fix" it and regress the C→Go paths.

Not fixed (noted for follow-up)

Latent items the pinned corpus doesn't exercise, out of scope for this PR: GArray/GByteArray C→Go converters (no *_steal binding today), the toggle-ref resurrection race in intern (needs an atomic finalize/resurrection check — the area has a reverted-fix history), the slab once-path copy, and four girgen hardening items (constant-name sanitization, record field-getter dedup, filter regex anchoring). The InternObject(x).Native() double-compute is a pre-existing ~56-site generator idiom; this branch adds one line following house style rather than diverging.

🤖 Generated with Claude Code

lang315 and others added 6 commits July 3, 2026 16:50
…ting records

Three ownership fixes in the Go<->C marshaling generator:

1. Values built into a GHashTable created with destroy functions are owned
   by the table. The out-parameter-in-callback path (vfunc exports) used to
   compute ShouldFree()==true for them, emitting a defer C.free on every
   key (double free with the table's key-destroy) and skipping the
   g_object_ref for object values (over-unref when the C caller destroys
   the table). New ConversionValue.OwnedByContainer forces ShouldFree to
   false for these inners; the hash-table builder now uses it for both
   keys and values.

2. Refcounted records (GVariant, GskTransform, PangoAttrList, ...) handed
   to C with transfer-full got neither a reference nor a finalizer detach,
   so the C consumer's unref raced the Go wrapper's finalizer unref
   (use-after-free). The Go->C record path now takes its own reference via
   the record's ref function whenever C assumes ownership, mirroring the
   existing Class/Interface behavior.

3. Taking our own reference on a received record now prefers ref_sink over
   ref when the record has one: identical for non-floating instances, and
   for floating ones (fresh GVariants from g_variant_new_*) it claims the
   floating reference instead of leaving the flag set - matching what
   intern.Get already does for GObjects.

Pinned by in-memory GIR fixtures: TestCallbackOutParamHashTableOwnership
(fails pre-fix with the spurious defer C.free) and
TestRecordTakeRefPrefersRefSink (fails pre-fix with plain g_variant_ref).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QKdQkPHq1L3jPLTpBkJbEq
The union CastBlock always emits runtime.SetFinalizer, but the "runtime"
import was only added inside the per-field conversion loop. A union with a
copy() method whose fields are all non-record/enum/bitfield generated code
referencing runtime without importing it. Non-manifest in the pinned GTK
4.22 corpus (the only generated union, GdkEvent in gtk/v3, has record
fields), but certain from the code path.

Pinned by TestUnionRuntimeImport (fails pre-fix).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QKdQkPHq1L3jPLTpBkJbEq
…terned box

intern.Get did the reference-transfer accounting only on the new-box path:
for take=false (glib.AssumeOwnership, i.e. every transfer-full return) it
unref'd the reference the C function handed us, because the box's toggle
reference already embodies our ownership. Both already-interned paths
returned early without that unref, permanently leaking one reference per
transfer-full return of an interned object - e.g. every repeated
gio.ListModel.Item(i) on the same item - and pinning the object strong
forever (it could never reach the toggle's is_last state, so it was never
finalized).

The unref runs outside shared.mu, like the new-box path, because it can
dispatch a toggle notify that takes the lock.

Pinned by pkg/core/internal/interncheck (cgo helpers cannot live in
_test.go files, so the package mirrors internal/cstrbench):
TestGetTransferFullAlreadyInterned observes refcount 3-vs-2 pre-fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QKdQkPHq1L3jPLTpBkJbEq
…ling

Adds core/glib/marshal_bench_test.go covering the layer above the slab/gbox
registries: _gotk4_goMarshal end to end (void fast path 641ns/4allocs,
reflect path 1587ns/8allocs via gio.SimpleAction "activate"), Object.Cast
(714ns), steady-state intern lookup (22.9ns/0allocs), and GValue
read/write. BENCHMARKS.md records the numbers and the decisions: the
intern layer is off the suspect list; typed per-signal marshalers and the
G_TYPE_OBJECT double-marshal fix are deferred behind a profile-first bar.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QKdQkPHq1L3jPLTpBkJbEq
go generate in the canonical Docker/Nix environment (GTK 4.22.4). All
changes are the two typeconv ownership fixes landing in the corpus:

- MenuModel get_item_links/get_item_attributes exports: the spurious
  defer C.free on hash-table keys is gone; values now take
  g_object_ref/g_variant_ref_sink for the table to own.
- Transfer-full refcounted-record parameters and vfunc returns now take a
  reference for the C consumer to steal (GskTransform's consuming
  transform API, gtk *ScrollTo GtkScrollInfo, g_application_add_option_group,
  GDir.Close, DBus/Resolver/Menu GVariant vfunc returns, PangoAttrList and
  PangoFontMetrics vfunc outputs, GdkContentFormats union methods).
- Every C->Go take-own-reference site on GVariant switched from
  g_variant_ref to g_variant_ref_sink, so floating variants from
  g_variant_new_* constructors are sunk on wrap (standard GI binding
  behavior; keeps the new transfer-full refs balanced).

Verified: go build -C pkg ./... in Docker (exit 0), goimports -l clean,
pkg core tests green on macOS, examples/telegram builds against the
regenerated tree.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QKdQkPHq1L3jPLTpBkJbEq
…ent ownership rules

/simplify pass over the branch:

- transfer_ownership_test.go's generateNamespace now delegates to
  generateNamespaceDeps (its strict superset) instead of duplicating the
  ~35-line parse/generate/concat bootstrap; the two copies had already
  drifted (logger block, gofmt comment).
- The Go->C record path now prefers ref_sink like the C->Go path does,
  removing a realized asymmetry: a user vfunc returning a fresh floating
  GVariant previously had it plain-ref'd (floating flag left set, one ref
  leaked once the C consumer sinks); now the sink claims the floating
  reference for C to own. 8 gio_export.go sites regenerate to
  g_variant_ref_sink; no-op for records without ref_sink.
- Documented the container-ownership rule on the OwnedByContainer field
  (destroy funcs => container owns; g_list_free frees nodes only => list
  elements follow transfer) and the deliberately-unfixed callback flip in
  ShouldFree (fixing it globally would regress C->Go borrow semantics).

Skipped by design: the pre-existing InternObject(x).Native() double-compute
idiom (~56 sites corpus-wide, this branch adds one following house style)
is a separate generator cleanup.

Verified: root suite green, Docker regen + go build -C pkg ./... rc=0,
goimports clean, second regen idempotent.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QKdQkPHq1L3jPLTpBkJbEq
@lang315
lang315 merged commit 39e9b46 into main Jul 3, 2026
15 checks passed
@lang315
lang315 deleted the fix/marshal-ownership-audit branch July 3, 2026 11:05
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