aboutsummaryrefslogtreecommitdiff
path: root/libbuild2
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2020-07-17 17:17:55 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2020-07-17 17:17:55 +0200
commit4ca5a5bc2991438602d3b1fdb56b91d2b425c52d (patch)
treeb9a2f6f801031af9299bfc45095eac1eb3ceb495 /libbuild2
parentab4a9ef42e8f1070dcb5d783a5afccd2f685e86d (diff)
Fix race in path/mtime assignment and file_rule::match()
Diffstat (limited to 'libbuild2')
-rw-r--r--libbuild2/bash/rule.cxx7
-rw-r--r--libbuild2/cc/common.cxx24
-rw-r--r--libbuild2/cc/msvc.cxx8
-rw-r--r--libbuild2/rule.cxx3
-rw-r--r--libbuild2/search.cxx3
-rw-r--r--libbuild2/target.hxx13
-rw-r--r--libbuild2/target.ixx15
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::