More crash-proof KCrash (was Re: Konq crash backtraces)

[Oswald Buddenhagen]

On Tue, Jan 17, 2006 at 01:36:30PM +0100, Lubos Lunak wrote:
> > It's called to free up the dcop name while the crash dialog is
> > showing. It also allows kicker to restart from the crash handler.
> 
> I guess it pre-dates the closing of all open filedescriptors done in
> the crashhandler. Kicker restarts here just fine and the dcop
> connection is broken by the close() too.
> 
fwiw, i have the attached patch on my disk for something like 1 - 1½
years. it's particularly remarkable that it closes only the dcop and x
connection in the parent, then forks off the process for drkonqi and
closes the file descriptors only in the child. the idea is to keep the
handles open, so a parent process that determines the child's state
based on some pipes would not bury the process until drkonqi diagnosed
death - in kdm's case that means resetting the x-server, so drkonqi
wouldn't have a particularly long appearance otherwise.

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
--
Chaos, panic, and disorder - my work here is done.
-------------- next part --------------
--- kcrash.cpp.orig	2003-11-22 16:17:23.000000000 +0100
+++ kcrash.cpp	2003-11-22 15:55:48.000000000 +0100
@@ -125,13 +125,11 @@
     crashRecursionCounter++; //
   }
 
-        // Close dcop connections
+  // Close dcop connections
   DCOPClient::emergencyClose();
-  // Close all remaining file descriptors
-  struct rlimit rlp;
-  getrlimit(RLIMIT_NOFILE, &rlp);
-  for (int i = 0; i < (int)rlp.rlim_cur; i++)
-    close(i);
+  // Close X connection
+  if ( qt_xdisplay() )
+    close(ConnectionNumber(qt_xdisplay()));
 
   bool shuttingDown = false;
 
@@ -152,16 +150,15 @@
         pid_t pid = fork();
 
         if (pid <= 0) {
-          // this code is leaking, but this should not hurt cause we will do a
-          // exec() afterwards. exec() is supposed to clean up.
-          char * argv[24]; // don't forget to update this
+          QCString sigstr, pidstr, verstr, namestr, addrstr;
+          const char * argv[24]; // don't forget to update this
           int i = 0;
 
           // argument 0 has to be drkonqi
-          argv[i++] = qstrdup("drkonqi");
+          argv[i++] = "drkonqi";
 
           // start up on the correct display
-          argv[i++] = qstrdup("-display");
+          argv[i++] = "-display";
 #if defined Q_WS_X11 && ! defined K_WS_QTONLY
           if ( qt_xdisplay() )
             argv[i++] = XDisplayString(qt_xdisplay());
@@ -172,70 +169,73 @@
 #endif
 
           // we have already tested this
-          argv[i++] = qstrdup("--appname");
-          argv[i++] = qstrdup(appName);
+          argv[i++] = "--appname";
+          argv[i++] = appName;
           if (KApplication::loadedByKdeinit)
-            argv[i++] = qstrdup("--kdeinit");
+            argv[i++] = "--kdeinit";
 
           // only add apppath if it's not NULL
           if (appPath) {
-            argv[i++] = qstrdup("--apppath");
-            argv[i++] = qstrdup(appPath);
+            argv[i++] = "--apppath";
+            argv[i++] = appPath;
           }
 
           // signal number -- will never be NULL
-          QCString tmp;
-          tmp.setNum(sig);
-          argv[i++] = qstrdup("--signal");
-          argv[i++] = qstrdup(tmp.data());
+          sigstr.setNum(sig);
+          argv[i++] = "--signal";
+          argv[i++] = sigstr.data();
 
           // pid number -- only include if this is the child
-          // the debug stuff will be disabled if we was not able to fork
+          // the debug stuff will be disabled if we were not able to fork
           if (pid == 0) {
-            tmp.setNum(getppid());
-            argv[i++] = qstrdup("--pid");
-            argv[i++] = qstrdup(tmp.data());
+            pidstr.setNum(getppid());
+            argv[i++] = "--pid";
+            argv[i++] = pidstr.data();
           }
 
           const KInstance *instance = KGlobal::_instance;
           const KAboutData *about = instance ? instance->aboutData() : 0;
           if (about) {
             if (!about->version().isNull()) {
-              argv[i++] = qstrdup("--appversion");
-              argv[i++] = qstrdup(about->version().utf8());
+              verstr = about->version().utf8();
+              argv[i++] = "--appversion";
+              argv[i++] = verstr.data();
             }
 
             if (!about->programName().isNull()) {
-              argv[i++] = qstrdup("--programname");
-              argv[i++] = qstrdup(about->programName().utf8());
+              namestr = about->programName().utf8();
+              argv[i++] = "--programname";
+              argv[i++] = namestr.data();
             }
 
             if (!about->bugAddress().isNull()) {
-              argv[i++] = qstrdup("--bugaddress");
-              argv[i++] = qstrdup(about->bugAddress().utf8());
+              addrstr = about->bugAddress().utf8();
+              argv[i++] = "--bugaddress";
+              argv[i++] = addrstr.data();
             }
           }
 
           if ( kapp && !kapp->startupId().isNull()) {
-            argv[i++] = qstrdup("--startupid");
-            argv[i++] = qstrdup(kapp->startupId());
+            argv[i++] = "--startupid";
+            argv[i++] = kapp->startupId().data();
           }
 
-          if ( safer )
-            argv[i++] = qstrdup("--safer");
+          if ( safer || geteuid() != getuid() || getegid() != getgid() )
+            argv[i++] = "--safer";
 
           // NULL terminated list
           argv[i++] = NULL;
 
-          setgid(getgid());
-          setuid(getuid());
+          // Close all remaining file descriptors
+          if (pid == 0) {
+            struct rlimit rlp;
+            getrlimit(RLIMIT_NOFILE, &rlp);
+            for (int i = 0; i < (int)rlp.rlim_cur; i++)
+              close(i);
+          }
 
-          execvp("drkonqi", argv);
+          execvp("drkonqi", (char**)argv);
 
-          // we could clean up here
-          // i = 0;
-          // while (argv[i])
-          //   free(argv[i++]);
         }
         else
         {