plug memory leaks on error paths (adversarial review)#54
Merged
Conversation
…aths Adversarial review surfaced several leak patterns on rare error paths. - channel_rs: publish/attachRelaySession freed uninitialized member_ids and stats when allocator.dupe failed partway through the loop. Track a populated counter and only free the prefix that was actually filled. - wire/broadcast: decodeBcast/decodeSess leaked the just-decoded inner buffer when a duplicate oneof field was rejected; free it before returning MalformedProtobuf. - wire/rs: decodePreamble leaked the per-iteration hash buffer if hashes.append failed, and leaked the owned hashes slice if the default-hash dupe failed during the return. Add errdefer for both. - discv5/protocol: six encoders used try items.append(a, try enc(a, ..)) which leaks the inner buffer if the list grow fails. Add an appendOwned helper that frees the buffer on append error. Also add errdefers in decodeFindNode/decodeNodes so partially populated lists are freed on error. - enr: same nested-try leak pattern in buildContent/sign; add an appendOwnedItem helper and route all appends through it. - transport/zquic_quic_shim: streamMake/streamMakeUni leaked the QuicStream allocation if streams_owned.append failed. Add errdefer for alloc.destroy and qs.deinit between create and append. - sim/gossipsub_rpc_pb: decodeIHaveOwned, decodeIWantOwned, decodePruneOwned and decodeRpcOwned leaked the just-decoded child (dupe'd bytes or owned sub-struct) when the outer list append failed. Register an errdefer on the intermediate value before append. Tests: zig fmt --check . plus test, test-broadcast, test-sim-rs, test-sim-gossipsub and test-quic all pass locally.
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
Adversarial review of the codebase surfaced a cluster of rare-error-path memory leaks and one undefined-behavior case. All of them share the same shape: an allocation succeeds but a follow-up step (usually
ArrayList.append, a validation check, or a secondary allocation) fails and the intermediate allocation escapes cleanup.Fixes
broadcast/channel_rs-publish/attachRelaySessionallocatedmember_idsandstatsarrays, then populated them in a loop. Theerrdeferiterated the full array, so a mid-loopallocator.dupefailure would free uninitialized slots (UB). Track apopulatedcounter and only freemember_ids[0..populated].wire/broadcast-decodeBcast/decodeSessdecoded a nestedinnerbuffer and then rejected duplicate oneof fields without freeing it. Freeinnerbefore returningerror.MalformedProtobuf.wire/rs-decodePreambleleaked the per-iteration hash buffer ifhashes.appendfailed, and leaked the ownedhashesslice if the final default-hashdupefailed. Add anerrdeferfor the per-iteration buffer and restructure the return sohashes_ownedis covered before the default-hash allocation.discv5/protocol- Six encoders (encodePing,encodePong,encodeFindNode,encodeNodes,encodeTalkReq,encodeTalkRes) used the leakytry items.append(a, try enc(a, ..))pattern. Introduce anappendOwnedhelper that frees the buffer on append failure and route every append through it. Also adderrdefercleanup todecodeFindNode/decodeNodesso partially populated temporary lists are freed on error.discovery/enr- Same nested-try pattern inbuildContentandsign. Add anappendOwnedItemhelper and route all appends through it.transport/zquic_quic_shim-streamMake/streamMakeUnileaked theQuicStreamallocation ifconn.streams_owned.appendfailed. Adderrdefer alloc.destroy(qs)anderrdefer qs.deinit()between create and append.sim/gossipsub_rpc_pb-decodeIHaveOwned,decodeIWantOwned,decodePruneOwnedanddecodeRpcOwnedleaked the freshly decoded child (duped bytes, or an owned sub-struct with its own nested allocations) when the outer listappendfailed. Register anerrdeferon the intermediate value before appending.No public API changes, no dependency changes, no behavior change on the happy path.
Test plan
zig fmt --check .zig build test(150 passed)zig build test-broadcast(54 passed)zig build test-sim-rs(22 passed)zig build test-sim-gossipsub(23 passed)zig build test-quic(44 passed)