release extraction-consumed result payloads and retain borrowed unwrap_or fallbacks - #622
Merged
Conversation
…ions unwrap_or, catch, and ! on a named result local all leave the shell holding the count it was built with: the ok arm retains a fresh count for the value it hands out, and the err arm keeps the error in place. the local's exit cleanup was supposed to release that count, but any of those uses disqualified the local from the payload cascade, so cleanup freed only the three-slot shell and one heap payload (or one heap error string) leaked per extraction — about 110 bytes a round for a small bytes payload, linear forever. the fix whitelists the three extraction forms in the cascade scan, gated to result-typed locals: the optional unwrap_or transfers a borrowed subject's count out instead of retaining a new one, so cascading an optional local would release a count it no longer holds. a mention of the name inside the fallback argument still disqualifies. a rebind of one name to results of different payload types stays shell-only (the recorded tid conflicts), which remains a documented safe under-fix.
the fallback arm of unwrap_or (result err, optional none) stored the default straight into the value it hands its consumer. the consumer treats a heap-kind unwrap_or value as owned and releases it, so a default read from a named local was handed out without a count of its own: the release stripped the owner's count and the owner's next use read freed memory. valgrind catches it with the struct freelist off; with the freelist on the recycled block makes the numbers come out right, which is how it stayed hidden. both arms now retain a borrowed heap default before the store, the same compensation the catch fallback has always made. string defaults stay on the borrowed classification (the escape site retains), so they are excluded. the new regression churns both the result and the optional shape and joins the curated memcheck list.
the new case churns unwrap_or, catch, and ! on a named result local, ok and err arms both, and joins the growth gate. before the cascade covered extraction uses it grew by about 147 mb across the gate's two round counts; flat now. the ownership doc's result-local bullet moves the extraction forms to the released side and names the remaining shape that still leaks: rebinding one name to results of different payload types.
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
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
awaiting a task result and extracting its payload leaked ~110 bytes per round. the await turned out to be irrelevant: the leak is in extracting from a result held in a named local.
r.unwrap_or(d),r catch d, andr!all classify a named operand as borrowed and leave the shell holding the count it was built with, for the local's exit cleanup to release — but any of those uses disqualified the local from the payload cascade, so cleanup freed only the three-slot shell and one heap payload (or one heap error string, on the err arm) leaked per extraction..okreads and flag probes were already whitelisted; the extraction forms never were.the fix whitelists the three extraction forms in the cascade scan, gated to result-typed locals — the optional
unwrap_ortransfers a borrowed subject's count out instead of retaining a new one, so cascading an optional local would over-release. a mention of the local inside the fallback argument still disqualifies.reading that code surfaced a second, worse bug in the same function: the fallback arm of
unwrap_or(result err, optional none) handed a borrowed heap default straight to a consumer that treats the value as owned.b := r.unwrap_or(fb)withfba named local strippedfb's count on the fallback path and the owner's next use read freed memory — the struct freelist recycled the block in place, so only valgrind with the freelist off sees it. both arms now retain a borrowed heap default before the store, the same compensation thecatchfallback has always made.probe numbers (spawn
Int!+Bytes!tasks, await both,unwrap_oreach; peak rss):the same shape without spawn/await leaked identically, and
catch/!variants and heap error strings leaked the same way; all flat after the fix.one shape stays on the leak side and is now documented in docs/ownership.md: rebinding one name to results of different payload types (
r := await tathenr := await tbwith differentT!s) — the cleanup path is keyed by name and cannot pick one payload shape, so it remains shell-only. bind results of different types to different names.what was tested
tests/leaks/leak_result_extraction(unwrap_or, catch, and!on a named result local, ok and err arms): grew ~147 mb across 200k→800k rounds before the fix, flat (52 kb) after; registered intooling/leak_check.shtests/cases/test_unwrap_or_borrowed_fallback(borrowed named fallback, result and optional): invalid reads under valgrind before, clean after, both backendsmake bootstrap-verify(before and after the seed refresh),make leak-check,make memcheck(72 cases, green and os-thread rounds, struct freelist off),make test,PITH_GREEN=0 make test, both corpora (284 passed each),make run-examples(99 passed),make docsite-checkexamples/grpc_chat.pithandexamples/grpc_reflect.pithrun to completionPITH_STRUCT_FREELIST=0on the ok and err arms: direct call, await, catch,!, heap error, named fallbacknotes
the borrowed-operand extraction convention (fresh count out, shell count stays home) was already written to be cascade-compatible — the try emitter's err arm even reads the error before cleanup so a cascade there is safe. the whitelist entries just make the cleanup side keep the promise the extraction side was relying on.