Fix race in path/mtime assignment and file_rule::match()

7 files changed, 43 insertions, 30 deletions

diff --git a/libbuild2/bash/rule.cxx b/libbuild2/bash/rule.cxx
index 801f02c..0df9792 100644
--- a/libbuild2/bash/rule.cxx
+++ b/libbuild2/bash/rule.cxx
@@ -166,13 +166,10 @@ namespace build2
 
                   bash& pt (rp.first.as<bash> ());
 
-                  // Only set mtime/path on first insertion.
+                  // Only set path/mtime on first insertion.
                   //
                   if (rp.second.owns_lock ())
-                  {
-                    pt.mtime (mt);
-                    pt.path (move (ap));
-                  }
+                    pt.path_mtime (move (ap), mt);
 
                   // Save the length of the import path in auxuliary data. We
                   // use it in substitute_import() to infer the installation
diff --git a/libbuild2/cc/common.cxx b/libbuild2/cc/common.cxx
index e11dea2..f848003 100644
--- a/libbuild2/cc/common.cxx
+++ b/libbuild2/cc/common.cxx
@@ -651,25 +651,20 @@ namespace build2
                 else
                   assert (find_adhoc_member<libi> (*s) == i);
 
-                i->mtime (mt);
-                i->path (move (f));
-
                 // Presumably there is a DLL somewhere, we just don't know
                 // where (and its possible we might have to look for one if we
                 // decide we need to do rpath emulation for installed
                 // libraries as well). We will represent this as empty path
                 // but valid timestamp (aka "trust me, it's there").
                 //
-                s->mtime (mt);
-                s->path (empty_path);
+                i->path_mtime (move (f), mt);
+                s->path_mtime (path (), mt);
               }
             }
             else
             {
               insert_library (ctx, s, name, d, ld, se, exist, trace);
-
-              s->mtime (mt);
-              s->path (move (f));
+              s->path_mtime (move (f), mt);
             }
           }
           else if (!ext && tsys == "mingw32")
@@ -687,9 +682,7 @@ namespace build2
             if (mt != timestamp_nonexistent)
             {
               insert_library (ctx, s, name, d, ld, se, exist, trace);
-
-              s->mtime (mt);
-              s->path (move (f));
+              s->path_mtime (move (f), mt);
             }
           }
         }
@@ -712,8 +705,7 @@ namespace build2
             // as out trees.
             //
             insert_library (ctx, a, name, d, ld, ae, exist, trace);
-            a->mtime (mt);
-            a->path (move (f));
+            a->path_mtime (move (f), mt);
           }
         }
 
@@ -747,15 +739,13 @@ namespace build2
             if (na && !r.first.empty ())
             {
               insert_library (ctx, a, name, d, ld, nullopt, exist, trace);
-              a->mtime (timestamp_unreal);
-              a->path (empty_path);
+              a->path_mtime (path (), timestamp_unreal);
             }
 
             if (ns && !r.second.empty ())
             {
               insert_library (ctx, s, name, d, ld, nullopt, exist, trace);
-              s->mtime (timestamp_unreal);
-              s->path (empty_path);
+              s->path_mtime (path (), timestamp_unreal);
             }
 
             // Only keep these .pc paths if we found anything via them.
diff --git a/libbuild2/cc/msvc.cxx b/libbuild2/cc/msvc.cxx
index 20239c7..8f0a853 100644
--- a/libbuild2/cc/msvc.cxx
+++ b/libbuild2/cc/msvc.cxx
@@ -528,10 +528,7 @@ namespace build2
         //
         T* t;
         common::insert_library (p.scope->ctx, t, name, d, ld, e, exist, trace);
-
-        t->mtime (mt);
-        t->path (move (f));
-
+        t->path_mtime (move (f), mt);
         return t;
       }
 
@@ -603,8 +600,7 @@ namespace build2
 
             // Presumably there is a DLL somewhere, we just don't know where.
             //
-            s->mtime (i->mtime ());
-            s->path (path ());
+            s->path_mtime (path (), i->mtime ());
           }
         }
 
diff --git a/libbuild2/rule.cxx b/libbuild2/rule.cxx
index 4b2d69d..ac5b310 100644
--- a/libbuild2/rule.cxx
+++ b/libbuild2/rule.cxx
@@ -71,6 +71,9 @@ namespace build2
         // me, this file exists" situations (used, for example, for installed
         // stuff where we know it's there, just not exactly where).
         //
+        // See also path_target::path_mtime() for a potential race in this
+        // logic.
+        //
         mtime_target& mt (t.as<mtime_target> ());
 
         timestamp ts (mt.mtime ());
diff --git a/libbuild2/search.cxx b/libbuild2/search.cxx
index 5887138..8f5410c 100644
--- a/libbuild2/search.cxx
+++ b/libbuild2/search.cxx
@@ -194,8 +194,7 @@ namespace build2
     l5 ([&]{trace << (r.second ? "new" : "existing") << " target " << t
                   << " for prerequisite " << cpk;});
 
-    t.mtime (mt);
-    t.path (move (f));
+    t.path_mtime (move (f), mt);
 
     return &t;
   }
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 6022274..55b8270 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -1460,6 +1460,9 @@ namespace build2
     // Note also that while we can cache the mtime, it may be ignored if the
     // target state is set to group (see above).
     //
+    // NOTE: if setting both path and mtime (typically during match), then use
+    // the path_target::path_mtime() function to do it in the correct order.
+    //
     void
     mtime (timestamp) const;
 
@@ -1547,12 +1550,22 @@ namespace build2
     // lock the target in some other way; see file_rule) so in this case it
     // makes sense to set the timestamp first.
     //
+    // NOTE: if setting both path and mtime (typically during match), then use
+    // the path_mtime() function to do it in the correct order.
+    //
     const path_type&
     path () const;
 
     const path_type&
     path (path_type) const;
 
+    // Set both path and mtime and in the correct order.
+    //
+    const path_type&
+    path_mtime (path_type, timestamp) const;
+
+    // Load mtime using the cached path.
+    //
     timestamp
     load_mtime () const;
 
diff --git a/libbuild2/target.ixx b/libbuild2/target.ixx
index 3d92c78..94c54c0 100644
--- a/libbuild2/target.ixx
+++ b/libbuild2/target.ixx
@@ -582,6 +582,21 @@ namespace build2
     return mtime_target::load_mtime (path ());
   }
 
+  inline const path& path_target::
+  path_mtime (path_type p, timestamp mt) const
+  {
+    // Because we use the presence of mtime to indicate the special "trust me,
+    // this file exists" situation, the order in which we do things is
+    // important. In particular, the fallback file_rule::match() will skip
+    // assigning the path if there is a valid timestamp. As a result, with the
+    // wrong order we may end up in a situation where the rule is matched but
+    // the path is not assigned.
+    //
+    const path_type& r (path (move (p)));
+    mtime (mt);
+    return r;
+  }
+
   // exe
   //
   inline auto exe::