From: Joel Rosdahl Date: Thu, 17 Nov 2022 20:31:58 +0000 (+0100) Subject: fix: Avoid race condition in inode cache for quick updates X-Git-Tag: v4.7.4~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3b6c2a5a63e104ed2090ddebe611182297fd1f9c;p=thirdparty%2Fccache.git 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 6ff430d38..934227916 100644 --- a/src/InodeCache.cpp +++ b/src/InodeCache.cpp @@ -29,6 +29,8 @@ #include "Util.hpp" #include "fmtmacros.hpp" +#include + #include #include #include @@ -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 6bb11a5c4..c61f4b8e8 100644 --- a/src/InodeCache.hpp +++ b/src/InodeCache.hpp @@ -18,6 +18,8 @@ #pragma once +#include + #include #include #include @@ -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; 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 e53bf8b20..830ef8c75 100644 --- 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 c546b1643..b02adab08 100644 --- 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();