Review Request 119497: Report crashes of KDE apps in Apple OS X (1) (fix kcrash, kinit)

Mon Sep 15 02:39:53 BST 2014

> On July 27, 2014, 11:32 a.m., Thomas Lübking wrote:
> > kdeui/util/kcrash.cpp, line 316
> > <https://git.reviewboard.kde.org/r/119497/diff/1/?file=293441#file293441line316>
> >
> >     is libdispatch OSX only or is it also used on FreeBSD?
> >     
> >     The question (more to Michael ;-) is whether unconditionally closing all FDs is acceptable in general (there's hell lot of Q_OS_* items, incl. Q_OS_HURD ;-) or the check should be inverted to
> >     
> >     #ifdef Q_OS_LINUX
> 
> Michael Pyne wrote:
>     Unfortunately closing all FDs is probably not acceptable *in general*, but in the context of a launching a crash handler in a crashed KDE GUI application, I don't see any reason why it would hurt, it's not as if the application itself is going to need the FD anymore as it's about to crash and has already run its emergency save function (and if the app does need it, they can either use their own crash function or use the documented KeepFD flag). Closing all FDs is a security and rlimit precaution in case Dr. Konqi gets started directly from the crashing proc instead of via kdeinit.
>     
>     The check should probably remain on any POSIX-alike, in my opinion, as we wouldn't want Dr. Konqi on Mac OS X accidentally having access to a crashing application FDs if launched directly either.
> 
> Ian Wadham wrote:
>     On Apple OS X it is Apple's COCOA library internals that have the problem FDs, not the app, and COCOA crashes internally if you close its FDs in the crash handler. We do not know what numbers those FDs have in any arbitrary app or run of an app, and I think there is intrinsically no way to know. We also do not know what FDs may have been created by internals of the KDE library, but they do not seem to mind being closed peremptorily.
>     
>     I could put a setFlags call (conditional on Q_OS_MAC) a few lines earlier in the default crash-handler, but there does not seem to be much point in that.
>     
>     It is quite safe to leave FDs open, because KCrash closes them all unconditionally on line 623 of kcrash.cpp (if it starts Dr K by forking), with the comment "We are in the child now. Close FDs unconditionally." Presumably, by that time, the COCOA library has had a chance to terminate its internals gracefully, because this time there is no secondary crash. Presumably also, if Dr K is started via kinit and klauncher, its FDs are all its own, but kinit is a very grey area for me.
> 
> Michael Pyne wrote:
>     Yes, you're right. It seems in the only problematic case (a direct fork/exec to start drkonqi) that we already close FDs unconditionally. In that case I think don't closing the FDs from the crash handler adds any value (and I suppose, might even interfere in debugging once drkonqi can get a debugger attached), so it might be best to remove the feature from the default crash handler entirely. David, thoughts?

NOT closing FDs at this point has been made conditional on Q_OS_WIN or Q_OS_MAC. Others may make it unconditional on all platforms. It might be a safe thing to do...

> On July 27, 2014, 11:32 a.m., Thomas Lübking wrote:
> > kinit/kinit.cpp, line 1478
> > <https://git.reviewboard.kde.org/r/119497/diff/1/?file=293442#file293442line1478>
> >
> >     this and line 1504 look like debug leftovers? or are they needed for extra logging?
> 
> Ian Wadham wrote:
>     I was using them for diagnosing this portability bug in kinit, but I thought it would be best to leave them in as a safety measure, so that someone can see what happened in the event that my issue/question re line 119 (what to do about the usage of "DISPLAY" in Apple OS X) causes premature termination of kdeinit4 in the future in an Apple OS X environment.

This part of the patch has been discontinued.

- Ian

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

On Sept. 15, 2014, 1:39 a.m., Ian Wadham wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119497/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2014, 1:39 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and Michael Pyne.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> When a KDE app crashes in Apple OS X, it just disappears from the screen. At the most, the user is invited to report the crash to Apple. AFAIK this has been a problem in KDE on Apple OS X for years, leading to frustration with KDE among Apple users and MacPorts developers and an attitude among KDE developers of "Why does nobody report the problem(s) on bugs.kde.org?"
> 
> It is my strong belief that the failure to report crashes of KDE apps in Apple OS X also exists in Frameworks.
> 
> So far I have identified a number of portability bugs in KDE on Apple OS X: 1 in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Patches for the first two are submitted in this review. Patches for three more are submitted in part 2 of this review, against kde-runtime. I am still investigating the other two problems in Dr Konqi - and there could be more than two...
> 
> In this review we have two portability problems:
> 
> 1. KCrash itself crashes (SIGILL) when it tries to close all file descriptors and so Dr Konqi is not started. Some of the FDs belong to the Apple OS X library (COCOA) and it crashes if they are closed prematurely.
> 
> 2. The preferred way to start Dr K is via a socket message to kdeinit4, but that fails in Apple OS X because kdeinit4 is listening with the wrong socket name. The DISPLAY ID is missing from the end of the name. The fallback is for KCrash to use fork() and exec(), which works, but could cause Dr K to be polluted, depending on the nature of the crash.
> 
> This "deafness" of kdeinit4 (and klauncher) could be causing other failures in KDE software in the Apple OS X environment.
> 
> 
> Diffs
> -----
> 
>   kdeui/util/kcrash.cpp 45eb46b 
> 
> Diff: https://git.reviewboard.kde.org/r/119497/diff/
> 
> 
> Testing
> -------
> 
> Using Apple OS X 10.7.5 (Lion) on a MacBook Pro, I have installed KDE libs via MacPorts (at version 4.12.5) and I have adapted kdesrc-build to run in an Apple OS X environment and used it to test against the KDE 4.13 branch.  I have been testing with a KDE app that I can crash at will and using stderr and Apple OS X Console log output to determine the outcome.
> 
> Please note that I am the -only- KDE developer who has this kind of setup, but I am NOT a KDE core developer. My experience before now has been in KDE Games. However I used to be a UNIX and database guru before I retired in 1998.
> 
> I NEED HELP from KDE -core- developers to proceed further. These problems also exist in FRAMEWORKS, but I am as yet unable to build or test Frameworks on Apple OS X. And I am sure there are many more Apple OS X portability problems in KDE software.
> 
> Without my patches, Dr Konqi is not started and, if it does get past its own crash, KCrash reports:
> KCrash: Attempting to start /kdedev/kde4.13/kde4/lib/kde4/libexec/drkonqi.app/Contents/MacOS/drkonqi from kdeinit
> sock_file=/kdedev/kde4.13/home/.kde4.13/socket-Tara.local/kdeinit4__tmp_launch-svPd0L_org.x_0
> Warning: connect() failed: : No such file or directory
> 
> With my patches, Dr Konqi can always be started directly (using fork()) and it -will- start via kdeinit4 and klauncher but it immediately runs into a privilege problem with Apple OS X (a problem which I am still investigating).
> 
> I would not have got this far without help from Michael Pyne, Thomas Lübking and several of the MacPorts developers, as well as the unfailing enthusiasm and encouragement of my friend Marko Käning.
> 
> 
> Thanks,
> 
> Ian Wadham
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140915/54b6505f/attachment.htm>