[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Race condition in KUniqueApplication
From:       Krzysztof Lichota <krzysiek () lichota ! net>
Date:       2007-03-27 8:24:30
Message-ID: 4608D4BE.6050601 () lichota ! net
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


While investigating Ark bug I have come across possible race condition
in KUniqueApplication.

It does the following (if --nofork is not set):
- fork a child
- child checks if application is running, if not spawns it
- child reports to parent using pipe if application is running
- parent sends to application DCOP call to create new instance
- if DCOP call fails, it just aborts

If application is during shutdown (which happens often with Ark when it
is unpacking files), the check for application running in child is
successful, while the DCOP call in parent fails and starting app fails.

IMO the right thing to do would be to make DCOP call in child and repeat
call (and spawning the process) if it fails. This involves many changes
in the code, so I tried the other approach: repeat everything in parent
process. This works much better (although does not solve Ark bug
completely, so I have to investigate it further).
Anyway, the patch (preliminary, it is not intended for applying) is
attached, what do you think of this whole problem? Have I missed
something? Should I try to move the DCOP call and loop in child?

BTW. I wonder who got the idea to remove arguments from command line in
KCmdLineArgs::saveAppArgs() - I would expect functions with "save" in
name to not modify things they save... The patch contains fix for this,
but I am not sure it should be applied, this might have some side
effects. Looping and making the DCOP call in child would solve race
condition without changing KCmdLineArgs::saveAppArgs().

	Krzysztof Lichota

PS. The for(;;) loop in patch should be replaced with some limited
number of loops to avoid possible fork bomb.


["kuniqueapplication-race-condition.diff" (text/x-patch)]

Index: kdecore/kcmdlineargs.cpp
===================================================================
--- kdecore/kcmdlineargs.cpp	(wersja 646305)
+++ kdecore/kcmdlineargs.cpp	(kopia robocza)
@@ -241,22 +241,33 @@
       parseAllArgs();
 
    // Remove Qt and KDE options.
-   removeArgs("qt");
-   removeArgs("kde");
+   KCmdLineArgs *args;
+   uint matchingArgs = 0;
+   for(args = argsList->first(); args; args = argsList->next())
+   {
+      if (!args->id  || !(::qstrcmp(args->id, "qt") == 0 || ::qstrcmp(args->id, \
"kde") == 0) ) +      {
+        matchingArgs++;
+      }
+   }
+//    removeArgs("qt");
+//    removeArgs("kde");
 
    QCString qCwd = mCwd;
    ds << qCwd;
 
-   uint count = argsList ? argsList->count() : 0;
+   uint count = matchingArgs;
    ds << count;
 
    if (!count) return;
 
-   KCmdLineArgs *args;
    for(args = argsList->first(); args; args = argsList->next())
    {
-      ds << QCString(args->id);
-      args->save(ds);
+      if (!args->id  || ! (::qstrcmp(args->id, "qt") == 0 || ::qstrcmp(args->id, \
"kde") == 0) ) +      {
+        ds << QCString(args->id);
+        args->save(ds);
+      }
    }
 }
 
Index: kdecore/kuniqueapplication.cpp
===================================================================
--- kdecore/kuniqueapplication.cpp	(wersja 646305)
+++ kdecore/kuniqueapplication.cpp	(kopia robocza)
@@ -131,6 +131,10 @@
      // We'll call newInstance in the constructor. Do nothing here.
      return true;
   }
+  
+  for (;;)
+  {
+    //repeat until we succeed in making DCOP call to create new instance or spawning \
new process  DCOPClient *dc;
   int fd[2];
   signed char result;
@@ -283,9 +287,11 @@
 #endif
      
      QByteArray data, reply;
+     
      QDataStream ds(data, IO_WriteOnly);
 
-     KCmdLineArgs::saveAppArgs(ds);
+    KCmdLineArgs::saveAppArgs(ds);
+    
      ds << new_asn_id;
 
      dc->setPriorityCall(true);
@@ -294,7 +300,7 @@
      {
         kdError() << "Communication problem with " << KCmdLineArgs::about->appName() \
<< ", it probably crashed." << endl;  delete dc;	// Clean up DCOP commmunication
-        ::exit(255);
+        continue;
      }
      dc->setPriorityCall(false);
      if (replyType != "int")
@@ -310,6 +316,8 @@
      ::exit(exitCode);
      break;
   }
+  break; //break out of for (;;) loop
+  } //for (;;)
   return false; // make insure++ happy
 }
 


["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]