diff mbox

[Ada] Fix obscure race condition in term alts

Message ID 20110804100120.GA21273@adacore.com
State New
Headers show

Commit Message

Arnaud Charlet Aug. 4, 2011, 10:01 a.m. UTC 
This patch is fixes an obscure race condition in the
implementation of terminate alternatives.
No small test case is available.

Tested on x86_64-pc-linux-gnu, committed on trunk

2011-08-04  Bob Duff  <duff@adacore.com>

	* s-tasren.adb (Task_Do_Or_Queue): Previous code was reading
	Acceptor.Terminate_Alternative without locking Acceptor, which causes a
	race condition whose symptom is to fail to lock Parent. That, in turn,
	causes Parent.Awake_Count to be accessed without locking Parent, which
	causes another race condition whose symptom is that Parent.Awake_Count
	can be off by 1 (either too high or too low). The solution is to lock
	Parent unconditionally, and then lock Acceptor, before reading
	Acceptor.Terminate_Alternative.
diff mbox

Patch

Index: s-tasren.adb
===================================================================
--- s-tasren.adb	(revision 177274)
+++ s-tasren.adb	(working copy)
@@ -6,7 +6,7 @@ 
 --                                                                          --
 --                                 B o d y                                  --
 --                                                                          --
---         Copyright (C) 1992-2010, Free Software Foundation, Inc.          --
+--         Copyright (C) 1992-2011, Free Software Foundation, Inc.          --
 --                                                                          --
 -- GNARL is free software; you can  redistribute it  and/or modify it under --
 -- terms of the  GNU General Public License as published  by the Free Soft- --
@@ -1077,7 +1077,6 @@ 
       Old_State     : constant Entry_Call_State := Entry_Call.State;
       Acceptor      : constant Task_Id := Entry_Call.Called_Task;
       Parent        : constant Task_Id := Acceptor.Common.Parent;
-      Parent_Locked : Boolean := False;
       Null_Body     : Boolean;
 
    begin
@@ -1105,25 +1104,24 @@ 
       --  record for another call.
       --  We rely on the Caller's lock for call State mod's.
 
-      --  We can't lock Acceptor.Parent while holding Acceptor,
-      --  so lock it in advance if we expect to need to lock it.
+      --  If Acceptor.Terminate_Alternative is True, we need to lock Parent and
+      --  Acceptor, in that order; otherwise, we only need a lock on
+      --  Acceptor. However, we can't check Acceptor.Terminate_Alternative
+      --  until Acceptor is locked. Therefore, we need to lock both. Attempts
+      --  to avoid locking Parent tend to result in race conditions. It would
+      --  work to unlock Parent immediately upon finding
+      --  Acceptor.Terminate_Alternative to be False, but that violates the
+      --  rule of properly nested locking (see System.Tasking).
 
-      if Acceptor.Terminate_Alternative then
-         STPO.Write_Lock (Parent);
-         Parent_Locked := True;
-      end if;
-
+      STPO.Write_Lock (Parent);
       STPO.Write_Lock (Acceptor);
 
       --  If the acceptor is not callable, abort the call and return False
 
       if not Acceptor.Callable then
          STPO.Unlock (Acceptor);
+         STPO.Unlock (Parent);
 
-         if Parent_Locked then
-            STPO.Unlock (Parent);
-         end if;
-
          pragma Assert (Entry_Call.State < Done);
 
          --  In case we are not the caller, set up the caller
@@ -1186,11 +1184,8 @@ 
 
                   STPO.Wakeup (Acceptor, Acceptor_Sleep);
                   STPO.Unlock (Acceptor);
+                  STPO.Unlock (Parent);
 
-                  if Parent_Locked then
-                     STPO.Unlock (Parent);
-                  end if;
-
                   STPO.Write_Lock (Entry_Call.Self);
                   Initialization.Wakeup_Entry_Caller
                     (Self_ID, Entry_Call, Done);
@@ -1207,10 +1202,7 @@ 
                   end if;
 
                   STPO.Unlock (Acceptor);
-
-                  if Parent_Locked then
-                     STPO.Unlock (Parent);
-                  end if;
+                  STPO.Unlock (Parent);
                end if;
 
                return True;
@@ -1236,11 +1228,8 @@ 
             and then Entry_Call.Cancellation_Attempted)
       then
          STPO.Unlock (Acceptor);
+         STPO.Unlock (Parent);
 
-         if Parent_Locked then
-            STPO.Unlock (Parent);
-         end if;
-
          STPO.Write_Lock (Entry_Call.Self);
 
          pragma Assert (Entry_Call.State >= Was_Abortable);
@@ -1261,11 +1250,8 @@ 
            New_State (Entry_Call.With_Abort, Entry_Call.State);
 
          STPO.Unlock (Acceptor);
+         STPO.Unlock (Parent);
 
-         if Parent_Locked then
-            STPO.Unlock (Parent);
-         end if;
-
          if Old_State /= Entry_Call.State
            and then Entry_Call.State = Now_Abortable
            and then Entry_Call.Mode /= Simple_Call