Skip to content

plug memory leaks on error paths (adversarial review)#54

Merged
ch4r10t33r merged 1 commit into
mainfrom
fix/adversarial-review
Apr 16, 2026
Merged

plug memory leaks on error paths (adversarial review)#54
ch4r10t33r merged 1 commit into
mainfrom
fix/adversarial-review

Conversation

@ch4r10t33r
Copy link
Copy Markdown
Collaborator

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 / attachRelaySession allocated member_ids and stats arrays, then populated them in a loop. The errdefer iterated the full array, so a mid-loop allocator.dupe failure would free uninitialized slots (UB). Track a populated counter and only free member_ids[0..populated].
  • wire/broadcast - decodeBcast / decodeSess decoded a nested inner buffer and then rejected duplicate oneof fields without freeing it. Free inner before returning error.MalformedProtobuf.
  • wire/rs - decodePreamble leaked the per-iteration hash buffer if hashes.append failed, and leaked the owned hashes slice if the final default-hash dupe failed. Add an errdefer for the per-iteration buffer and restructure the return so hashes_owned is covered before the default-hash allocation.
  • discv5/protocol - Six encoders (encodePing, encodePong, encodeFindNode, encodeNodes, encodeTalkReq, encodeTalkRes) used the leaky try items.append(a, try enc(a, ..)) pattern. Introduce an appendOwned helper that frees the buffer on append failure and route every append through it. Also add errdefer cleanup to decodeFindNode / decodeNodes so partially populated temporary lists are freed on error.
  • discovery/enr - Same nested-try pattern in buildContent and sign. Add an appendOwnedItem helper and route all appends through it.
  • transport/zquic_quic_shim - streamMake / streamMakeUni leaked the QuicStream allocation if conn.streams_owned.append failed. Add errdefer alloc.destroy(qs) and errdefer qs.deinit() between create and append.
  • sim/gossipsub_rpc_pb - decodeIHaveOwned, decodeIWantOwned, decodePruneOwned and decodeRpcOwned leaked the freshly decoded child (duped bytes, or an owned sub-struct with its own nested allocations) when the outer list append failed. Register an errdefer on 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)

…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.
@ch4r10t33r ch4r10t33r marked this pull request as ready for review April 16, 2026 20:18
@ch4r10t33r ch4r10t33r merged commit 6ed1a9d into main Apr 16, 2026
6 checks passed
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