fix: Avoid race condition in inode cache for quick updates

The inode cache has a race condition that consists of these events:

1. A file is written with content C1, size S and timestamp (ctime/mtime)
   T.
2. Ccache hashes the file content and asks the inode cache to store the
   digest with a hash of S and T (and some other data) as the key.
3. The file is quickly thereafter written with content C2 without
   changing size S and timestamp T. The timestamp is not updated since
   the file writes are made within a time interval smaller than the
   granularity of the clock used for file system timestamps. At the time
   of writing, a common granularity on a Linux system is 0.004 s (250
   Hz).
4. The inode cache is asked for the file digest and the inode cache
   delivers a digest of C1 even though the file's content is C2.

To avoid the race condition, the inode cache now only caches inodes
whose timestamp was updated more than two seconds ago. This conservative
value is chosen since not all file systems have subsecond resolution.

Fixes #1215.

diff --git a/src/InodeCache.cpp b/src/InodeCache.cpp
index 6ff430d38c46d50749e1ba5739d93f689a77722e..934227916987062afe9507cddd4902ea4336c632 100644 (file)
--- a/src/InodeCache.cpp
+++ b/src/InodeCache.cpp
@@ -29,6 +29,8 @@
 #include "Util.hpp"
 #include "fmtmacros.hpp"
 
+#include <util/TimePoint.hpp>
+
 #include <fcntl.h>
 #include <libgen.h>
 #include <sys/mman.h>
@@ -225,6 +227,13 @@ InodeCache::hash_inode(const std::string& path,
     return false;
   }
 
+  // See comment for InodeCache::InodeCache why this check is done.
+  auto now = util::TimePoint::now();
+  if (now - stat.ctime() < m_min_age || now - stat.mtime() < m_min_age) {
+    LOG("Too new ctime or mtime of {}, not considering for inode cache", path);
+    return false;
+  }
+
   Key key;
   memset(&key, 0, sizeof(Key));
   key.type = type;
@@ -375,7 +384,12 @@ InodeCache::initialize()
   return false;
 }
 
-InodeCache::InodeCache(const Config& config) : m_config(config)
+InodeCache::InodeCache(const Config& config, util::Duration min_age)
+  : m_config(config),
+    // CCACHE_DISABLE_INODE_CACHE_MIN_AGE is only for testing purposes; see
+    // test/suites/inode_cache.bash.
+    m_min_age(getenv("CCACHE_DISABLE_INODE_CACHE_MIN_AGE") ? util::Duration(0)
+                                                           : min_age)
 {
 }
 

diff --git a/src/InodeCache.hpp b/src/InodeCache.hpp
index 6bb11a5c435ffd20a8bfd07f8411bd92ef8e03ec..c61f4b8e8c7e635f122cb1da57c427765a612450 100644 (file)
--- a/src/InodeCache.hpp
+++ b/src/InodeCache.hpp
@@ -18,6 +18,8 @@
 
 #pragma once
 
+#include <util/Duration.hpp>
+
 #include <cstdint>
 #include <functional>
 #include <string>
@@ -41,7 +43,26 @@ public:
     checked_for_temporal_macros = 1,
   };
 
-  InodeCache(const Config& config);
+  // `min_age` specifies how old a file must be to be put in the cache. The
+  // reason for this is that there is a race condition that consists of these
+  // events:
+  //
+  // 1. A file is written with content C1, size S and timestamp (ctime/mtime) T.
+  // 2. Ccache hashes the file content and asks the inode cache to store the
+  //    digest with a hash of S and T (and some other dataa) as the key.
+  // 3. The file is quickly thereafter written with content C2 without changing
+  //    size S and timestamp T. The timestamp is not updated since the file
+  //    writes are made within a time interval smaller than the granularity of
+  //    the clock used for file system timestamps. At the time of writing, a
+  //    common granularity on a Linux system is 0.004 s (250 Hz).
+  // 4. The inode cache is asked for the file digest and the inode cache
+  //    delivers a digest of C1 even though the file's content is C2.
+  //
+  // To avoid the race condition, the inode cache only caches inodes whose
+  // timestamp was updated more than `min_age` ago. The default value is a
+  // conservative 2 seconds since not all file systems have subsecond
+  // resolution.
+  InodeCache(const Config& config, util::Duration min_age = util::Duration(2));
   ~InodeCache();
 
   // Return whether it's possible to use the inode cache on the filesystem
