NSConnection bugs

[David Chisnall]

Hello the list,

I spent some time with valgrind and found out why NSConnection is so flakey on 
64-bit platforms.  It seems that we use the same incorrect pattern in a lot of 
places throughout the class.  We probably use it in other places, so it would 
be worth doing a brief audit for this bug before the next release.  I had a 
quick look, and I think that NSConnection is the only place where we use GSIMap 
with something other than pointers.

The problem is in statements like this:

      node = GSIMapNodeForKey(IreplyMap, (GSIMapKey)seq);

Looks fine?  The problem is that (GSIMapKey) is a union (something concealed by 
the fact that it is a macro - unions are difficult to use safely, and anything 
that hides the fact that you are using a union is generally a bad thing).  
Specifically, it's a problem that GSIMapKey is a union of void*, id, unsigned, 
and int.  On most current 64-bit platforms, unsigned and int are 32-bit 
quantities, while void* and id are 64-bit quantities. 

The problem, therefore, is that the we are defining 4 bytes of an 8-byte 
quantity and then (in the callee) accessing all 8 bytes.  This results in some 
random behaviour.  The connection.m test was failing because sometimes the 
uninitialised bytes were 0 (so the map was finding the value it was looking 
for) and sometimes they were some other value from the stack.

This bug is completely hidden by the tangle of defines in GSIMap.h, in 
combination with the GCC cast-to-union extension.  I'm not really convinced 
that GSIMap should be used directly anywhere - if we'd used NSMapTable then 
this bug would not have existed, because all keys and values would have been 
cast to pointer-sized quantities, and the compiler would have warned us when we 
were doing a bad cast.  

A simple fix ought to be to use NS[U]Integer instead of [unsigned] int here, 
but it seems that NSConnection uses int and unsigned in a seemingly random 
fashion, and I'm not sure exactly where it's safe to make this change (for 
example, we seem to be decoding ints from port coders, so fixing this bug may 
involve breaking the wire protocol if not done very carefully...)

An ugly-hack fix would be a global replace of (GSIMapKey) with 
(GSIMapKey)(id)(NSInteger).  Doing this (26 replacements - this bug appears in 
a lot of places) makes the connection.m test pass for me consistently on 
Linux/x86-64.  I've committed that for now, but someone who understands what is 
meant to be happening in NSConnection should take a more detailed look.

David

-- Sent from my Difference Engine