From 066980729c57e00abc3765053cf3f39e3d223a54 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Fri, 3 Nov 2023 08:05:46 +0200
Subject: Fix data race in match options logic

---
 libbuild2/algorithm.cxx | 23 ++++++++++++++++++++++-
 libbuild2/algorithm.ixx |  1 +
 libbuild2/target.hxx    | 22 +++++++++++++---------
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 1f2d88b..0e995d5 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -291,7 +291,12 @@ namespace build2
       // We don't lock already applied or executed targets unless there
       // are new options.
       //
-      if (e >= appl && (cs.match_extra.cur_options & options) == options)
+      // Note: we don't have the lock yet so we must use atomic cur_options.
+      // We also have to re-check this once we've grabbed the lock.
+      //
+      if (e >= appl &&
+          (cs.match_extra.cur_options_.load (memory_order_relaxed) & options)
+          == options)
         return target_lock {a, nullptr, e - b, false};
     }
 
@@ -312,7 +317,21 @@ namespace build2
       offset = target::offset_touched;
     }
     else
+    {
+      // Re-check the options if already applied or worse.
+      //
+      if (e >= appl && (s.match_extra.cur_options & options) == options)
+      {
+        // Essentially unlock_impl().
+        //
+        task_count.store (e, memory_order_release);
+        ctx.sched->resume (task_count);
+
+        return target_lock {a, nullptr, e - b, false};
+      }
+
       offset = e - b;
+    }
 
     return target_lock {a, &t, offset, first};
   }
@@ -962,6 +981,7 @@ namespace build2
     recipe re (ar != nullptr ? f (*ar, a, t, me) : ru.apply (a, t, me));
 
     me.free (); // Note: cur_options are still in use.
+    me.cur_options_.store (me.cur_options, memory_order_relaxed);
     return re;
   }
 
@@ -1024,6 +1044,7 @@ namespace build2
     // Note: for now no adhoc_reapply().
     //
     ru.reapply (a, t, me);
+    me.cur_options_.store (me.cur_options, memory_order_relaxed);
   }
 
   // If anything goes wrong, set target state to failed and return nullopt.
diff --git a/libbuild2/algorithm.ixx b/libbuild2/algorithm.ixx
index bae9151..e384433 100644
--- a/libbuild2/algorithm.ixx
+++ b/libbuild2/algorithm.ixx
@@ -622,6 +622,7 @@ namespace build2
 
     me.reinit (false /* fallback */);
     me.cur_options = options; // Already applied, so cur_, not new_options.
+    me.cur_options_.store (me.cur_options, memory_order_relaxed);
     clear_target (l.action, *l.target);
     set_rule (l, nullptr); // No rule.
     set_recipe (l, move (r));
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 83e6994..c61f241 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -221,14 +221,16 @@ namespace build2
     // rematch, if current options already include new options, then no call
     // to reapply() is made. This, in particular, means that a rule that does
     // not adjust cur_options in match() will never get a reapply() call
-    // (because all the options are enabled from the start). If a rematch is
-    // triggered after the rule has already been executed, an error is issued.
-    // This means that match options are not usable for operation/target types
-    // that could plausibly be executed during match. In particular, using
-    // match options for update and clean operations is a bad idea (update of
-    // pretty much any target can happen during match as a result of a tool
-    // update while clean might have to be performed during match to provide
-    // the mirror semantics).
+    // (because all the options are enabled from the start). Note that
+    // cur_options should only be modfied in apply() or reapply().
+    //
+    // If a rematch is triggered after the rule has already been executed, an
+    // error is issued.  This means that match options are not usable for
+    // operation/target types that could plausibly be executed during
+    // match. In particular, using match options for update and clean
+    // operations is a bad idea (update of pretty much any target can happen
+    // during match as a result of a tool update while clean might have to be
+    // performed during match to provide the mirror semantics).
     //
     // Note also that with rematches the assumption that in the match phase
     // after matching the target we can MT-safely examine its state (such as
@@ -264,10 +266,12 @@ namespace build2
     // Note: match options are currently not exposed in Buildscript ad hoc
     // recipes/rules (but are in C++).
     //
+    static constexpr uint64_t all_options = ~uint64_t (0);
+
     uint64_t cur_options;
     uint64_t new_options;
 
-    static constexpr uint64_t all_options = ~uint64_t (0);
+    atomic<uint64_t> cur_options_; // Implementation detail (see lock_impl()).
 
     // The list of post hoc prerequisite targets for this target. Only not
     // NULL in rule::apply_posthoc() and rule::reapply() functions and only if
-- 
cgit v1.1