release a result or optional local's payload when it is provably the sole owner#537
Merged
Conversation
…safe a `T!`/`T?` bound to a local lowers to a 3-slot tuple whose slot 8 (and slot 16 on the error side) owns its payload. the exit cleanup and the rebind path only released the shell via pith_struct_release, so the payload leaked — about 300 bytes per iteration for a loop doing `r := make(); if r.is_err: ...; b := r.ok`. a cascading destructor on the tuple itself is not safe: the many sites that extract the payload and then release the shell (`!`, catch, unwrap_or, if-let, match, for-iter, the json/toml decoders) would double-free. instead the cascade only fires for tracked tuple locals whose EVERY use is a `.ok`/`.err` read (which retains, leaving the tuple's own count intact) or a flag check (`.is_ok`, `== none`, which only read slot 0). none of those release the shell, so cleanup stays the sole releaser. any other use — `!`, `?`, catch, unwrap_or, a bind, a call argument, a return, a match or a for — falls back to the shell-only release, which may still leak but can never double-free. the release is emitted as a call to a generated per-signature helper rather than inline branches: an inline brif would split the caller's basic block between a register's definition and its use (the freshly built value in a rebind, the return value at an exit edge). the helper also short-circuits on a null shell, which is what an unassigned local or a first loop iteration hands in. Claude-Session: https://claude.ai/code/session_01Uzg5VKBucCeHxan34VAsjv
…ocal the prologue retains a tracked param's shell so the exit release stays balanced with the caller, but the payload behind slot 8 belongs to the caller's tuple. cascading one would drop a count the caller still holds. params record no producing type id today, so this was only reachable by shadowing a `T!` param with a same-named local bind — cheap to rule out outright.
valgrind on tests/cases/test_http2_tls_roundtrip (the curated memcheck gate) turned up an invalid read once `.ok` reads were treated as cascade-safe: baseline passes 3/3, with `.ok` whitelisted it fails 3/3 with a use-after-free of a 48-byte block, freed on the main thread and then read from a spawned server task. the counts balance locally — `r := attempt.ok` does emit a retain, and the two releases at the exit edge match the two counts — so the cascade itself is not obviously wrong. what changes is that the ok payload is now actually FREED where it previously leaked, and something in the http2 path outlives it. that is a latent ownership bug the leak was hiding, not something this change can settle safely. so `.ok`/`.err` come back out of the whitelist. what is left is the flag reads (`.is_ok`, `.is_err`, `== none`, `!= none`), which only touch slot 0 and never hand the payload to anyone — a tuple local whose value is never extracted is unambiguously the sole owner. that still fixes the optional repro (295MB -> flat) and any result local that is only ever probed, and leaves the `.ok` case leaking exactly as before. an under-fix that leaks beats a use-after-free.
a re-measurement of the threaded server puts per-request growth at ~2.8 kb (218 mb over 78k requests), not the ~0.8 kb this document has carried since july. the old figure understated it, and the growth now has a cause worth naming rather than being filed as "a real per-request leak that is still unfixed": serve_connection reads each request into a `T!` local and reads it back with `.ok`, and such a local is released as a bare three-slot shell, leaking the HttpRequestBytes it owns. performance.md gains an entry for what the reclamation does fix, with the million-iteration numbers (295 mb -> 2.5 mb for a probed optional, 295 mb -> 2.7 mb for a probed result, unchanged for a `.ok` read) and the reason `.ok` is left alone. ownership.md's list of known gaps gains the same case, since that list is where someone would look for it. throughput numbers are left as they were: this machine had been running benchmarks all session and is in no state to produce an honest req/s. growth-per-request is a ratio and survives that.
the fmt gate wants the two new tracking globals' trailing comments a single space off the declaration, not column-aligned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
a
T!orT?bound to a name lowers to a three-slot heap value (flag, payload, error). releasing one freed those three slots and nothing else, so the payload it owned was never dropped. a loop binding a fallible call a million times grew to ~295 mb; the same loop writtenx := call()!stayed flat, because!hands the payload's count to the caller instead of leaving it in the tuple.such a local now releases its payload — but only when every use of it is provably safe. today that means the flag reads (
.is_ok,.is_err,== none,!= none), where the value is never handed to anyone and the local is unambiguously its sole owner. anything else keeps the old shell-only release.== none.is_err.okwhy
.okis deliberately excludedthis is the interesting part, and the reason the whitelist is narrow.
treating
.okas cascade-safe looks right on paper — the read does emit a retain, and the releases at the exit edge match the counts. but valgrind ontests/cases/test_http2_tls_roundtrip(the curated memcheck gate) then fails 3/3 with a use-after-free: a 48-byte block freed on the main thread and read afterwards from a spawned server task. baseline passes 3/3.the cascade is not what's wrong. what changes is that the payload is now genuinely freed where it previously leaked, and something in the http/2 path outlives it. the leak had been hiding an ownership bug. that bug is real and still there; this change simply declines to convert it into memory corruption. an under-fix that leaks beats a use-after-free, so
.okwaits until the http/2 ownership issue is fixed on its own.a result/optional parameter is likewise never cascaded — it is a borrow owned by the caller.
docs
docs/performance.md's http-server figure has read "~0.8 kb/request ... a real per-request leak that is still unfixed" since july. a re-measurement puts it at ~2.8 kb/request (218 mb over 78k requests), and the cause is the leak above —serve_connectionreads each request into aT!local and reads it back with.ok, leaking the wholeHttpRequestBytesevery request. the entry now says so, and gains the reclamation numbers.docs/ownership.md's known-gaps list gains the same case.throughput numbers are left untouched: this machine had been running benchmarks all session and is in no state to produce an honest req/s. growth-per-request is a ratio and survives that.
what was tested
make memcheck— all cases clean, includingtest_http2_tls_roundtrip, the one that fails under a wider whitelist. this is the load-bearing check.make bootstrap-verify— ir fixed point verified (the seed is regenerated here).make run-regressions— 235 passed,make run-examplesandmake green-testsgreen.!,?,catch,unwrap_or,or_else,.ok/.err, if-let, while-let, match, for-iter, index-propagate, discarded call, passing and returning a result/optional) — no invalid free, no invalid read/write.pith run.notes
this does not make the http server's rss flat — that needs the
.okcase, which is blocked on the http/2 ownership bug above. that bug is the next thing worth fixing, independently of the leak: it is live today.https://claude.ai/code/session_01Uzg5VKBucCeHxan34VAsjv