[asterisk-dev] [Code Review] Fix descriptor (and structure reference) leak when call is hungup before answer

[Russell Bryant]

> On 2010-03-30 12:11:59, Mark Michelson wrote:
> > /trunk/channels/sip/dialplan_functions.c, line 472
> > <https://reviewboard.asterisk.org/r/591/diff/1/?file=8936#file8936line472>
> >
> >     I contend that this test should be re-tooled as an external test. I have two main arguments for this.
> >     
> >     1. There is a dependency on an external application (SIPp). Internal tests should live entirely within the bounds of Asterisk.
> >     
> >     2. The scope of this test goes well beyond what is typically defined as a "unit test." While it does exercise a specific issue that has been resolved with this patch, the coverage of this test is much too broad to be considered a test of a unit of code.
> >     
> >     The tricky part of this is that the condition on which this test will pass or fail is something that is not necessarily easily detected outside of Asterisk. Specifically, it is not easy to tell from outside of Asterisk which address was used to allocate a sip_pvt and then test whether the memory at that address has been freed. In this particular test, there is a way to test externally whether the proper resources have been freed because you can check if the RTP port is still being used after the call has finished.
> >     
> >     This test has opened the door to discussion of a good method of externally monitoring the memory usage of Asterisk. I, unfortunately, am not exactly a fount of knowledge in this particular area, but I would be willing to help with the necessary research.
> 
> Tilghman Lesher wrote:
>     Unfortunately, this cannot be retooled as an external test, because access is required to the internals of chan_sip, to areas which cannot be seen from any external interface at this point in time.  Internal access is needed to verify that the test is successful.

The issue brought up here with deciding the best approach to writing a test is very interesting to me.  I think that this is a very important architectural issue that we will continue to face as we write tests.  The line between what should be inside and outside of Asterisk is not yet very well defined.  

In this case, it is almost entirely clear to me that it should be an external test.  Having a pre-defined dialplan that gets installed and a script that runs the sipp scenario is _much_ easier via an external script than from within Asterisk.  Furthermore, with an external test, it can be applied to all versions of Asterisk and not just Asterisk trunk.  

The part that is not immediately obvious to accomplish externally is the approach for detecting success.  The current code watches the reference count of an object.  What you need is a way to detect that the object has been destroyed from outside of Asterisk.

As we discussed this test yesterday, an idea was proposed that would allow you to detect if the data associated with the call has been appropriately torn down.  Get Asterisk to send you an SDP so that you know what ports it has reserved for RTP and RTCP.  Then, after the call is over, attempt a socket() and bind() on that port.  This is an especially appropriate solution given that the bug report itself was about ports not being closed.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/591/#review1776
-----------------------------------------------------------


On 2010-03-25 23:34:39, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/591/
> -----------------------------------------------------------
> 
> (Updated 2010-03-25 23:34:39)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> In trunk and current 1.6.x releases, if a SIP channel is hungup prior to being answered, then a reference leak results, which causes RTP ports not to be freed.
> 
> The fix itself is a one-liner, but the additional changes were needed in order to build a regression test for this scenario.
> 
> 
> This addresses bug 16774.
>     https://issues.asterisk.org/view.php?id=16774
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 254405 
>   /trunk/channels/sip/dialplan_functions.c 254405 
>   /trunk/channels/sip/include/devices.h PRE-CREATION 
>   /trunk/channels/sip/include/globals.h 254405 
> 
> Diff: https://reviewboard.asterisk.org/r/591/diff
> 
> 
> Testing
> -------
> 
> Unit test written and tested.  There is one error that running the test still causes, but it's cosmetic and should be able to be tracked down fairly easily.
> 
> 
> Thanks,
> 
> Tilghman
> 
>