Summary:	ASTERISK-28826: res_rtp_asterisk: Duplicate seqnos being added to send buffer with NACK
Reporter:	nappsoft (nappsoft)	Labels:	patch webrtc
Date Opened:	2020-04-14 06:39:00	Date Closed:	2020-04-20 11:41:37
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Resources/res_rtp_asterisk
Versions:	16.9.0	Frequency of Occurrence	Frequent
Related Issues:
Environment:		Attachments:	( 0) patch1.diff
Description:	When a packet is received twice (and though ast_data_buffer_put would report "Packet with position ... is already in buffer. Not inserting.") the payload will never be freed resulting in a memory leak. This behavior could be observerd with asterisk 16.9 during WebRTC conferences when some clients had bad connections and some packets needed to be repeated. Attached you'll find a patch that fixes the issue.
Comments:	By: Asterisk Team (asteriskteam) 2020-04-14 06:39:02.659-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. Please note that once your issue enters an open state it has been accepted. As Asterisk is an open source project there is no guarantee or timeframe on when your issue will be looked into. If you need expedient resolution you will need to find and pay a suitable developer. Asking for an update on your issue will not yield any progress on it and will not result in a response. All updates are posted to the issue when they occur. By: Joshua C. Colp (jcolp) 2020-04-14 06:56:49.970-0500 The code uses ast_data_buffer_get before the mentioned code is executed to see if the sequence number is already in the receive buffer, and if so doesn't proceed and outputs a log message. This handles the case where the same packet is received multiple times. Things are locked such that it should not be possible for the buffer to have been modified while this is happening. Can you explain further how the scenario you've mentioned is occurring? As well on sending the NACK retransmission of RTP packets does not use the same path as normal sending, so they aren't added to the send buffer again. By: nappsoft (nappsoft) 2020-04-14 07:06:22.639-0500 MALLOC_DBUG showed lots of the following lines after finishing the conference: rtp_raw_write() line  4924 of res_rtp_asterisk.c Without my patch I could reproduce this, with my patch not any longer. So it seems to do something at least, no voodoo here ;) By: Joshua C. Colp (jcolp) 2020-04-14 07:08:18.435-0500 Ah, that line is the send buffer and not the receive buffer. So it's something to do with sending, not receive. By: nappsoft (nappsoft) 2020-04-14 07:13:00.928-0500 Ah yes, you're right. Tbh: I am not even sure whether the mentioned output "Packet with position ... is already in buffer. Not inserting." was generated or not, it's just what I suspected as this seemed to be the only place in this function that made sense to not insert to packet. However: the thing with MALLOC_DEBUG and the fact that the patch fixed the problem is true. By: nappsoft (nappsoft) 2020-04-14 07:16:51.555-0500 However: I guess it makes sense to handle the error condition (the case when a packet has not been inserted for whatever reason) in both places... By: Joshua C. Colp (jcolp) 2020-04-14 07:24:12.343-0500 If it happens on the receive side then something is critically broken because the logic there already handles that case. We could be lenient in that case, but a contract has been broken in the implementation. By: Joshua C. Colp (jcolp) 2020-04-14 07:26:34.665-0500 Do you plan on putting this up for code review and inclusion? By: nappsoft (nappsoft) 2020-04-14 07:30:34.563-0500 Can do that tomorrow, yes. (Together with the other patch and the one that is still pending concerning fork() and exec() => ASTERISK-28776). By: Joshua C. Colp (jcolp) 2020-04-14 07:38:11.037-0500 Assigning to you since you'll be placing it up for review. By: Friendly Automation (friendly-automation) 2020-04-15 13:58:00.825-0500 Change 14240 merged by Friendly Automation: res_rtp_asterisk: Free payload when error on insertion to data buffer [https://gerrit.asterisk.org/c/asterisk/+/14240|https://gerrit.asterisk.org/c/asterisk/+/14240] By: Friendly Automation (friendly-automation) 2020-04-15 13:58:05.550-0500 Change 14239 merged by Friendly Automation: res_rtp_asterisk: Free payload when error on insertion to data buffer [https://gerrit.asterisk.org/c/asterisk/+/14239|https://gerrit.asterisk.org/c/asterisk/+/14239] By: Friendly Automation (friendly-automation) 2020-04-15 14:02:16.090-0500 Change 14207 merged by Friendly Automation: res_rtp_asterisk: Free payload when error on insertion to data buffer [https://gerrit.asterisk.org/c/asterisk/+/14207|https://gerrit.asterisk.org/c/asterisk/+/14207] By: Friendly Automation (friendly-automation) 2020-04-15 14:02:30.090-0500 Change 14238 merged by Friendly Automation: res_rtp_asterisk: Free payload when error on insertion to data buffer [https://gerrit.asterisk.org/c/asterisk/+/14238|https://gerrit.asterisk.org/c/asterisk/+/14238]