Bug 337154 - preventing a crash in the KWindowInfo::Private destructor
Summary: preventing a crash in the KWindowInfo::Private destructor
Status: RESOLVED FIXED
Alias: None
Product: kdelibs
Classification: Frameworks and Libraries
Component: kdeui (show other bugs)
Version: 4.12.5
Platform: MacPorts macOS
: NOR crash
Target Milestone: ---
Assignee: kdelibs bugs
URL: https://git.reviewboard.kde.org/r/119...
Keywords:
: 334827 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-07-06 22:40 UTC by RJVB
Modified: 2014-07-15 23:02 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
patch for kwindowinfo_mac.cpp (461 bytes, application/octet-stream)
2014-07-06 22:40 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description RJVB 2014-07-06 22:40:06 UTC
Created attachment 87602 [details]
patch for kwindowinfo_mac.cpp

After building and installing rekonq under OS X/MacPorts, I quickly ran into a systematic crash each time I accepted the settings dialog after making a change. 
A little debugging showed that the snippet

    Q_FOREACH(const QWeakPointer<RekonqWindow> &pointer, wList)
    {
        if (KWindowInfo(pointer.data()->effectiveWinId(), NET::WMDesktop, 0).isOnCurrentDesktop())
            return pointer.data();
    }

will lead to calling CFRelease with a NULL argument in
KWindowInfo::Private::~Private(). Cocoa allows sending a message (like
[obj release]) to a nil object, but the CoreFoundation version CFRelease will
crash when passed a NULL pointer. This is confirmed by the API
documentation.

Attached is a very simple patch that adds the required check before
calling CFRelease - as is done all other times that function is called.

