fix(router): release packets Router::send() declines instead of leaking them - #11231
fix(router): release packets Router::send() declines instead of leaking them#11231ndoo wants to merge 1 commit into
Conversation
…ng them Router::send() leaked p on the invalid-payload-variant BAD_REQUEST early return (every other early return in the function releases first), and assert(iface) hung forever with no diagnostic on STM32WL instead of NAK'ing like every other error path here. Separately, ERRNO_SHOULD_RELEASE is a documented contract: RadioLibInterface::send() can decline to send and return it without releasing p, expecting the caller to release (MeshService::sendToMesh() already does this correctly). NextHopRouter::relayOpaquePacket() and both MeshBeaconModule::sendBeaconPacket() call sites ignored the return value and leaked p whenever send() declined - e.g. the p->to == NODENUM_BROADCAST_NO_LORA path in RadioLibInterface::send(). NextHopRouter::setNextTx() had its own separate assert(iface), same hang risk as Router::send()'s. The sibling leak/hang sites already flagged in this same audit - NextHopRouter::shouldFilterReceived()'s rebroadcast path and all five call sites in doRetransmissions() - turned out to already be fixed upstream; verified against a clean upstream/develop checkout rather than re-fixing already-fixed code. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Closing — the One fix in this PR isn't covered by #11223: |
Problem
Two related packet-lifecycle bugs in
src/mesh/Router.cppand its callers, both against the STM32WL dynamicpacketPoolcontext from the ongoing memory-safety audit (#11230 and follow-ups):Router::send()leakedpon the invalid-payload-variantBAD_REQUESTearly return — every other early return in this function releases the packet first.assert(iface)(both inRouter::send()and separately inNextHopRouter::setNextTx()) hangs forever with no diagnostic on STM32WL instead of NAK'ing/logging like every other error path in the same file.ERRNO_SHOULD_RELEASEis a documented contract:RadioLibInterface::send()can decline to send a packet and return this code without releasing it, expecting the caller to release (MeshService::sendToMesh()already does this correctly).NextHopRouter::relayOpaquePacket()and bothMeshBeaconModule::sendBeaconPacket()call sites ignored the return value entirely and leakedpwheneversend()declined — e.g. thep->to == NODENUM_BROADCAST_NO_LORApath inRadioLibInterface::send().I also traced
NextHopRouter::shouldFilterReceived()'s rebroadcast path and all five call sites indoRetransmissions(), which looked like the same bug in an earlier audit pass — those turned out to already be fixed upstream (they already check forERRNO_SHOULD_RELEASEand release correctly). Verified against a cleanupstream/developcheckout rather than re-fixing already-fixed code.Fix
Router::send(): releasepbefore theBAD_REQUESTreturn; convertassert(iface)to the sameabortSendAndNak()pattern already used for every other error in this function.NextHopRouter::setNextTx(): convertassert(iface)to a logged early return.NextHopRouter::relayOpaquePacket(),MeshBeaconModule::sendBeaconPacket()(both call sites): check thesend()return value and release onERRNO_SHOULD_RELEASE, matchingMeshService::sendToMesh()'s existing correct handling.Test plan
pio run -e wio-e5/pio run -e rak3172— build clean.--infohealthy before/after).p->to == NODENUM_BROADCAST_NO_LORAviaRadioLibInterface::send(), opaque-relay mode, beacon broadcast under a declined send) are narrow, protocol-level conditions that weren't independently reproduced live in this pass — the fix is a mechanical return-value check mirroring the already-shipped-correctMeshService::sendToMesh()pattern in the same file.🤝 Attestations