AmSession::postDtmfEvent: don't leak refused DTMF events - #567
Merged
Merged
Conversation
AmSession::postDtmfEvent() silently dropped the heap allocated AmDtmfEvent it was handed whenever m_dtmfDetectionEnabled is false, without ever deleting it. The event is allocated by the caller with new and ownership is only taken over when it is actually queued, so every refused event leaks. The RTP path makes this remotely triggerable: AmRtpStream::recvDtmfPacket() posts an AmRtpDtmfEvent for every telephone-event RTP packet that arrives, with no check on whether the session wants DTMF at all. A single DTMF digit is several RTP packets (plus the end retransmissions), so a peer sending DTMF to a session that disabled detection leaks steadily for the lifetime of the process. Applications shipped in this tree disable detection routinely (conference, voicemail, callback, b2b_connect, serviceline, announce_auth, cacheannounce, jukecall, DSM's disableDtmfDetection(), IVR), so this is a normal runtime configuration, not a corner case. The SIP INFO path (AmSession::onSipRequest) leaks the same way for application/dtmf-relay bodies. Make the AmDtmfSink::postDtmfEvent() contract explicit: return true when the sink took ownership, false when it refused, and release the event at the three call sites when it comes back refused. AmSession is the only implementation of AmDtmfSink in the tree.
Contributor
Author
|
It dies in Debian 11 (bullseye) reached end of security support, so its There is no fix for it in the tree yet to carry into this PR. Unblocking it needs a packaging-side change (bump the base image off bullseye, or pass The compile itself is fine: a full Generated by Claude Code |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Pull request overview
Fixes leaked heap-allocated DTMF events when detection is disabled by making ownership explicit.
Changes:
- Returns ownership status from
postDtmfEvent(). - Releases refused SIP, RTP, and detector-generated events.
- Documents the updated ownership contract.
File summaries
| File | Description |
|---|---|
core/AmSession.h |
Updates the DTMF API contract. |
core/AmSession.cpp |
Returns ownership status and handles SIP events. |
core/AmRtpStream.cpp |
Releases refused RTP events. |
core/AmDtmfDetector.h |
Updates the sink interface documentation. |
core/AmDtmfDetector.cpp |
Releases refused aggregated events. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add a test_dtmf suite for the ownership contract introduced by the previous commit: postDtmfEvent() returns whether the sink took the event over, and the caller releases what was refused. - AmDtmfDetector with a stub sink: an accepting sink is handed the aggregated event (key and duration), and a refusing sink does not wedge the detector, each pending key is still offered exactly once. - AmSession with detection disabled refuses raw SIP INFO, raw RTP and aggregated events, queues nothing, and once detection is enabled again only accepted keys reach onDtmf(). - AmSession with detection enabled takes all three kinds: aggregated events land in the session's event queue, raw SIP INFO and RTP events go through its DTMF detector and come out at onDtmf(). - AmSession::onSipRequest() for an application/dtmf-relay INFO answers 200 with detection disabled and enabled, and only the INFOs received while enabled are delivered. The session runs on an AmSipDialog that records replies instead of sending them. Keys are flushed out of the detector by sending a different key, so no test waits for the detector timeout. The release of a refused event at the call sites is not visible to assertions, because the event is allocated inside the code under test. A missing delete in AmDtmfDetector::reportEvent() or in AmSession::onSipRequest() is only reported as a leak by the asan job. AmRtpStream::recvDtmfPacket() is not covered, since it needs a stream with a negotiated telephone-event payload type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The bug
AmSession::postDtmfEvent()takes a heap-allocatedAmDtmfEvent*and only queues it whenm_dtmfDetectionEnabledis true. When detection is disabled it returns without queueing and without deleting — the event leaks. Callers allocate withnewand have no way to tell whether ownership was taken.Three call sites are affected:
AmRtpStream::recvDtmfPacket()AmSession::onSipRequest()application/dtmf-relaybodyAmDtmfDetector::reportEvent()The RTP path is the serious one:
recvDtmfPacket()posts an event for every telephone-event packet with no check on whether the session wants DTMF. One DTMF digit is several RTP packets plus the end-of-event retransmissions, so a remote peer sending DTMF into a session with detection disabled leaks continuously, for the lifetime of the process.This is not a corner case — disabling DTMF detection is a normal configuration. In-tree users:
apps/conference,apps/voicemail,apps/callback,apps/examples/b2b_connect,apps/examples/serviceline,apps/examples/announce_auth,apps/examples/cacheannounce,apps/examples/jukecall, DSM'sdisableDtmfDetection(), and the IVR binding.The fix
Make the ownership contract explicit.
AmDtmfSink::postDtmfEvent()now returnsbool— true when the sink took ownership, false when it refused — and the three call sites release the event when it comes back refused.AmSessionis the only implementation ofAmDtmfSinkin the tree, so the signature change is contained.Validation
Full
cmakebuild of core and all apps: clean, no new warnings.Credit
Backported from sipwise/sems commit
1ec76519("AmSession: do not leak heap allocatedAmDtmfEvent", MT#59962). Thanks to the Sipwise team for finding and fixing this; adapted to this tree'sAmSession/AmRtpStreamlayout.Generated by Claude Code