From 5115b419cefd41e8d5cc465c8c7ae5140cde71d4 Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Fri, 6 Mar 2026 12:39:06 +0100 Subject: zenstore bug-fixes from static analysis pass (#815) **Bug fixes across zenstore, zenremotestore, and related subsystems, primarily surfaced by static analysis.** ## Cache subsystem (cachedisklayer.cpp) - Fixed tombstone scoping bug: tombstone flag and missing entry were recorded outside the block where data was removed, causing non-missing entries to be incorrectly tombstoned - Fixed use-after-overwrite: `RemoveMemCachedData`/`RemoveMetaData` were called after `Payload` was overwritten on cache put, leaking stale data - Fixed incorrect retry sleep formula (`100 - (3 - RetriesLeft) * 100` always produced the same or negative value; corrected to `(3 - RetriesLeft) * 100`) - Fixed broken `break` missing from sidecar file read loop, causing reads past valid data - Fixed missing format argument in three `ZEN_WARN`/`ZEN_ERROR` log calls (format string had `{}` placeholders with no corresponding argument, or vice versa) - Fixed elapsed timer being accumulated inside the wrong scope in `HandleRpcGetCacheRecords` - Fixed test asserting against unserialized `RecordPolicy` instead of the deserialized `Loaded` copy - Initialized `AbortFlag`/`PauseFlag` atomics at declaration (UB if read before first write) ## Build store (buildstore.cpp / buildstore.h) - Fixed wrong variable used in warning log: used loop index `ResultIndex` instead of `Index`/`MetaLocationResultIndexes[Index]`, logging wrong hash values - Fixed `sizeof(AccessTimesHeader)` used instead of `sizeof(AccessTimeRecord)` when advancing write offset, corrupting the access times file if the sizes differ - Initialized `m_LastAccessTimeUpdateCount` atomic member (was uninitialized) - Changed map iteration loops to use `const auto&` to avoid unnecessary copies ## Project store (projectstore.cpp / projectstore.h) - Fixed wrong iterator dereferenced in `IterateChunks`: used `ChunkIt->second` (from a different map lookup) instead of `MetaIt->second` - Fixed wrong assert variable: `Sizes[Index]` should be `RawSizes[Index]` - Fixed `MakeTombstone`/`IsTombstone` inconsistency: `MakeTombstone` was zeroing `OpLsn` but `IsTombstone` checks `OpLsn.Number != 0`; tombstone creation now preserves `OpLsn` - Fixed uninitialized `InvalidEntries` counter - Fixed format string mismatch in warning log - Initialized `AbortFlag`/`PauseFlag` atomics; changed map iteration to `const auto&` ## Workspaces (workspaces.cpp) - Fixed missing alias registration when a workspace share is updated: alias was deleted but never re-inserted - Fixed integer overflow in range clamping: `(RequestedOffset + RequestedSize) > Size` could wrap; corrected to `RequestedSize > Size - RequestedOffset` - Changed map iteration loops to `const auto&` ## CAS subsystem (cas.cpp, caslog.cpp, compactcas.cpp, filecas.cpp) - Fixed `IterateChunks` passing original `Payload` buffer instead of the modified `Chunk` buffer (content type was set on the copy but the original was sent to the callback) - Fixed invalid `std::future::get()` call on default-constructed futures - Fixed sign-comparison in `CasLogFile::Replay` loop (`int i` vs `size_t`) - Changed `CasLogFile::IsValid` and `Open` to take `const std::filesystem::path&` instead of by value - Fixed format string in `~CasContainerStrategy` error log ## Remote store (zenremotestore) - Fixed `FolderContent::operator==` always returning true: loop variable `PathCount` was initialized to 0 instead of `Paths.size()` - Fixed `GetChunkIndexForRawHash` looking up from wrong map (`RawHashToSequenceIndex` instead of `ChunkHashToChunkIndex`) - Fixed double-counted `UniqueSequencesFound` stat (incremented in both branches of an if/else) - Fixed `RawSize` sentinel value truncation: `(uint32_t)-1` assigned to a `uint64_t` field; corrected to `(uint64_t)-1` - Initialized uninitialized atomic and struct members across `buildstorageoperations.h`, `chunkblock.h`, and `remoteprojectstore.h` --- src/zenstore/projectstore.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) (limited to 'src/zenstore/projectstore.cpp') diff --git a/src/zenstore/projectstore.cpp b/src/zenstore/projectstore.cpp index 217336eec..3f705d12c 100644 --- a/src/zenstore/projectstore.cpp +++ b/src/zenstore/projectstore.cpp @@ -1488,7 +1488,7 @@ ProjectStore::Oplog::Read() else { std::vector OpLogEntries; - uint64_t InvalidEntries; + uint64_t InvalidEntries = 0; m_Storage->ReadOplogEntriesFromLog(OpLogEntries, InvalidEntries, m_LogFlushPosition); for (const OplogEntry& OpEntry : OpLogEntries) { @@ -1750,8 +1750,8 @@ ProjectStore::Oplog::Validate(const std::filesystem::path& ProjectRootDir, } }; - std::atomic AbortFlag; - std::atomic PauseFlag; + std::atomic AbortFlag{false}; + std::atomic PauseFlag{false}; ParallelWork Work(AbortFlag, PauseFlag, WorkerThreadPool::EMode::DisableBacklog); try { @@ -2373,7 +2373,7 @@ ProjectStore::Oplog::IterateChunks(const std::filesystem::path& P else if (auto MetaIt = m_MetaMap.find(ChunkId); MetaIt != m_MetaMap.end()) { CidChunkIndexes.push_back(ChunkIndex); - CidChunkHashes.push_back(ChunkIt->second); + CidChunkHashes.push_back(MetaIt->second); } else if (auto FileIt = m_FileMap.find(ChunkId); FileIt != m_FileMap.end()) { @@ -2384,8 +2384,8 @@ ProjectStore::Oplog::IterateChunks(const std::filesystem::path& P } if (OptionalWorkerPool) { - std::atomic AbortFlag; - std::atomic PauseFlag; + std::atomic AbortFlag{false}; + std::atomic PauseFlag{false}; ParallelWork Work(AbortFlag, PauseFlag, WorkerThreadPool::EMode::DisableBacklog); try { @@ -3817,7 +3817,7 @@ ProjectStore::Project::OpenOplog(std::string_view OplogId, bool AllowCompact, bo std::filesystem::path DeletePath; if (!RemoveOplog(OplogId, DeletePath)) { - ZEN_WARN("Failed to clean up deleted oplog {}/{}", Identifier, OplogId, OplogBasePath); + ZEN_WARN("Failed to clean up deleted oplog {}/{} at '{}'", Identifier, OplogId, OplogBasePath); } ReOpen = true; @@ -4053,8 +4053,8 @@ ProjectStore::Project::Scrub(ScrubContext& Ctx) RwLock::SharedLockScope _(m_ProjectLock); - std::atomic Abort; - std::atomic Pause; + std::atomic Abort{false}; + std::atomic Pause{false}; ParallelWork Work(Abort, Pause, WorkerThreadPool::EMode::DisableBacklog); try @@ -4433,8 +4433,8 @@ ProjectStore::Flush() } WorkerThreadPool& WorkerPool = GetSmallWorkerPool(EWorkloadType::Burst); - std::atomic AbortFlag; - std::atomic PauseFlag; + std::atomic AbortFlag{false}; + std::atomic PauseFlag{false}; ParallelWork Work(AbortFlag, PauseFlag, WorkerThreadPool::EMode::DisableBacklog); try { @@ -4974,7 +4974,7 @@ ProjectStore::GetProjectChunkInfos(LoggerRef InLog, Project& Project, Oplog& Opl } if (WantsRawSizeField) { - ZEN_ASSERT_SLOW(Sizes[Index] == (uint64_t)-1); + ZEN_ASSERT_SLOW(RawSizes[Index] == (uint64_t)-1); RawSizes[Index] = Payload.GetSize(); } } @@ -5762,7 +5762,7 @@ public: } } - for (auto ProjectIt : m_ProjectStore.m_Projects) + for (const auto& ProjectIt : m_ProjectStore.m_Projects) { Ref Project = ProjectIt.second; std::vector OplogsToCompact = Project->GetOplogsToCompact(); -- cgit v1.2.3