aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStefan Boberg <[email protected]>2026-02-24 13:23:52 +0100
committerStefan Boberg <[email protected]>2026-02-24 13:27:53 +0100
commit3c89c486338890ce39ddebe5be4722a09e85701a (patch)
tree1382d8e81683072f7cb3a7505e6af3bda7cd0312
parentimplement yaml generation (#774) (diff)
downloadzen-3c89c486338890ce39ddebe5be4722a09e85701a.tar.xz
zen-3c89c486338890ce39ddebe5be4722a09e85701a.zip
Fix correctness and concurrency bugs found during code review
zenstore fixes: - cas.cpp: GetFileCasResults Results param passed by value instead of reference (large chunk results were silently lost) - structuredcachestore.cpp: MissCount unconditionally incremented (counted hits as misses) - cacherpc.cpp: Wrong boolean in Incomplete response array (all entries marked incomplete) - cachedisklayer.cpp: sizeof(sizeof(...)) in two validation checks computed sizeof(size_t) instead of struct size - buildstore.cpp: Wrong hash tracked in GC key list (BlobHash pushed twice instead of MetadataHash) - buildstore.cpp: Removed duplicate m_LastAccessTimeUpdateCount increment in PutBlob zenserver fixes: - httpbuildstore.cpp: Reversed subtraction in HTTP range calculation (unsigned underflow) - hubservice.cpp: Deadlock in Provision() calling Wake() while holding m_Lock (extracted WakeLocked helper) - zipfs.cpp: Data race in GetFile() lazy initialization (added RwLock with shared/exclusive paths) Co-Authored-By: Claude Opus 4.6 <[email protected]>
-rw-r--r--src/zenserver/frontend/zipfs.cpp20
-rw-r--r--src/zenserver/frontend/zipfs.h2
-rw-r--r--src/zenserver/hub/hubservice.cpp12
-rw-r--r--src/zenserver/storage/buildstore/httpbuildstore.cpp2
-rw-r--r--src/zenstore/buildstore/buildstore.cpp3
-rw-r--r--src/zenstore/cache/cachedisklayer.cpp4
-rw-r--r--src/zenstore/cache/cacherpc.cpp2
-rw-r--r--src/zenstore/cache/structuredcachestore.cpp5
-rw-r--r--src/zenstore/cas.cpp12
9 files changed, 42 insertions, 20 deletions
diff --git a/src/zenserver/frontend/zipfs.cpp b/src/zenserver/frontend/zipfs.cpp
index f9c2bc8ff..42df0520f 100644
--- a/src/zenserver/frontend/zipfs.cpp
+++ b/src/zenserver/frontend/zipfs.cpp
@@ -149,13 +149,25 @@ ZipFs::ZipFs(IoBuffer&& Buffer)
IoBuffer
ZipFs::GetFile(const std::string_view& FileName) const
{
- FileMap::iterator Iter = m_Files.find(FileName);
- if (Iter == m_Files.end())
{
- return {};
+ RwLock::SharedLockScope _(m_FilesLock);
+
+ FileMap::const_iterator Iter = m_Files.find(FileName);
+ if (Iter == m_Files.end())
+ {
+ return {};
+ }
+
+ const FileItem& Item = Iter->second;
+ if (Item.GetSize() > 0)
+ {
+ return IoBuffer(IoBuffer::Wrap, Item.GetData(), Item.GetSize());
+ }
}
- FileItem& Item = Iter->second;
+ RwLock::ExclusiveLockScope _(m_FilesLock);
+
+ FileItem& Item = m_Files.find(FileName)->second;
if (Item.GetSize() > 0)
{
return IoBuffer(IoBuffer::Wrap, Item.GetData(), Item.GetSize());
diff --git a/src/zenserver/frontend/zipfs.h b/src/zenserver/frontend/zipfs.h
index 1fa7da451..19f96567c 100644
--- a/src/zenserver/frontend/zipfs.h
+++ b/src/zenserver/frontend/zipfs.h
@@ -3,6 +3,7 @@
#pragma once
#include <zencore/iobuffer.h>
+#include <zencore/thread.h>
#include <unordered_map>
@@ -20,6 +21,7 @@ public:
private:
using FileItem = MemoryView;
using FileMap = std::unordered_map<std::string_view, FileItem>;
+ mutable RwLock m_FilesLock;
FileMap mutable m_Files;
IoBuffer m_Buffer;
};
diff --git a/src/zenserver/hub/hubservice.cpp b/src/zenserver/hub/hubservice.cpp
index 4d9da3a57..a00446a75 100644
--- a/src/zenserver/hub/hubservice.cpp
+++ b/src/zenserver/hub/hubservice.cpp
@@ -151,6 +151,7 @@ struct StorageServerInstance
inline uint16_t GetBasePort() const { return m_ServerInstance.GetBasePort(); }
private:
+ void WakeLocked();
RwLock m_Lock;
std::string m_ModuleId;
std::atomic<bool> m_IsProvisioned{false};
@@ -211,7 +212,7 @@ StorageServerInstance::Provision()
if (m_IsHibernated)
{
- Wake();
+ WakeLocked();
}
else
{
@@ -294,9 +295,14 @@ StorageServerInstance::Hibernate()
void
StorageServerInstance::Wake()
{
- // Start server in-place using existing data
-
RwLock::ExclusiveLockScope _(m_Lock);
+ WakeLocked();
+}
+
+void
+StorageServerInstance::WakeLocked()
+{
+ // Start server in-place using existing data
if (!m_IsHibernated)
{
diff --git a/src/zenserver/storage/buildstore/httpbuildstore.cpp b/src/zenserver/storage/buildstore/httpbuildstore.cpp
index f5ba30616..bf7afcc02 100644
--- a/src/zenserver/storage/buildstore/httpbuildstore.cpp
+++ b/src/zenserver/storage/buildstore/httpbuildstore.cpp
@@ -185,7 +185,7 @@ HttpBuildStoreService::GetBlobRequest(HttpRouterRequest& Req)
{
const HttpRange& Range = Ranges.front();
const uint64_t BlobSize = Blob.GetSize();
- const uint64_t MaxBlobSize = Range.Start < BlobSize ? Range.Start - BlobSize : 0;
+ const uint64_t MaxBlobSize = Range.Start < BlobSize ? BlobSize - Range.Start : 0;
const uint64_t RangeSize = Min(Range.End - Range.Start + 1, MaxBlobSize);
if (Range.Start + RangeSize > BlobSize)
{
diff --git a/src/zenstore/buildstore/buildstore.cpp b/src/zenstore/buildstore/buildstore.cpp
index 04a0781d3..aa37e75fe 100644
--- a/src/zenstore/buildstore/buildstore.cpp
+++ b/src/zenstore/buildstore/buildstore.cpp
@@ -266,13 +266,12 @@ BuildStore::PutBlob(const IoHash& BlobHash, const IoBuffer& Payload)
m_BlobLookup.insert({BlobHash, NewBlobIndex});
}
- m_LastAccessTimeUpdateCount++;
if (m_TrackedBlobKeys)
{
m_TrackedBlobKeys->push_back(BlobHash);
if (MetadataHash != IoHash::Zero)
{
- m_TrackedBlobKeys->push_back(BlobHash);
+ m_TrackedBlobKeys->push_back(MetadataHash);
}
}
}
diff --git a/src/zenstore/cache/cachedisklayer.cpp b/src/zenstore/cache/cachedisklayer.cpp
index ead7e4f3a..b73b3e6fb 100644
--- a/src/zenstore/cache/cachedisklayer.cpp
+++ b/src/zenstore/cache/cachedisklayer.cpp
@@ -626,7 +626,7 @@ BucketManifestSerializer::ReadSidecarFile(RwLock::ExclusiveLockScope& B
return false;
}
- const uint64_t ExpectedEntryCount = (FileSize - sizeof(sizeof(BucketMetaHeader))) / sizeof(ManifestData);
+ const uint64_t ExpectedEntryCount = (FileSize - sizeof(BucketMetaHeader)) / sizeof(ManifestData);
if (Header.EntryCount > ExpectedEntryCount)
{
ZEN_WARN(
@@ -1057,7 +1057,7 @@ ZenCacheDiskLayer::CacheBucket::ReadIndexFile(RwLock::ExclusiveLockScope&, const
return 0;
}
- const uint64_t ExpectedEntryCount = (FileSize - sizeof(sizeof(cache::impl::CacheBucketIndexHeader))) / sizeof(DiskIndexEntry);
+ const uint64_t ExpectedEntryCount = (FileSize - sizeof(cache::impl::CacheBucketIndexHeader)) / sizeof(DiskIndexEntry);
if (Header.EntryCount > ExpectedEntryCount)
{
return 0;
diff --git a/src/zenstore/cache/cacherpc.cpp b/src/zenstore/cache/cacherpc.cpp
index 94abcf547..e1fd0a3e6 100644
--- a/src/zenstore/cache/cacherpc.cpp
+++ b/src/zenstore/cache/cacherpc.cpp
@@ -966,7 +966,7 @@ CacheRpcHandler::HandleRpcGetCacheRecords(const CacheRequestContext& Context, Cb
}
else
{
- ResponseObject.AddBool(true);
+ ResponseObject.AddBool(false);
}
}
ResponseObject.EndArray();
diff --git a/src/zenstore/cache/structuredcachestore.cpp b/src/zenstore/cache/structuredcachestore.cpp
index 52b494e45..4e8475293 100644
--- a/src/zenstore/cache/structuredcachestore.cpp
+++ b/src/zenstore/cache/structuredcachestore.cpp
@@ -608,7 +608,10 @@ ZenCacheStore::GetBatch::Commit()
m_CacheStore.m_HitCount++;
OpScope.SetBytes(Result.Value.GetSize());
}
- m_CacheStore.m_MissCount++;
+ else
+ {
+ m_CacheStore.m_MissCount++;
+ }
}
}
}
diff --git a/src/zenstore/cas.cpp b/src/zenstore/cas.cpp
index ed017988f..7402d92d3 100644
--- a/src/zenstore/cas.cpp
+++ b/src/zenstore/cas.cpp
@@ -300,12 +300,12 @@ GetCompactCasResults(CasContainerStrategy& Strategy,
};
static void
-GetFileCasResults(FileCasStrategy& Strategy,
- CasStore::InsertMode Mode,
- std::span<IoBuffer> Data,
- std::span<IoHash> ChunkHashes,
- std::span<size_t> Indexes,
- std::vector<CasStore::InsertResult> Results)
+GetFileCasResults(FileCasStrategy& Strategy,
+ CasStore::InsertMode Mode,
+ std::span<IoBuffer> Data,
+ std::span<IoHash> ChunkHashes,
+ std::span<size_t> Indexes,
+ std::vector<CasStore::InsertResult>& Results)
{
for (size_t Index : Indexes)
{