From 8c637bf3530a92d8e22776e0dfda3df24f21e5d2 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 25 Oct 2018 10:56:22 +0200
Subject: Fix race in rule synthesis logic

---
 build2/cc/link-rule.cxx | 41 +++++++++++++++++++++++++++++++++++------
 build2/rule.cxx         |  2 +-
 build2/target.hxx       | 18 ++++++++++++++----
 3 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx
index 1dd8726..0ff339d 100644
--- a/build2/cc/link-rule.cxx
+++ b/build2/cc/link-rule.cxx
@@ -857,6 +857,10 @@ namespace build2
             ? &search (t, (mod ? tt.bmi : tt.obj), rt.dir, rt.out, rt.name)
             : &rt;
 
+          const target_type& rtt (mod
+                                  ? (group ? bmi::static_type : tt.bmi)
+                                  : (group ? obj::static_type : tt.obj));
+
           // If this obj*{} already has prerequisites, then verify they are
           // "compatible" with what we are doing here. Otherwise, synthesize
           // the dependency. Note that we may also end up synthesizing with
@@ -864,7 +868,12 @@ namespace build2
           //
           bool verify (true);
 
-          if (!pt->has_prerequisites ())
+          // Note that we cannot use has_group_prerequisites() since the
+          // target is not yet matched. So we check the group directly. Of
+          // course, all of this is racy (see below).
+          //
+          if (!pt->has_prerequisites () &&
+              (!group || !rt.has_prerequisites ()))
           {
             prerequisites ps {p.as_prerequisite ()}; // Source.
 
@@ -922,9 +931,33 @@ namespace build2
               }
             }
 
-            // Note: add to the group, not the member.
+            // Note: adding to the group, not the member.
             //
             verify = !rt.prerequisites (move (ps));
+
+            // Recheck that the target still has no prerequisites. If that's
+            // no longer the case, then verify the result is compatible with
+            // what we need.
+            //
+            // Note that there are scenarios where we will not detect this or
+            // the detection will be racy. For example, thread 1 adds the
+            // prerequisite to the group and then thread 2, which doesn't use
+            // the group, adds the prerequisite to the member. This could be
+            // triggered by something like this (undetectable):
+            //
+            // lib{foo}: cxx{foo}
+            // exe{foo}: cxx{foo}
+            //
+            // Or this (detection is racy):
+            //
+            // lib{bar}: cxx{foo}
+            // liba{baz}: cxx{foo}
+            //
+            // The current feeling, however, is that in non-contrived cases
+            // (i.e., the source file is the same) this should be harmless.
+            //
+            if (!verify && group)
+              verify = pt->has_prerequisites ();
           }
 
           if (verify)
@@ -939,10 +972,6 @@ namespace build2
             // async match here and finish this verification in the "harvest"
             // loop below.
             //
-            const target_type& rtt (mod
-                                    ? (group ? bmi::static_type : tt.bmi)
-                                    : (group ? obj::static_type : tt.obj));
-
             resolve_group (a, *pt); // Not matched yet so resolve group.
 
             bool src (false);
diff --git a/build2/rule.cxx b/build2/rule.cxx
index fc80816..85f4e36 100644
--- a/build2/rule.cxx
+++ b/build2/rule.cxx
@@ -116,7 +116,7 @@ namespace build2
     // to unchanged. This is an important optimization on which quite a few
     // places that deal with predominantly static content rely.
     //
-    if (!t.has_prerequisites ())
+    if (!t.has_group_prerequisites ()) // Group as in match_prerequisites().
       return noop_recipe;
 
     // Match all the prerequisites.
diff --git a/build2/target.hxx b/build2/target.hxx
index d73a98c..fb1c68c 100644
--- a/build2/target.hxx
+++ b/build2/target.hxx
@@ -193,6 +193,10 @@ namespace build2
     // special target_state::group state. You would normally also use the
     // group_recipe for group members.
     //
+    // Note that the group-member link-up can happen anywhere between the
+    // member creation and rule matching so reading the group before the
+    // member has been matched can be racy.
+    //
     const target* group = nullptr;
 
     // What has been described above is a "normal" group. That is, there is
@@ -320,14 +324,20 @@ namespace build2
     bool
     prerequisites (prerequisites_type&&) const;
 
-    // Check if there are any prerequisites, taking into account group
-    // prerequisites.
+    // Check if there are any prerequisites. Note that the group version may
+    // be racy (see target::group).
     //
     bool
     has_prerequisites () const
     {
-      return !prerequisites ().empty () ||
-        (group != nullptr && !group->prerequisites ().empty ());
+      return !prerequisites ().empty ();
+    }
+
+    bool
+    has_group_prerequisites () const
+    {
+      return has_prerequisites () ||
+        (group != nullptr && !group->has_prerequisites ());
     }
 
   private:
-- 
cgit v1.1