transfer ownership on a channel try_send, not just send#538
Merged
Conversation
a channel send hands its value to whoever receives it — on another task,
usually after the sender's frame is gone. the emitter knew that for
`send` and gave the value a count on the way in, but `try_send` was
never in the list. so a try_send of a heap value either released an
owned temporary the moment the call returned, or (for a borrowed value
like `wire.ok`) never took a count at all. either way the receiver ends
up holding memory nobody is keeping alive.
std.net.http2's send_goaway hits exactly that shape:
wire := frames.frame_bytes(frame.ok)
client.outbound.try_send(wire.ok)
the GOAWAY frame bytes went to the writer task with no count. it did not
crash only because the `wire` result local leaked its payload, which
kept the block alive by accident. releasing that payload — the point of
the cascade work on this branch — turns the leak into a use-after-free:
valgrind reports a 48-byte Bytes header freed on the main thread inside
Client.close and then read by writer_loop/drain_coalesced.
fixing the ownership makes both halves correct, and the cascade
whitelist can now be widened to `.ok` with memcheck still clean. the
whitelist stays narrow here so the two changes can be judged apart.
a try_send that fails leaves the extra count behind. that is a bounded
leak, and the same trade the blocking form already makes.
Claude-Session: https://claude.ai/code/session_01Uzg5VKBucCeHxan34VAsjv
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
a channel send hands its value to another task, usually after the sender's frame is gone, so the value has to arrive with a count of its own. the emitter knew that for
sendand did not know it fortry_send: the name list that drives channel-send ownership only matchedsend/pith_channel_send. two things silently did not happen fortry_send:ch.try_send(wire.ok)) got no compensating retain, so the receiver held a value nobody ownedeither way the receiving task reads freed memory.
pith_channel_sendstores a barei64with no refcount awareness, so the count has to come from the emitter.the fix is one predicate,
ir_is_channel_send_name, used at the two places that decide channel-send ownership.try_sendnow behaves exactly likesend. atry_sendthat fails leaves the extra count behind — a bounded leak, and the same trade the blocking form already makes.how it showed up
std.net.http2'ssend_goawayqueues its frame withclient.outbound.try_send(wire.ok). under valgrind that is a 48-byteBytesheader freed on the main thread (Client_close→send_goaway, the sender's exit-edge release) and then read from the spawned writer task (writer_loop→drain_coalesced→pith_bytes_len).it had been masked. the payload used to leak, and the leak kept the block alive; once a recent change made that payload actually get released, the dangling read surfaced. the bug is older than that change and independent of it — this 20-line program reproduces it on unmodified
main:what was tested
tests/cases/test_channel_try_send_ownership— new, wired intoMEMCHECK_CASES, covering both shapes (an owned temporary and a.okread). built with a compiler that lacks the fix it reportsInvalid read of size 4; with the fix it is clean.make memcheck— all cases clean, includingtest_http2_tls_roundtrip, which fails 3/3 without this.make run-regressions236/236,make green-testsgreen,make bootstrap-verifyfixed point,pith fmt --checkclean.notes
two adjacent gaps found and deliberately left alone:
ir_emit_select_send_probeemitspith_channel_try_senddirectly, bypassing the method-call path, so aselectsend arm still takes no count. it needs different handling — a probe that fails must leave the value usable — so it is not a copy of this fix.std/net/http2/server.pith:403works aroundtry_send"not copying a struct payload correctly" with a blockingsend. part of that is this missing retain; the aliasing half looks like the zero-capacity pending-value slot, a separate runtime question. worth re-testing that workaround now.separately: with this fixed, widening the result-local cascade to
.okreads is memcheck-clean and regression-green. that is a bigger call — it changes ownership for every result local in the codebase — so it belongs in its own change with its own verification, not here.https://claude.ai/code/session_01Uzg5VKBucCeHxan34VAsjv