Review Request 127840: phonon: Avoid use-after-free after enumerating pulseaudio devices

[Harald Sitter]

> On May 9, 2016, 8:37 a.m., Harald Sitter wrote:
> > 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.
> > 
> > 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:
> > ```
> > <0 = failure during whatever should have produced data for the callback
> >  0 = not at end
> > >0 = at end, no more data
> > ```
> > 
> > So, our callback essentially goes 
> > 
> > ```
> > 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
> > ```
> > 
> > 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 *should* be the case but isn't a very smart way to handle this IMO.
> > 
> > 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.
> > 
> > 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.
> 
> Michael Pyne wrote:
>     Sounds fair to me. Did you want me to revise the patch or did you want to just make those changes? Since I don't have Pulse I'd need someone else to test for me anyways.

Ah well, I might as well do the change and test. Thanks for highlighting the issue :)


- Harald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127840/#review95294
-----------------------------------------------------------


On May 5, 2016, 2:07 a.m., Michael Pyne wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127840/
> -----------------------------------------------------------
> 
> (Updated May 5, 2016, 2:07 a.m.)
> 
> 
> Review request for Phonon.
> 
> 
> Repository: phonon
> 
> 
> Description
> -------
> 
> Coverity notes (CID 1336170) there's a potential use-after-free in the PulseAudio support code (pulsesupport.cpp:472 uses `u`, which may have been deleted at pulsesupport.cpp:408 if this was the last time the callback needed to be run).
> 
> 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.
> 
> The problem with fixing is that the lifetime of the `userdata` must be dynamic since (from what I can tell), the callback can be called multiple times. So as long as the `info` block is filled in before the very last callback is made, things would seem to work fine and there's no problem using `u`.
> 
> The fix is as simple as delaying the delete call to just before the function return once you're past the `if (eol)` 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 `delete u`.
> 
> 
> Diffs
> -----
> 
>   phonon/pulsesupport.cpp 6594c61 
> 
> Diff: https://git.reviewboard.kde.org/r/127840/diff/
> 
> 
> Testing
> -------
> 
> I don't actually have PulseAudio so to be honest I'm not even sure if this compiles...
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20160510/e5a2d2f8/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia