[tcobrowser-cvs] SF.net SVN: tcobrowser:[1504] trunk/bibdesk/BDSKTask.m

[<ama...@us...>]

Revision: 1504
          http://tcobrowser.svn.sourceforge.net/tcobrowser/?rev=1504&view=rev
Author:   amaxwell
Date:     2009-10-01 22:23:13 +0000 (Thu, 01 Oct 2009)

Log Message:
-----------
fix potential race between dealloc and kevent

Modified Paths:
--------------
    trunk/bibdesk/BDSKTask.m

Modified: trunk/bibdesk/BDSKTask.m
===================================================================
--- trunk/bibdesk/BDSKTask.m	2009-09-25 18:52:11 UTC (rev 1503)
+++ trunk/bibdesk/BDSKTask.m	2009-10-01 22:23:13 UTC (rev 1504)
@@ -58,6 +58,7 @@
     int32_t            _terminationStatus;
     int32_t            _running;
     int32_t            _launched;
+    int32_t            _canNotify;
     struct kevent      _event;
     CFRunLoopRef       _rl;
     CFRunLoopSourceRef _rlsource;
@@ -76,6 +77,15 @@
     [NSThread detachNewThreadSelector:@selector(_watchQueue) toTarget:self withObject:nil];
 }
 
++ (BDSKTask *)launchedTaskWithLaunchPath:(NSString *)path arguments:(NSArray *)arguments;
+{
+    BDSKTask *task = [[self new] autorelease];
+    [task setLaunchPath:path];
+    [task setArguments:arguments];
+    [task launch];
+    return task;
+}
+
 #define ASSERT_LAUNCH do { if (!_internal->_launched) { [NSException raise:@"BDSKTaskException" format:@"Task has not been launched"]; } } while (0)
 #define ASSERT_NOTLAUNCHED do { if (_internal->_launched) { [NSException raise:@"BDSKTaskException" format:@"Task has already been launched"]; } } while (0)
 
@@ -86,13 +96,23 @@
         _internal = NSZoneCalloc([self zone], 1, sizeof(struct BDSKTaskInternal));
         memset(&_internal->_event, 0, sizeof(struct kevent));
         pthread_mutex_init(&_internal->_lock, NULL);
+        _internal->_canNotify = 1;
     }
     return self;
 }
 
 - (void)dealloc
 {
+    /*
+     Set _canNotify in case kevent unblocks before we can remove it from the queue,
+     since the event's task pointer is about to become invalid (and it mustn't access
+     the lock after this flag is set).  Lock before entering _disableNotification, so 
+     we can shrink our race window even smaller.
+     */
+    OSAtomicCompareAndSwap32Barrier(1, 0, &_internal->_canNotify);
+    pthread_mutex_lock(&_internal->_lock);
     [self _disableNotification];
+    pthread_mutex_unlock(&_internal->_lock);
     pthread_mutex_destroy(&_internal->_lock);
     [_launchPath release];
     [_arguments release];
@@ -197,10 +217,10 @@
 {
     ASSERT_NOTLAUNCHED;
     
-    int argCount = [_arguments count];
+    NSUInteger argCount = [_arguments count];
     const char *workingDir = [_currentDirectoryPath fileSystemRepresentation];
     char **args = NSZoneCalloc([self zone], (argCount + 2), sizeof(char *));
-    int i, iMax = argCount;
+    NSUInteger i, iMax = argCount;
     args[0] = (char *)[_launchPath fileSystemRepresentation];
     for (i = 0; i < iMax; i++) {
         args[i + 1] = (char *)[[_arguments objectAtIndex:i] fileSystemRepresentation];
@@ -214,10 +234,9 @@
     if (environment) {
         // fill with pointers to autoreleased C strings
         env = NSZoneCalloc([self zone], [environment count] + 1, sizeof(char *));
-        NSEnumerator *keyEnum = [environment keyEnumerator];
         NSString *key;
-        unsigned envIndex = 0;
-        while (key = [keyEnum nextObject]) {
+        NSUInteger envIndex = 0;
+        for (key in environment) {
             env[envIndex++] = (char *)[[NSString stringWithFormat:@"%@=%@", key, [environment objectForKey:key]] UTF8String];        
         }
         env[envIndex] = NULL;
@@ -301,7 +320,7 @@
          be protected by that lock.  Closing all remaining file descriptors doesn't break any documented 
          behavior of NSTask, and it should take care of that problem.  It's not a great solution, since 
          inheriting other descriptors could possibly be useful, but I don't need to share arbitrary file 
-         descriptors, wherease I do need subclassing and threads to work properly.
+         descriptors, whereas I do need subclassing and threads to work properly.
          */
         rlim_t j;
         for (j = (STDERR_FILENO + 1); j < maxOpenFiles; j++) {
@@ -405,6 +424,7 @@
 
 - (void)waitUntilExit;
 {
+    ASSERT_LAUNCH;
     while ([self isRunning]) {
         NSDate *next = [[NSDate allocWithZone:[self zone]] initWithTimeIntervalSinceNow:0.1];
         [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:next];
@@ -426,9 +446,10 @@
         if (kevent(_kqueue, NULL, 0, &evt, 1, NULL)) {
             
             BDSKTask *task = evt.udata;
+            OSMemoryBarrier();
             
             // can only fail if _disableNotification is called immediately after kevent unblocks
-            if (pthread_mutex_trylock(&task->_internal->_lock) == 0) {
+            if (task->_internal->_canNotify && pthread_mutex_trylock(&task->_internal->_lock) == 0) {
                 
                 /* 
                  Retain to make sure we hold a reference to the task long enough to handle these calls,
@@ -469,10 +490,8 @@
  thread won't do callout during/after dealloc.
  */
 - (void)_disableNotification
-{
-    pthread_mutex_lock(&_internal->_lock);
-    
-    // called unconditionally from -dealloc, so we may have already notified
+{        
+    // called unconditionally from -dealloc, so we may have already notified and freed this source
     if (_internal->_rlsource) {
         
         // after this point, _taskExited and __BDSKTaskNotify will never be called, so account for their teardown
@@ -489,11 +508,9 @@
             _internal->_rl = NULL;
         } 
     }
-    
-    pthread_mutex_unlock(&_internal->_lock);
 }
 
-// kevent thread has a retain, so no contention with _disableNotification
+// kevent thread has a retain, so no contention with _disableNotification since we can't dealloc
 - (void)_taskExited
 {
     NSParameterAssert(_internal->_launched);


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.