[asterisk-dev] [Code Review] app_meetme.c conf_run() leaks refs

[Russell Bryant]

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

Ship it!


This looks right to me.

I think the patch that went in related to this went in to all branches (1.4, 1.6.2, 1.8, trunk), so check all of them when you go to merge it.  Information on merging between branches can be found on the wiki.

https://wiki.asterisk.org/wiki/display/AST/Subversion+Usage

- Russell


On 2010-11-21 11:32:58, kobaz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1025/
> -----------------------------------------------------------
> 
> (Updated 2010-11-21 11:32:58)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There's two conditions (original lines 2626, 3675) where in the unlikly event of an ao2 alloc failing, we bail from conf_run and never do any cleanup.  This leaks a ref to an ast_conf_user.
> 
> Also there's a problem with a misuse of the user* pointer while in the *8 submenu (original line 3209), we lose the original *user pointer which will prevent proper cleanup on leave.
> 
> And lastly, this part at the bottom looks like it will never run.  It looks like if we get to this point, user->user_no will always have been properly initialized... and why would we want to decrement the ref count only if user_no is invalid?  We should always clean up at the end of this function.
> 
> 3699		if (!user->user_no) {
> 3700			ao2_ref(user, -1);
> 
> So far, I think that everything in the else block of the above if could be always done on cleanup.
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_meetme.c 295784 
> 
> Diff: https://reviewboard.asterisk.org/r/1025/diff
> 
> 
> Testing
> -------
> 
> -- join
> 0xba69cb8 =1   app_meetme.c:1204:build_conf ()
> 0xb970d38 =1   app_meetme.c:2247:conf_run ()
> 0xb970d38 +1   app_meetme.c:2427:conf_run () [@1]
> -- leave
> 0xb970d38 -1   app_meetme.c:3753:conf_run () [@2]
> 0xb970d38 -1   app_meetme.c:3756:conf_run () [@1]
> 0xba69cb8 -1   app_meetme.c:1849:conf_free () [@1]
> 0xba69cb8 **call destructor** app_meetme.c:1849:conf_free ()
> 
> 
> Thanks,
> 
> kobaz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20101121/41d041a4/attachment.htm