[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/#review2954
-----------------------------------------------------------


I see multiple places that goto outrun before setting the user->user_no field.  I think the conditional check of doing some of that cleanup makes sense.  However, regardless of whether or not user_no is set, I think the ao2_ref() should just be moved down to the end.

- Russell


On 2010-11-20 22:19:51, kobaz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1025/
> -----------------------------------------------------------
> 
> (Updated 2010-11-20 22:19:51)
> 
> 
> 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 295123 
> 
> 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/14ad5693/attachment-0001.htm