Expand a lock in the connection keepalive code, because it still allowed a zombie call

1 files changed, 19 insertions, 2 deletions

diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m
index d46b5552..8728bf3f 100644
--- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m
+++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m
@@ -85,9 +85,9 @@
 		if(keepAliveThread) {
 			NSLog(@"warning: overwriting existing keepAliveThread: %@, results may be unpredictable!",keepAliveThread);
 		}
+		keepAliveThread = [NSThread currentThread];
 	}
 	
-	keepAliveThread = [NSThread currentThread];
 	[keepAliveThread setName:[NSString stringWithFormat:@"SPMySQL connection keepalive monitor thread (id=%p)", self]];
 
 	// If the maximum number of ping failures has been reached, determine whether to reconnect.
@@ -282,7 +282,24 @@ void _pingThreadCleanup(void *pingDetails)
 	if (keepAliveThread) {
 
 		// Mark the thread as cancelled
-		[keepAliveThread cancel];
+		@synchronized(self) {
+			// the synchronized is neccesary here, because we don't retain keepAliveThread.
+			// If it were ommitted, for example this could happen:
+			//
+			//   this thread                                 keepalive thread
+			//   --------------                              -----------------
+			// 1 fetch value of keepAliveThread to register
+			// 2                                             keepAliveThread = nil
+			// 3                                             [[NSThread currentThread] release]
+			// 4 objc_msgSend() <-- invalid memory accessed
+			//
+			// With synchronized we are guaranteed to either message nil or block the keepAliveThread from exiting
+			// (and thus releasing the NSThread object) until this call finishes.
+			//
+			// We can omit it in the other 2 cases, since keepAliveThread is already volatile and we are only
+			// checking for NULL, not dereferencing it.
+			[keepAliveThread cancel];
+		}
 
 		// Wait inside a time limit of ten seconds for it to exit
 		uint64_t threadCancelStartTime_t = mach_absolute_time();