AdaCore / AdaCore/gnatcoll-core

Race condition in Weak References implementation

Open
#29 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ada
Stars
55
Forks
36
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the Weak References implementation and the Proxied read, reference-counter, and destruction paths mentioned in the report. Review the existing concurrency tests, if any, and establish a reproducible reader-versus-destruction case. Done means weak-reference resolution cannot access freed memory; the issue names no repository files or tests.

Written by the indexing model from the issue text.

Assessment

Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.