The patch was made with KDE 4.12.5 but should apply to the current and development versions too given that kwindowinfo_mac.cpp hasn't been touched in over a year (https://projects.kde.org/projects/kde/kdelibs/repository/revisions/master/changes/kdeui/windowmanagement/kwindowinfo_mac.cpp).
Comment 1 Marko Käning 2014-07-11 06:11:47 UTC
See also issue 334827!
Comment 2 Marko Käning 2014-07-11 06:14:50 UTC
*** Bug 334827 has been marked as a duplicate of this bug. ***
Comment 3 Thomas Braxton 2014-07-11 10:05:41 UTC
A review request would probably make more sense.
Comment 4 RJVB 2014-07-11 10:29:03 UTC
(In reply to comment #3)
> A review request would probably make more sense.

For a very evident bug and equally straightforward patch??

Online CFRelease documentation: https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFTypeRef/Reference/reference.html#//apple_ref/c/func/CFRelease
"Special Considerations
If cf is NULL, this will cause a runtime error and your application will crash.
"
Comment 5 Martin Flöser 2014-07-11 10:48:43 UTC
yes a review request is needed for the very obvious reason that it will be lost on bugzilla. You see I did not open the bug report because I saw that it is about MacOS and cannot do anything there so wouldn't spot that there is a patch attached.
Comment 6 RJVB 2014-07-11 10:58:13 UTC
Frankly, then what's the sense about having a bug tracker?? And why would  someone who "cannot do anything" on OS X get involved with a patch that's only relevant to that OS?

Anyway, I just tried to submit a review request for patches against 4.12.5 that improve OS integration, and found they were refused because not git diff patches. Tell me how to submit the patches as is and I'll submit the request.
Comment 7 Martin Flöser 2014-07-11 11:05:48 UTC
(In reply to comment #6)
> Frankly, then what's the sense about having a bug tracker??

the bug tracker is for bugs, not for patches - at least that's the way we work here in KDE.

> And why would 
> someone who "cannot do anything" on OS X get involved with a patch that's
> only relevant to that OS?

because I'm the maintainer of the framework and are supposed to ACK the change.

> 
> Anyway, I just tried to submit a review request for patches against 4.12.5
> that improve OS integration, and found they were refused because not git
> diff patches. Tell me how to submit the patches as is and I'll submit the
> request.

Unfortunately it needs to be git formatted. That's also needed for pushing - after all we want to keep the author information.
Comment 8 RJVB 2014-07-11 11:17:11 UTC
Well, this *is* a bug report. You're free to disregard the patch and simply correct the bug going by the documents and other invocations of CFRelease. Maybe that'll be quicker than me finding time and motivation to figure out how to share a patch that already took me enough time (and for which I don't care the least to be the recognised author) :)
Comment 9 Thomas Lübking 2014-07-11 12:09:08 UTC
because reviewboard is better suited to announce (and review) patches.

problem on the "straightforward" aspect is that most devs here (inc. me) do not operate on OSX - iff there is any strategy on dealing with CF, only one or two ppl. might know and they'll more likely look at a patch presented for review than at some random, pot. module related, bug.

Situation is similar w/ eg convert_CFString_to_QString(CFStringRef str) which segfaults if you dump in  a nullptr "str" what at least klocale (mac) seems prone to because the apparently CFStringRef( CFLocaleGetValue( m_macLocale, key ) ); can return one.
See http://lists.kde.org/?t=139514981600001&r=1&w=2


-> If OSX is a target system for KDE, we severely lack developers, so if you're interested in KDE on OSX and apparently have coding skills, it would be *really* great if you would seek for commit rights and get in touch with other OSX using developers.
There is certainly ambition, but also a severe lack of manpower.
Comment 10 RJVB 2014-07-11 14:41:36 UTC
> -> If OSX is a target system for KDE, we severely lack developers, so if you're interested in KDE
> on OSX and apparently have coding skills, it would be *really* great if you would seek for
> commit rights and get in touch with other OSX using developers.

Not that I am particularly fluent in C++ or familiar with Qt/KDE frameworks, but I certainly wouldn't mind to obtain commit rights and contribute at my own pace (i.e. not necessarily spearheading development of the latest and greatest with all the requirements that come with it).
Comment 11 Ian Wadham 2014-07-12 07:13:50 UTC
René (RJVB) is already part of our group at KDE Software on Mac OS X <kde-mac@kde.org> and a frequent contributor on the MacPorts lists.  Please accept his patch.

IMHO, a patch is a patch, regardless of how it is submitted or from where - even if it is written on old newspaper "with a thumbnail dipped in tar" and it washes up on the seashore in a bottle... :-)  Our only concern should be if it is valid, properly written, low risk and works.  And if a user reports a bug and submits a good patch, that is treasure indeed - or it used to be in the old days of KDE.

This patch can only affect the Mac OS X version of KDE 4.  Martin, if you need any tests done on an Apple OS X platform, please let Marko (Comment 1) or me know.

Re git-format patches, not every programmer in the world has or understands git.  I usually receive a patch like the attached by cd'ing to the source code in my local repository, using the "patch" utility and then using "git diff" to get it in git format, or even "git checkout" to get the source files back as they were.  Is it that hard?
Comment 12 Marko Käning 2014-07-12 09:26:33 UTC
So, I filed the patch at RB.

https://git.reviewboard.kde.org/r/119240/
Comment 13 Marko Käning 2014-07-14 22:18:18 UTC
Git commit d32b52996ce48d484bbf54a62321872a482e8d74 by Marko Käning.
Committed on 14/07/2014 at 22:09.
Pushed by kaning into branch 'KDE/4.13'.

Preventing a crash in the KWindowInfo::Private destructor on OSX

René J.V. Bertin found that Apple's developer documentation for
CFRelease mentions that its argument may not be a NULL pointer.

M  +3    -1    kdeui/windowmanagement/kwindowinfo_mac.cpp

http://commits.kde.org/kdelibs/d32b52996ce48d484bbf54a62321872a482e8d74
Comment 14 Marko Käning 2014-07-15 23:02:01 UTC
Git commit 10398255165afda1a0ecddd1671bbb599846bb35 by Marko Käning.
Committed on 15/07/2014 at 22:51.
Pushed by kaning into branch 'master'.

Preventing a crash in the KWindowInfo::Private destructor on OSX

René J.V. Bertin found that Apple's developer documentation for
CFRelease mentions that its argument may not be a NULL pointer.
CCMAIL: kde-frameworks-devel@kde.org
REVIEW: 119240

M  +3    -1    src/kwindowinfo_mac.cpp

http://commits.kde.org/kwindowsystem/10398255165afda1a0ecddd1671bbb599846bb35