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

2016-01-02 Thread David Faure 

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

Ship it!


Ship It!

- David Faure


On Jan. 2, 2016, 4:02 p.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, 4:02 p.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 117; 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

2016-01-02 Thread Michael Pyne 

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

(Updated Jan. 2, 2016, 4:02 p.m.)


Review request for KDE Frameworks.


Changes
---

Remove the double-lookup.

Agreed on the relative safety of the GUI thread. The big issue I had when first 
prepping this patch was reconciling object lifetime of `*tiles` when tiles is 
return value of `QCache::object()` vs. when use `QCache::insert()`. If we make 
local copy of `*tiles` then we must remember to free it. If we use return value 
of `::object()` we must *not* free it.

I also had a version of this patch that just uses a bool flag `shouldDelete` 
and just uses the existing `*tiles` throughout, which might be simpler. I'll 
leave it up to you whether you think that would be a better fit here.


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 117; 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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

2016-01-02 Thread Michael Pyne 

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

(Updated Jan. 2, 2016, 11:29 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit f4e9dbf2b4ee2770e554c735b7604637e7b5ec54 by Michael Pyne 
to branch master.


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 117; 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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

2016-01-01 Thread Michael Pyne 

---
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.


Changes
---

Revise the patch to match the original code's logic as noted in David's review.


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 117; 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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

2016-01-01 Thread David Faure 

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



src/kdeui/k4style.cpp (line 1213)


this condition used to be evaluated when tiles was non-null (i.e. key in 
cache). In your patch, key-in-cache goes to the very first branch and not here 
anymore.


- David Faure


On Dec. 25, 2015, 12:16 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126507/
> ---
> 
> (Updated Dec. 25, 2015, 12:16 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 117; 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

2015-12-24 Thread Michael Pyne 

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

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 117; 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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel