From 9a1ec661147e6c2580a889e6f53d9c7a8bf73b69 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 19 Jan 2017 02:13:54 +0100 Subject: avoid validate/delete/load race in cache generation Keeping the Fd of the cache file we have validated around to later load it into the mmap ensures not only that we load the same file (which wouldn't really be a problem in practice), but that this file also still exists and wasn't deleted e.g. by a 'apt clean' call run in parallel. (cherry picked from commit 06606f073210fe3902fe92d5ff77fa1ab621b972) (cherry picked from commit 2e5726edcac4fc9228c6b16281365c3ade189b8b) --- apt-pkg/pkgcachegen.cc | 59 ++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index f0b5a982e..23b606e01 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -1363,24 +1363,27 @@ public: ScopedErrorRevert() { _error->PushToStack(); } ~ScopedErrorRevert() { _error->RevertToStack(); } }; -static bool CheckValidity(const string &CacheFile, +static bool CheckValidity(FileFd &CacheFile, std::string const &CacheFileName, pkgSourceList &List, FileIterator const Start, FileIterator const End, MMap **OutMap = 0, pkgCache **OutCache = 0) { + if (CacheFileName.empty()) + return false; ScopedErrorRevert ser; + bool const Debug = _config->FindB("Debug::pkgCacheGen", false); // No file, certainly invalid - if (CacheFile.empty() == true || FileExists(CacheFile) == false) + if (CacheFile.Open(CacheFileName, FileFd::ReadOnly, FileFd::None) == false) { if (Debug == true) - std::clog << "CacheFile " << CacheFile << " doesn't exist" << std::endl; + std::clog << "CacheFile " << CacheFileName << " doesn't exist" << std::endl; return false; } - if (List.GetLastModifiedTime() > GetModificationTime(CacheFile)) + if (List.GetLastModifiedTime() > CacheFile.ModificationTime()) { if (Debug == true) std::clog << "sources.list is newer than the cache" << std::endl; @@ -1388,8 +1391,7 @@ static bool CheckValidity(const string &CacheFile, } // Map it - FileFd CacheF(CacheFile,FileFd::ReadOnly); - std::unique_ptr Map(new MMap(CacheF,0)); + std::unique_ptr Map(new MMap(CacheFile,0)); if (unlikely(Map->validData()) == false) return false; std::unique_ptr CacheP(new pkgCache(Map.get())); @@ -1397,7 +1399,7 @@ static bool CheckValidity(const string &CacheFile, if (_error->PendingError() || Map->Size() == 0) { if (Debug == true) - std::clog << "Errors are pending or Map is empty() for " << CacheFile << std::endl; + std::clog << "Errors are pending or Map is empty() for " << CacheFileName << std::endl; return false; } @@ -1623,13 +1625,12 @@ static bool writeBackMMapToFile(pkgCacheGenerator * const Gen, DynamicMMap * con return true; } static bool loadBackMMapFromFile(std::unique_ptr &Gen, - std::unique_ptr &Map, OpProgress * const Progress, std::string const &FileName) + std::unique_ptr &Map, OpProgress * const Progress, FileFd &CacheF) { Map.reset(CreateDynamicMMap(NULL, 0)); if (unlikely(Map->validData()) == false) return false; - FileFd CacheF(FileName, FileFd::ReadOnly); - if (CacheF.IsOpen() == false || CacheF.Failed()) + if (CacheF.IsOpen() == false || CacheF.Seek(0) == false || CacheF.Failed()) return false; _error->PushToStack(); map_pointer_t const alloc = Map->RawAllocate(CacheF.Size()); @@ -1661,20 +1662,20 @@ bool pkgCacheGenerator::MakeStatusCache(pkgSourceList &List,OpProgress *Progress return false; // Decide if we can write to the files.. - string const CacheFile = _config->FindFile("Dir::Cache::pkgcache"); - string const SrcCacheFile = _config->FindFile("Dir::Cache::srcpkgcache"); + string const CacheFileName = _config->FindFile("Dir::Cache::pkgcache"); + string const SrcCacheFileName = _config->FindFile("Dir::Cache::srcpkgcache"); // ensure the cache directory exists - if (CacheFile.empty() == false || SrcCacheFile.empty() == false) + if (CacheFileName.empty() == false || SrcCacheFileName.empty() == false) { string dir = _config->FindDir("Dir::Cache"); size_t const len = dir.size(); if (len > 5 && dir.find("/apt/", len - 6, 5) == len - 5) dir = dir.substr(0, len - 5); - if (CacheFile.empty() == false) - CreateDirectory(dir, flNotFile(CacheFile)); - if (SrcCacheFile.empty() == false) - CreateDirectory(dir, flNotFile(SrcCacheFile)); + if (CacheFileName.empty() == false) + CreateDirectory(dir, flNotFile(CacheFileName)); + if (SrcCacheFileName.empty() == false) + CreateDirectory(dir, flNotFile(SrcCacheFileName)); } if (Progress != NULL) @@ -1683,8 +1684,8 @@ bool pkgCacheGenerator::MakeStatusCache(pkgSourceList &List,OpProgress *Progress bool pkgcache_fine = false; bool srcpkgcache_fine = false; bool volatile_fine = List.GetVolatileFiles().empty(); - - if (CheckValidity(CacheFile, List, Files.begin(), Files.end(), volatile_fine ? OutMap : NULL, + FileFd CacheFile; + if (CheckValidity(CacheFile, CacheFileName, List, Files.begin(), Files.end(), volatile_fine ? OutMap : NULL, volatile_fine ? OutCache : NULL) == true) { if (Debug == true) @@ -1692,9 +1693,11 @@ bool pkgCacheGenerator::MakeStatusCache(pkgSourceList &List,OpProgress *Progress pkgcache_fine = true; srcpkgcache_fine = true; } + + FileFd SrcCacheFile; if (pkgcache_fine == false) { - if (CheckValidity(SrcCacheFile, List, Files.end(), Files.end()) == true) + if (CheckValidity(SrcCacheFile, SrcCacheFileName, List, Files.end(), Files.end()) == true) { if (Debug == true) std::clog << "srcpkgcache.bin is valid - it can be reused" << std::endl; @@ -1712,10 +1715,10 @@ bool pkgCacheGenerator::MakeStatusCache(pkgSourceList &List,OpProgress *Progress bool Writeable = false; if (srcpkgcache_fine == false || pkgcache_fine == false) { - if (CacheFile.empty() == false) - Writeable = access(flNotFile(CacheFile).c_str(),W_OK) == 0; - else if (SrcCacheFile.empty() == false) - Writeable = access(flNotFile(SrcCacheFile).c_str(),W_OK) == 0; + if (CacheFileName.empty() == false) + Writeable = access(flNotFile(CacheFileName).c_str(),W_OK) == 0; + else if (SrcCacheFileName.empty() == false) + Writeable = access(flNotFile(SrcCacheFileName).c_str(),W_OK) == 0; if (Debug == true) std::clog << "Do we have write-access to the cache files? " << (Writeable ? "YES" : "NO") << std::endl; @@ -1754,8 +1757,8 @@ bool pkgCacheGenerator::MakeStatusCache(pkgSourceList &List,OpProgress *Progress Files.end(),Files.end()) == false) return false; - if (Writeable == true && SrcCacheFile.empty() == false) - if (writeBackMMapToFile(Gen.get(), Map.get(), SrcCacheFile) == false) + if (Writeable == true && SrcCacheFileName.empty() == false) + if (writeBackMMapToFile(Gen.get(), Map.get(), SrcCacheFileName) == false) return false; } @@ -1767,8 +1770,8 @@ bool pkgCacheGenerator::MakeStatusCache(pkgSourceList &List,OpProgress *Progress Files.begin(), Files.end()) == false) return false; - if (Writeable == true && CacheFile.empty() == false) - if (writeBackMMapToFile(Gen.get(), Map.get(), CacheFile) == false) + if (Writeable == true && CacheFileName.empty() == false) + if (writeBackMMapToFile(Gen.get(), Map.get(), CacheFileName) == false) return false; } -- cgit v1.2.3