<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1025/">https://reviewboard.asterisk.org/r/1025/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Russell</p>
<br />
<p>On November 20th, 2010, 10:19 p.m., kobaz wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By kobaz.</div>
<p style="color: grey;"><i>Updated 2010-11-20 22:19:51</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">-- 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 ()
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/apps/app_meetme.c <span style="color: grey">(295123)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1025/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>