From 4ca5a5bc2991438602d3b1fdb56b91d2b425c52d Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 17 Jul 2020 17:17:55 +0200 Subject: Fix race in path/mtime assignment and file_rule::match() --- libbuild2/bash/rule.cxx | 7 ++----- libbuild2/cc/common.cxx | 24 +++++++----------------- libbuild2/cc/msvc.cxx | 8 ++------ libbuild2/rule.cxx | 3 +++ libbuild2/search.cxx | 3 +-- libbuild2/target.hxx | 13 +++++++++++++ libbuild2/target.ixx | 15 +++++++++++++++ 7 files changed, 43 insertions(+), 30 deletions(-) (limited to 'libbuild2') 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 ()); - // 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 (*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 ()); 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:: -- cgit v1.1