Another multithreading bug (I think)

[Wim Oudshoorn]

On a 64 bit multiprocessor machine, running Redhat,
our program sometimes crashes, but unfortunately not
easily reproducable.

I was lucky enough to capture one crash while 
running under valgrind and the valgrind output
is interesting:

==15122==
==15122== Invalid read of size 8
==15122==    at 0x5E512D2: objc_msg_lookup (sarray.h:230)
==15122==    by 0x57F054E: existingConnection (NSConnection.m:269)
==15122==    by 0x57F0AD9: _c_NSConnection__connectionWithReceivePort_sendPort_ 
(NSConnection.m:358)
==15122==    by 0x57F926F: _i_NSConnection_Private_handlePortMessage_ 
(NSConnection.m:2156)
==15122==    by 0x58C6B35: 
_i_GSMessageHandle__receivedEvent_type_extra_forMode_ (NSMessagePort.m:884)
==15122==    by 0x58C95C3: _i_NSMessagePort__receivedEvent_type_extra_forMode_ 
(NSMessagePort.m:1702)
==15122==    by 0x59027E7: _i_GSRunLoopCtxt__pollUntil_within_ 
(GSRunLoopCtxt.m:534)
==15122==    by 0x58789C1: _i_NSRunLoop__acceptInputForMode_beforeDate_ 
(NSRunLoop.m:1064)
==15122==    by 0x5878D19: _i_NSRunLoop__runMode_beforeDate_ (NSRunLoop.m:1136)
==15122==    by 0x59F1A9: _i_PhapApplication__startApplication (in 
/usr/lib/infor-ap-5.0.0.0.39/PhapServer)
==15122==    by 0x46423F: main (in /usr/lib/infor-ap-5.0.0.0.39/PhapServer)
==15122==  Address 0xC1C0A00 is 16 bytes inside a block of size 192 free'd
==15122==    at 0x49057C8: free (vg_replace_malloc.c:233)
==15122==    by 0x57F2090: _i_NSConnection__dealloc (NSConnection.m:706)
==15122==    by 0x57F4BC2: _i_NSConnection__release (NSConnection.m:1296)
==15122==    by 0x57968AC: _i_GSArray__dealloc (GSArray.m:143)
==15122==    by 0x5798685: _i_GSArrayEnumerator__dealloc (GSArray.m:865)
==15122==    by 0x57DD6A6: _i_NSAutoreleasePool__dealloc 
(NSAutoreleasePool.m:379)
==15122==    by 0x57DD9CC: _c_NSAutoreleasePool___endThread_ 
(NSAutoreleasePool.m:458)
==15122==    by 0x589EEA0: _i_NSThread__dealloc (NSThread.m:710)
==15122==    by 0x589EB6C: _c_NSThread__exit (NSThread.m:593)
==15122==    by 0x5E52A4A: __objc_thread_detach_function (thr.c:106)
==15122==    by 0x3D22F06109: start_thread (in /lib64/tls/libpthread-2.3.4.so)
==15122==    by 0x3D222C68C2: clone (in /lib64/tls/libc-2.3.4.so)
==15122==
==15122== Invalid read of size 8
==15122==    at 0x5E512D8: objc_msg_lookup (sarray.h:230)
==15122==    by 0x57F054E: existingConnection (NSConnection.m:269)
==15122==    by 0x57F0AD9: _c_NSConnection__connectionWithReceivePort_sendPort_ 
(NSConnection.m:358)
==15122==    by 0x57F926F: _i_NSConnection_Private_handlePortMessage_ 
(NSConnection.m:2156)
==15122==    by 0x58C6B35: 
_i_GSMessageHandle__receivedEvent_type_extra_forMode_ (NSMessagePort.m:884)
==15122==    by 0x58C95C3: _i_NSMessagePort__receivedEvent_type_extra_forMode_ 
(NSMessagePort.m:1702)
==15122==    by 0x59027E7: _i_GSRunLoopCtxt__pollUntil_within_ 
(GSRunLoopCtxt.m:534)
==15122==    by 0x58789C1: _i_NSRunLoop__acceptInputForMode_beforeDate_ 
(NSRunLoop.m:1064)
==15122==    by 0x5878D19: _i_NSRunLoop__runMode_beforeDate_ (NSRunLoop.m:1136)
==15122==    by 0x59F1A9: _i_PhapApplication__startApplication (in 
/usr/lib/infor-ap-5.0.0.0.39/PhapServer)
==15122==    by 0x46423F: main (in /usr/lib/infor-ap-5.0.0.0.39/PhapServer)
==15122==  Address 0xDEADFB0E is not stack'd, malloc'd or (recently) free'd
==15122==
==15122== Process terminating with default action of signal 11 (SIGSEGV)
==15122==  Access not within mapped region at address 0xDEADFB0E
==15122==    at 0x5E512D8: objc_msg_lookup (sarray.h:230)
==15122==    by 0x57F054E: existingConnection (NSConnection.m:269)
==15122==    by 0x57F0AD9: _c_NSConnection__connectionWithReceivePort_sendPort_ 
(NSConnection.m:358)
==15122==    by 0x57F926F: _i_NSConnection_Private_handlePortMessage_ 
(NSConnection.m:2156)
==15122==    by 0x58C6B35: 
_i_GSMessageHandle__receivedEvent_type_extra_forMode_ (NSMessagePort.m:884)
==15122==    by 0x58C95C3: _i_NSMessagePort__receivedEvent_type_extra_forMode_ 
(NSMessagePort.m:1702)
==15122==    by 0x59027E7: _i_GSRunLoopCtxt__pollUntil_within_ 
(GSRunLoopCtxt.m:534)
==15122==    by 0x58789C1: _i_NSRunLoop__acceptInputForMode_beforeDate_ 
(NSRunLoop.m:1064)
==15122==    by 0x5878D19: _i_NSRunLoop__runMode_beforeDate_ (NSRunLoop.m:1136)
==15122==    by 0x59F1A9: _i_PhapApplication__startApplication (in 
/usr/lib/infor-ap-5.0.0.0.39/PhapServer)
==15122==    by 0x46423F: main (in /usr/lib/infor-ap-5.0.0.0.39/PhapServer)
==15122==
==15122== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 5 from 2)
==15122== malloc/free: in use at exit: 5,661,049 bytes in 66,423 blocks.
==15122== malloc/free: 1,262,119 allocs, 1,195,696 frees, 284,349,236 bytes 
allocated.
==15122== For counts of detected errors, rerun with: -v
==15122== searching for pointers to 66,423 not-freed blocks.
==15122== checked 40,424,984 bytes.
==15122==
==15122== LEAK SUMMARY:
==15122==    definitely lost: 562,277 bytes in 11,847 blocks.
==15122==      possibly lost: 1,133,051 bytes in 18,001 blocks.
==15122==    still reachable: 3,965,721 bytes in 36,575 blocks.
==15122==         suppressed: 0 bytes in 0 blocks.
==15122== Use --leak-check=full to see details of leaked memory.


So it seems that an NSConnection is deallocated but still exists in the 
connection_table.  

Looking at the NSConnection code,  a connection is added to this table in the 
-init... method, and removed in the -invalidate method.

The -invalidate should be called indirectly by the -release method which looks 
like:


- (void) release
{
  /*
   *    If this would cause the connection to be deallocated then we
   *    must perform all necessary work (done in [-gcFinalize]).
   *    We bracket the code with a retain and release so that any
   *    retain/release pairs in the code won't cause recursion.
   */
  if ([self retainCount] == 1)
    {
      [super retain];
      [self gcFinalize];
      [super release];
    }
  [super release];
}


To me this looks not very thread safe.   So I see if garding this
with the _refGate works.   But because it is so tricky to reproduce
(any logging seems to make the bug disappear) it is hard to confirm
if the fix is enough.


Wim Oudshoorn.