@@ -101,14 +122,14 @@ private:
   using BucketHandler = std::function<void(Bucket* bucket)>;
 
   bool mmap_file(const std::string& inode_cache_file);
-  static bool
-  hash_inode(const std::string& path, ContentType type, Digest& digest);
+  bool hash_inode(const std::string& path, ContentType type, Digest& digest);
   bool with_bucket(const Digest& key_digest,
                    const BucketHandler& bucket_handler);
   static bool create_new_file(const std::string& filename);
   bool initialize();
 
   const Config& m_config;
+  util::Duration m_min_age;
   struct SharedRegion* m_sr = nullptr;
   bool m_failed = false;
 };

diff --git a/test/suites/inode_cache.bash b/test/suites/inode_cache.bash
index e53bf8b20b00d8c20fec449d0f5887387d970fed..830ef8c75d0d82969d5c0b0bab03ff12cde74ab2 100644 (file)
--- a/test/suites/inode_cache.bash
+++ b/test/suites/inode_cache.bash
@@ -19,6 +19,11 @@ SUITE_inode_cache_SETUP() {
     export CCACHE_DEBUG=1
     unset CCACHE_NODIRECT
     export CCACHE_TEMPDIR="${CCACHE_DIR}/tmp"  # isolate inode cache file
+
+    # Disable safety guard against race condition in InodeCache. This is OK
+    # since files used in the tests have different sizes and thus will have
+    # different cache keys even if ctime/mtime are not updated quickly enough.
+    export CCACHE_DISABLE_INODE_CACHE_MIN_AGE=1
 }
 
 SUITE_inode_cache() {

diff --git a/unittest/test_InodeCache.cpp b/unittest/test_InodeCache.cpp
index c546b1643bb8fc3c8d435cc9fcf7d12eee4d3d86..b02adab08eed4fa5a273101185aca676c8d8548d 100644 (file)
--- a/unittest/test_InodeCache.cpp
+++ b/unittest/test_InodeCache.cpp
@@ -74,7 +74,7 @@ TEST_CASE("Test disabled")
   Config config;
   init(config);
   config.set_inode_cache(false);
-  InodeCache inode_cache(config);
+  InodeCache inode_cache(config, util::Duration(0));
 
   Digest digest;
   int return_value;
@@ -99,7 +99,7 @@ TEST_CASE("Test lookup nonexistent")
   Config config;
   init(config);
 
-  InodeCache inode_cache(config);
+  InodeCache inode_cache(config, util::Duration(0));
   util::write_file("a", "");
 
   Digest digest;
@@ -121,7 +121,7 @@ TEST_CASE("Test put and lookup")
   Config config;
   init(config);
 
-  InodeCache inode_cache(config);
+  InodeCache inode_cache(config, util::Duration(0));
   util::write_file("a", "a text");
 
   CHECK(put(inode_cache, "a", "a text", 1));
@@ -169,7 +169,7 @@ TEST_CASE("Drop file")
   Config config;
   init(config);
 
-  InodeCache inode_cache(config);
+  InodeCache inode_cache(config, util::Duration(0));
 
   Digest digest;
 
@@ -187,7 +187,7 @@ TEST_CASE("Test content type")
   Config config;
   init(config);
 
-  InodeCache inode_cache(config);
+  InodeCache inode_cache(config, util::Duration(0));
   util::write_file("a", "a text");
   Digest binary_digest = Hash().hash("binary").digest();
   Digest code_digest = Hash().hash("code").digest();