Review Request 126507: Fix leaked file description and potential use-after-free in kdelibs4support

[David Faure]

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



src/kdeui/k4style.cpp (line 1176)
<https://git.reviewboard.kde.org/r/126507/#comment61851>

    The double-lookup looks slow and unnecessary.
    contains + *object doesn't give you anything that object (followed by a dereference if not null) gives you.
    
    If the fear is concurrent access, then contains + *object can still dereference a null pointer anyway.
    
    But K4Style runs in the GUI thread always anyway. So I would call object(), put that into a pointer, and then if not null, make a copy into a value. This avoids the double lookup.


- David Faure


On Jan. 2, 2016, 1:55 a.m., Michael Pyne wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126507/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2016, 1:55 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> -------
> 
> Fix a couple of Coverity issues:
> 
> 1. CID 1175508; file descriptors used in KLockFile could be leaked in
> error conditions. Even when KLockFile sets mustCloseFd, the dtor's impl
> also checks that the lock has been taken, which is only considered true
> if LockOK had been returned in our lock function. Instead close() the fd
> ourselves unless we make it to LockOK.
> 
> 2. CID 1175555; The standard mis-use of QCache. QCache::insert can, in
> theory, delete our object as soon as we insert it into cache, so we have
> to check for that. Even ::contains() and ::object() can be risky (the
> pointers returned by object() have no lifetime guarantee), but since
> this is GUI code I assume it's only used single-threaded and not
> re-entrant. Otherwise we'd need even more paranoia...
> 
> 
> Diffs
> -----
> 
>   src/kdecore/klockfile_unix.cpp 67283a5 
>   src/kdeui/k4style.cpp a1a2ab1 
> 
> Diff: https://git.reviewboard.kde.org/r/126507/diff/
> 
> 
> Testing
> -------
> 
> Everything builds and appears to still work, though it's hard to test K4Style as I'm not sure what uses it right at this point.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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