Review Request 126409: Fix use-after-free in .desktop file parser

[Michael Pyne]

> On Dec. 18, 2015, 3:17 a.m., Alex Richardson wrote:
> > src/lib/plugin/desktopfileparser.cpp, line 346
> > <https://git.reviewboard.kde.org/r/126409/diff/1/?file=424369#file424369line346>
> >
> >     `defs << *def // This must...` instead? And then the `defs << *def` in the else block? Would remove the need for the temporary variable declaration at the top.
> >     
> >     Switching the if/else would also make this easier to understand.
> 
> Michael Pyne wrote:
>     I prefer to factor loop invariants (such as updated defs here) out of if/else blocks, which is why I used temporary, but whichever fix is used will be pretty obvious here so I've submitted a revision.

Any objections to the revised patch?


- Michael


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


On Dec. 18, 2015, 3:44 a.m., Michael Pyne wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126409/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2015, 3:44 a.m.)
> 
> 
> Review request for KDE Frameworks and Alex Richardson.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> Patch to fix a use-after-free issue in kcoreaddons/src/lib/plugin/desktopfileparser.cpp, noted by Coverity (CID 1336157)
> 
> The issue is that if `defs << *def` happens after the call to s_serviceTypes->insert(), the `*def` might already have been deleted by QCache. Attached patch fixes by making the copy before the call to insert().
> 
> Really, it's probably better to not use QCache here if we can avoid it, but I'm not sure enough of the code to go that route. But if this is something that's only run once or twice anyways then there's no real need to use QCache instead of QMap, I would think.
> 
> 
> Diffs
> -----
> 
>   src/lib/plugin/desktopfileparser.cpp 06a4a1d 
> 
> Diff: https://git.reviewboard.kde.org/r/126409/diff/
> 
> 
> Testing
> -------
> 
> desktoptojsontest still passes, code compiles.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151220/a5cf4ccf/attachment.html>