<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127840/">https://git.reviewboard.kde.org/r/127840/</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am having a hard time wrapping my head around why it uses a temporary object to begin with, rather than the static, but oh well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, I think, this is not actually a problem. The way the code is written makes it a problem though.
In pulseaudio callbacks the eol param basically is:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><0 = failure during whatever should have produced data for the callback
0 = not at end
>0 = at end, no more data
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, our callback essentially goes </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">if (eol < 0) {
// fail, delete userdata
return
}
if (eol) {
// end, deletes userdata
}
if (!info)
return // which is synonimous with if (eol), or rather, it should be.
// process !eol
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With that in mind the reason the static analysis thinks this is a problem is because this essentially depends on info being nullptr when eol is >0, which <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">should</em> be the case but isn't a very smart way to handle this IMO.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would propose:
- return inside if (eol) block
- Q_ASSERT(info) instead of the if (!info). If this code is run and we return in the eol case the only way we get there is if eol==0 and then we must have data.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Instead of implicitly linking eol's behavior with info's null-ness we explicitly make sure we have info when we aren't done processing callbacks. This should nicely solve the static analysis warning about use-after-free and at the same time make it more obvious what is going on with that return there.</p></pre>
<br />
<p>- Harald Sitter</p>
<br />
<p>On May 5th, 2016, 2:07 a.m. UTC, Michael Pyne wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Phonon.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated May 5, 2016, 2:07 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
phonon
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Coverity notes (CID 1336170) there's a potential use-after-free in the PulseAudio support code (pulsesupport.cpp:472 uses <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">u</code>, which may have been deleted at pulsesupport.cpp:408 if this was the last time the callback needed to be run).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Since there are some interesting git commits trying to troubleshoot corruption of the data being used at :472 (e.g. 23954b3c2ba3401f6c9843eb0490d7cc26598395, 71e136457c3a609b4af86de083d2dbb44a858f84 investigating a crash followed by 2671a170bef5196d55649a26a9cd5e108acb931b removing some of the extra asserts), I'm assuming this has actually happened at least some of the time.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The problem with fixing is that the lifetime of the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">userdata</code> must be dynamic since (from what I can tell), the callback can be called multiple times. So as long as the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">info</code> block is filled in before the very last callback is made, things would seem to work fine and there's no problem using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">u</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The fix is as simple as delaying the delete call to just before the function return once you're past the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">if (eol)</code> block. Since there are multiple return points I opted to make a very simple scope guard class that should do the right thing without needing multiple levels of indirection. But it wouldn't be hard just to manually delete in the right spots either and remove the existing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete u</code>.</p></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;">I don't actually have PulseAudio so to be honest I'm not even sure if this compiles...</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>phonon/pulsesupport.cpp <span style="color: grey">(6594c61)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127840/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>