Race condition in Weak References implementation

[OCTAGRAM]

There is a possible race condition in between Proxied read and test for reference counter = 0 in Proxied. A thread resolving weak reference could have read non-null Proxied into register and preemted, and after resume it will access invalid memory. Proxied is null on resume, but this check is passed.

The only good protection against it that comes to my mind is to introduce integer counter in proxy and increment it when reading it. This counter must be strongly associated with Proxied and modified atomically together with it using cmgxchg8b, cmpxchg16b or mutex.

Pseudocode for resolving weak reference:

declare
   Saved_Proxied : Proxy_Access;
   Saved_Lock : Atomic_Unsigned;
begin
   loop
      Saved_Proxied := Proxy.Proxied;
      if Saved_Proxied = null then
         return Null_Reference;
      end if;
      Saved_Lock := Proxy.Lock;
      exit when Proxy.Compare_And_Swap
        (Proxy.Proxied, Proxy.Lock,
         Saved_Proxied, Saved_Lock,
         Saved_Proxied, Saved_Lock + 1);
   end loop;

   if Saved_Proxied.Reference_Counter = 0 or Saved_Proxied.Is_Beeing_Freed then
      loop
         Saved_Proxied := Proxy.Proxied;
         Saved_Lock := Proxy.Lock;
         exit when Proxy.Compare_And_Swap
           (Proxy.Proxied, Proxy.Lock,
            Saved_Proxied, Saved_Lock,
            Saved_Proxied, Saved_Lock - 1);
      end loop;
      
      return Null_Reference;
   end if;

   return Result : Reference := Create_Reference (Saved_Proxied) do
      loop
         Saved_Proxied := Proxy.Proxied;
         if Saved_Proxied = null then
            Result := Null_Reference;
            loop
               Saved_Proxied := Proxy.Proxied;
               Saved_Lock := Proxy.Lock;
               exit when Proxy.Compare_And_Swap
                 (Proxy.Proxied, Proxy.Lock,
                  Saved_Proxied, Saved_Lock,
                  Saved_Proxied, Saved_Lock - 1);
            end loop;
            return;
         end if;
         Saved_Lock := Proxy.Lock;
         exit when Proxy.Compare_And_Swap
           (Proxy.Proxied, Proxy.Lock,
            Saved_Proxied, Saved_Lock,
            Saved_Proxied, Saved_Lock - 1);
      end loop;
   end return;
end;

Pseudocode for zeroing Proxied:

declare
   Saved_Proxied : Proxy_Access;
   Saved_Lock : Atomic_Unsigned;
begin
   loop
      Saved_Proxied := Proxy.Proxied;
      exit when Saved_Proxied = null;
      Saved_Lock := Proxy.Lock;
      exit when
         Proxy.Compare_And_Swap
           (Proxy.Proxied, Proxy.Lock,
            Saved_Proxied, Saved_Lock,
            null, Saved_Lock);
   end loop;
   
   while Saved_Lock /= 0 loop
      Saved_Lock := Proxy.Lock;
   end loop;
end;

This way reading Proxied becomes atomic. Reader thread technically reads value (Saved_Proxied := Proxy.Proxied), but cannot legally do anything useful with it, even dereference, until it puts a lock. Thread that is going to destroy object, first zeroes Proxy.Proxied, so that new threads can no longer appear, then waits for every remaining reader thread to abort. Then it can safely deallocate memory.

[anisimkov]

Can you provide complete example showing the problem ?

[anisimkov]

No need, I made reproducer.

[anisimkov]

Note that GNATCOLL.Refcount.Weakref interface is obsolete. It is recommended to use Shared_Pointers from GNATCOLL.Refcount.

[anisimkov]

Shared_Pointer race condition fixed, obsolete Smart_Pointer still in danger.