Possible dangling pointer when loading a weak reference about to deallocate?

[Graham--M]

I'm having a some trouble comprehending the reference counting code in arc.mm.

As I understand it:

• The most significant bit/sign bit is used to indicate that a weak reference has been taken.
• The other bits are for the reference count.
• Instances start with a reference count of 0 (which indicates the instance has only one strong reference).
• When all the reference count bits are set, this means that the reference count has saturated...
• ...or when releasing from a reference count of 0, the underflow indicates the instance should be destroyed.
• When the reference count has saturated, further retains and releases are ignored and the instance has a permanent lifetime.

My main concern is in objc_retain_fast_np() where here, it tries to handle the case where one thread has done the final release of a strong reference but before it can grab the weak reference lock to remove the WeakRef from the map and set the pointer to nil, another thread attempts to load and retain the instance through the weak reference.

I can't see how realCount < 0 can ever happen because the sign bit is masked off. When I disassemble the function in a release build, that branch has been removed by the optimisation:

(gdb) disassemble objc_retain_fast_np
Dump of assembler code for function objc_retain_fast_np:
   0x00007ffff7fb5ad0 <+0>:     movabs r8,0x7fffffffffffffff
   0x00007ffff7fb5ada <+10>:    mfence 
   0x00007ffff7fb5add <+13>:    mov    rax,QWORD PTR [rdi-0x8]
   0x00007ffff7fb5ae1 <+17>:    lea    rdx,[r8+0x1]
   0x00007ffff7fb5ae5 <+21>:    nop    WORD PTR cs:[rax+rax*1+0x0]
   0x00007ffff7fb5aef <+31>:    nop
   0x00007ffff7fb5af0 <+32>:    mov    rsi,rax
   0x00007ffff7fb5af3 <+35>:    and    rsi,r8
   0x00007ffff7fb5af6 <+38>:    cmp    rsi,r8
   0x00007ffff7fb5af9 <+41>:    je     0x7ffff7fb5b10 <objc_retain_fast_np+64>
   0x00007ffff7fb5afb <+43>:    add    rsi,0x1
   0x00007ffff7fb5aff <+47>:    mov    rcx,rax
   0x00007ffff7fb5b02 <+50>:    and    rcx,rdx
   0x00007ffff7fb5b05 <+53>:    or     rcx,rsi
   0x00007ffff7fb5b08 <+56>:    lock cmpxchg QWORD PTR [rdi-0x8],rcx
   0x00007ffff7fb5b0e <+62>:    jne    0x7ffff7fb5af0 <objc_retain_fast_np+32>
   0x00007ffff7fb5b10 <+64>:    mov    rax,rdi
   0x00007ffff7fb5b13 <+67>:    ret    
End of assembler dump.

So it looks like the retain always returns a pointer to the instance. setObjectHasWeakRefs() has a similar logic.

I'm also wondering how the difference between a saturated count and an instance deallocating is handled. If you handle the case where the instance is deallocating, wouldn't it mean that you can never load and retain a weak pointer to an instance that has a saturated retain count?

[Graham--M]

I did a crude simulation of the releasing thread getting pre-empted by a thread loading a weak ref. I used a debug build of the runtime with this code in gdb and set breakpoints on thread_func() and objc_delete_weak_refs() and set scheduler-locking to on, we reach a state where the release of the test object is about to grab the weakRefLock but gets beaten to it by the other thread.

When I swapped to the spawned thread and step through it, the thread's retain sees realCount == refcount_mask and gets what it thinks is a strong reference. If I then swap back to the main thread, it continues onward grabbing the weakRefLock, deleting/zeroing the weak reference pointer, and sending the dealloc message.

I think this maybe implements the intended behaviour?

[davidchisnall]

Thanks, I think you're right. I believe you'd need to have a bug in your code to actually trigger the overflow condition, because you can't have enough pointers to an object to hit the overflow case unless you have more pointers than will fit in the address space (even with retain + autorelease, we allocate a pointer to hold the autoreleased object). That said, if we steal more bits from the refcount in the future then this becomes feasible, so it's worth ensuring that the logic correctly handles it.

As you say, the < 0 check was not correct after stealing the high bit. This was meant to be sign extended, but wasn't. Checking for the all-ones value is probably correct. You fix looks correct, please could you put it in a PR?