[asterisk-dev] [Code Review] 4303: res_pjsip_outbound_registration: Fix reference leak.

[George Joseph]

> On Jan. 5, 2015, 2:37 p.m., Mark Michelson wrote:
> > branches/13/res/res_pjsip_outbound_registration.c, lines 416-420
> > <https://reviewboard.asterisk.org/r/4303/diff/2/?file=70165#file70165line416>
> >
> >     So, this code change is "correct" in the sense that it is getting rid of an extra reference to client_state. However, I believe that the reference being removed here is a weird one to remove and actually makes the code a bit harder to follow.
> >     
> >     Every call to pjsip_regc_send() has an accompanying ref-bump of the client_state. This is because sip_outbound_registration_response_cb() is called when a response is received for the outgoing registration, and that function expects to decrement the bumped reference count.
> >     
> >     The problem with the code you've changed is that prior to the removed ref-bump, the reference count is already off by one, and removing the ref-bump corrects the count. What should be happening is that it shouldn't be getting off by one to begin with.
> >     
> >     Where is the refcount getting skewed? I believe I've figured it out. In schedule_registration(), the refcount of client_state is bumped by one. The expected corresponding decrement of this is in cancel_registration(), called from handle_client_registration(). The problem is that the decrement to the refcount only occurs if pj_timer_heap_cancel() returns non-zero. In our case, we're calling pj_timer_heap_cancel() on a timer entry that is currently firing and is not in the timer heap any longer. Therefore, pj_timer_heap_cancel() returns 0, and we do not decrement the refcount. This will happen for every registration that goes through the schedule_registration() function.
> >     
> >     So, my thoughts on this are that the correct fix is to leave this section as it is, and switch from calling cancel_registration() to performing an explicit decrement of the refcount on the timer for the case where the timer fires and calls sip_outbound_registration_timer_cb().

I'm not sure what you want me to do here. :(


> On Jan. 5, 2015, 2:37 p.m., Mark Michelson wrote:
> > branches/13/res/res_pjsip_outbound_registration.c, lines 889-895
> > <https://reviewboard.asterisk.org/r/4303/diff/2/?file=70165#file70165line889>
> >
> >     Where is the corresponding decrement of this ref-bump?
> >     
> >     Kevin is right that it makes sense for the PJSIP registration to own a reference to the client_state. However, I don't think in practice that this works. The reason is that Asterisk isn't notified when the PJSIP registration is destroyed, so we can't be sure to remove the reference to the client_state that we added here. Instead, we use the model where we bump the refcount of the client_state each time that we send a registration, and decrement it each time the registration callback is called.

Well, sip_outbound_registration_response_cb increments the ref count before scheduling handle_registration_reponse but handle_registration_response never unrefs it.  My original patch added the unrefs in handle_registration_response.  Are you suggesting I go back to the original patch?


- George


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


On Jan. 2, 2015, 7:04 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4303/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2015, 7:04 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Every time a registration started, sip_outbound_registration_response_cb bumps the ref count on client_state then pushes a handle_registration_response task.  handle_registration_response never unreffed it though.  So every time a registration goes out, the ref count goes up by one.
> 
> This patch adds the unreffs to handle_registration_response.
> 
> 
> Diffs
> -----
> 
>   branches/13/res/res_pjsip_outbound_registration.c 430163 
> 
> Diff: https://reviewboard.asterisk.org/r/4303/diff/
> 
> 
> Testing
> -------
> 
> Watched the ref count on client_state in no-auth registrations, auth-successful registrations and auth-failed registrations to make sure the ref count stayed stable.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150105/d5734429/attachment-0001.html>