aboutsummaryrefslogtreecommitdiff
path: root/src
diff options
context:
space:
mode:
authorStefan Boberg <[email protected]>2026-02-24 14:56:57 +0100
committerGitHub Enterprise <[email protected]>2026-02-24 14:56:57 +0100
commit5c5e12d1f02bb7cc1f42750e47a2735dc933c194 (patch)
tree5dd917292848be3f123a98058ed1917af9df50a9 /src
parentRevert "Fix correctness and concurrency bugs found during code review" (diff)
downloadzen-5c5e12d1f02bb7cc1f42750e47a2735dc933c194.tar.xz
zen-5c5e12d1f02bb7cc1f42750e47a2735dc933c194.zip
Various bug fixes (#778)
zencore fixes: - filesystem.cpp: ReadFile error reporting logic - compactbinaryvalue.h: CbValue::As*String error reporting logic zenhttp fixes: - httpasio BindAcceptor would `return 0;` in a function returning `std::string` (UB) - httpsys async workpool initialization race 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)
Diffstat (limited to 'src')
-rw-r--r--src/zencompute-test/xmake.lua1
-rw-r--r--src/zencompute/xmake.lua2
-rw-r--r--src/zencore/filesystem.cpp16
-rw-r--r--src/zencore/include/zencore/compactbinaryvalue.h24
-rw-r--r--src/zencore/memtrack/callstacktrace.cpp8
-rw-r--r--src/zencore/string.cpp4
-rw-r--r--src/zenhttp/servers/httpasio.cpp4
-rw-r--r--src/zenhttp/servers/httpsys.cpp17
-rw-r--r--src/zenserver/frontend/frontend.cpp9
-rw-r--r--src/zenserver/frontend/frontend.h7
-rw-r--r--src/zenserver/frontend/zipfs.cpp20
-rw-r--r--src/zenserver/frontend/zipfs.h8
-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
-rw-r--r--src/zentest-appstub/xmake.lua2
20 files changed, 93 insertions, 69 deletions
diff --git a/src/zencompute-test/xmake.lua b/src/zencompute-test/xmake.lua
index 64a3c7703..1207bdefd 100644
--- a/src/zencompute-test/xmake.lua
+++ b/src/zencompute-test/xmake.lua
@@ -6,4 +6,3 @@ target("zencompute-test")
add_headerfiles("**.h")
add_files("*.cpp")
add_deps("zencompute", "zencore")
- add_packages("vcpkg::doctest")
diff --git a/src/zencompute/xmake.lua b/src/zencompute/xmake.lua
index c710b662d..50877508c 100644
--- a/src/zencompute/xmake.lua
+++ b/src/zencompute/xmake.lua
@@ -7,5 +7,3 @@ target('zencompute')
add_files("**.cpp")
add_includedirs("include", {public=true})
add_deps("zencore", "zenstore", "zenutil", "zennet", "zenhttp")
- add_packages("vcpkg::gsl-lite")
- add_packages("vcpkg::spdlog", "vcpkg::cxxopts")
diff --git a/src/zencore/filesystem.cpp b/src/zencore/filesystem.cpp
index 1a4ee4b9b..553897407 100644
--- a/src/zencore/filesystem.cpp
+++ b/src/zencore/filesystem.cpp
@@ -1326,11 +1326,6 @@ ReadFile(void* NativeHandle, void* Data, uint64_t Size, uint64_t FileOffset, uin
{
BytesRead = size_t(dwNumberOfBytesRead);
}
- else if ((BytesRead != NumberOfBytesToRead))
- {
- Ec = MakeErrorCode(ERROR_HANDLE_EOF);
- return;
- }
else
{
Ec = MakeErrorCodeFromLastError();
@@ -1344,20 +1339,15 @@ ReadFile(void* NativeHandle, void* Data, uint64_t Size, uint64_t FileOffset, uin
{
BytesRead = size_t(ReadResult);
}
- else if ((BytesRead != NumberOfBytesToRead))
- {
- Ec = MakeErrorCode(EIO);
- return;
- }
else
{
Ec = MakeErrorCodeFromLastError();
return;
}
#endif
- Size -= NumberOfBytesToRead;
- FileOffset += NumberOfBytesToRead;
- Data = reinterpret_cast<uint8_t*>(Data) + NumberOfBytesToRead;
+ Size -= BytesRead;
+ FileOffset += BytesRead;
+ Data = reinterpret_cast<uint8_t*>(Data) + BytesRead;
}
}
diff --git a/src/zencore/include/zencore/compactbinaryvalue.h b/src/zencore/include/zencore/compactbinaryvalue.h
index aa2d2821d..4ce8009b8 100644
--- a/src/zencore/include/zencore/compactbinaryvalue.h
+++ b/src/zencore/include/zencore/compactbinaryvalue.h
@@ -128,17 +128,21 @@ CbValue::AsString(CbFieldError* OutError, std::string_view Default) const
uint32_t ValueSizeByteCount;
const uint64_t ValueSize = ReadVarUInt(Chars, ValueSizeByteCount);
- if (OutError)
+ if (ValueSize >= (uint64_t(1) << 31))
{
- if (ValueSize >= (uint64_t(1) << 31))
+ if (OutError)
{
*OutError = CbFieldError::RangeError;
- return Default;
}
+ return Default;
+ }
+
+ if (OutError)
+ {
*OutError = CbFieldError::None;
}
- return std::string_view(Chars + ValueSizeByteCount, int32_t(ValueSize));
+ return std::string_view(Chars + ValueSizeByteCount, size_t(ValueSize));
}
inline std::u8string_view
@@ -148,17 +152,21 @@ CbValue::AsU8String(CbFieldError* OutError, std::u8string_view Default) const
uint32_t ValueSizeByteCount;
const uint64_t ValueSize = ReadVarUInt(Chars, ValueSizeByteCount);
- if (OutError)
+ if (ValueSize >= (uint64_t(1) << 31))
{
- if (ValueSize >= (uint64_t(1) << 31))
+ if (OutError)
{
*OutError = CbFieldError::RangeError;
- return Default;
}
+ return Default;
+ }
+
+ if (OutError)
+ {
*OutError = CbFieldError::None;
}
- return std::u8string_view(Chars + ValueSizeByteCount, int32_t(ValueSize));
+ return std::u8string_view(Chars + ValueSizeByteCount, size_t(ValueSize));
}
inline uint64_t
diff --git a/src/zencore/memtrack/callstacktrace.cpp b/src/zencore/memtrack/callstacktrace.cpp
index a5b7fede6..4a7068568 100644
--- a/src/zencore/memtrack/callstacktrace.cpp
+++ b/src/zencore/memtrack/callstacktrace.cpp
@@ -169,13 +169,13 @@ private:
std::atomic_uint64_t Key;
std::atomic_uint32_t Value;
- inline uint64 GetKey() const { return Key.load(std::memory_order_relaxed); }
+ inline uint64 GetKey() const { return Key.load(std::memory_order_acquire); }
inline uint32_t GetValue() const { return Value.load(std::memory_order_relaxed); }
- inline bool IsEmpty() const { return Key.load(std::memory_order_relaxed) == 0; }
+ inline bool IsEmpty() const { return Key.load(std::memory_order_acquire) == 0; }
inline void SetKeyValue(uint64_t InKey, uint32_t InValue)
{
- Value.store(InValue, std::memory_order_release);
- Key.store(InKey, std::memory_order_relaxed);
+ Value.store(InValue, std::memory_order_relaxed);
+ Key.store(InKey, std::memory_order_release);
}
static inline uint32_t KeyHash(uint64_t Key) { return static_cast<uint32_t>(Key); }
static inline void ClearEntries(FEncounteredCallstackSetEntry* Entries, int32_t EntryCount)
diff --git a/src/zencore/string.cpp b/src/zencore/string.cpp
index 0ee863b74..a9aed6309 100644
--- a/src/zencore/string.cpp
+++ b/src/zencore/string.cpp
@@ -24,6 +24,10 @@ utf16to8_impl(u16bit_iterator StartIt, u16bit_iterator EndIt, ::zen::StringBuild
// Take care of surrogate pairs first
if (utf8::internal::is_lead_surrogate(cp))
{
+ if (StartIt == EndIt)
+ {
+ break;
+ }
uint32_t trail_surrogate = utf8::internal::mask16(*StartIt++);
cp = (cp << 10) + trail_surrogate + utf8::internal::SURROGATE_OFFSET;
}
diff --git a/src/zenhttp/servers/httpasio.cpp b/src/zenhttp/servers/httpasio.cpp
index 1c0ebef90..fbc7fe401 100644
--- a/src/zenhttp/servers/httpasio.cpp
+++ b/src/zenhttp/servers/httpasio.cpp
@@ -89,10 +89,10 @@ IsIPv6AvailableSysctl(void)
char buf[16];
if (fgets(buf, sizeof(buf), f))
{
- fclose(f);
// 0 means IPv6 enabled, 1 means disabled
val = atoi(buf);
}
+ fclose(f);
}
return val == 0;
@@ -1544,7 +1544,7 @@ struct HttpAcceptor
{
ZEN_WARN("Unable to initialize asio service, (bind returned '{}')", BindErrorCode.message());
- return 0;
+ return {};
}
if (EffectivePort != BasePort)
diff --git a/src/zenhttp/servers/httpsys.cpp b/src/zenhttp/servers/httpsys.cpp
index c640ba90b..6995ffca9 100644
--- a/src/zenhttp/servers/httpsys.cpp
+++ b/src/zenhttp/servers/httpsys.cpp
@@ -25,6 +25,7 @@
# include <zencore/workthreadpool.h>
# include "iothreadpool.h"
+# include <atomic>
# include <http.h>
namespace zen {
@@ -129,8 +130,8 @@ private:
std::unique_ptr<WinIoThreadPool> m_IoThreadPool;
- RwLock m_AsyncWorkPoolInitLock;
- WorkerThreadPool* m_AsyncWorkPool = nullptr;
+ RwLock m_AsyncWorkPoolInitLock;
+ std::atomic<WorkerThreadPool*> m_AsyncWorkPool = nullptr;
std::vector<std::wstring> m_BaseUris; // eg: http://*:nnnn/
HTTP_SERVER_SESSION_ID m_HttpSessionId = 0;
@@ -1032,8 +1033,10 @@ HttpSysServer::~HttpSysServer()
ZEN_ERROR("~HttpSysServer() called without calling Close() first");
}
- delete m_AsyncWorkPool;
+ auto WorkPool = m_AsyncWorkPool.load(std::memory_order_relaxed);
m_AsyncWorkPool = nullptr;
+
+ delete WorkPool;
}
void
@@ -1323,17 +1326,17 @@ HttpSysServer::WorkPool()
{
ZEN_MEMSCOPE(GetHttpsysTag());
- if (!m_AsyncWorkPool)
+ if (!m_AsyncWorkPool.load(std::memory_order_acquire))
{
RwLock::ExclusiveLockScope _(m_AsyncWorkPoolInitLock);
- if (!m_AsyncWorkPool)
+ if (!m_AsyncWorkPool.load(std::memory_order_relaxed))
{
- m_AsyncWorkPool = new WorkerThreadPool(m_InitialConfig.AsyncWorkThreadCount, "http_async");
+ m_AsyncWorkPool.store(new WorkerThreadPool(m_InitialConfig.AsyncWorkThreadCount, "http_async"), std::memory_order_release);
}
}
- return *m_AsyncWorkPool;
+ return *m_AsyncWorkPool.load(std::memory_order_relaxed);
}
void
diff --git a/src/zenserver/frontend/frontend.cpp b/src/zenserver/frontend/frontend.cpp
index 2b157581f..1cf451e91 100644
--- a/src/zenserver/frontend/frontend.cpp
+++ b/src/zenserver/frontend/frontend.cpp
@@ -38,7 +38,7 @@ HttpFrontendService::HttpFrontendService(std::filesystem::path Directory, HttpSt
#if ZEN_EMBED_HTML_ZIP
// Load an embedded Zip archive
IoBuffer HtmlZipDataBuffer(IoBuffer::Wrap, gHtmlZipData, sizeof(gHtmlZipData) - 1);
- m_ZipFs = ZipFs(std::move(HtmlZipDataBuffer));
+ m_ZipFs = std::make_unique<ZipFs>(std::move(HtmlZipDataBuffer));
#endif
if (m_Directory.empty() && !m_ZipFs)
@@ -157,9 +157,12 @@ HttpFrontendService::HandleRequest(zen::HttpServerRequest& Request)
}
}
- if (IoBuffer FileBuffer = m_ZipFs.GetFile(Uri))
+ if (m_ZipFs)
{
- return Request.WriteResponse(HttpResponseCode::OK, ContentType, FileBuffer);
+ if (IoBuffer FileBuffer = m_ZipFs->GetFile(Uri))
+ {
+ return Request.WriteResponse(HttpResponseCode::OK, ContentType, FileBuffer);
+ }
}
Request.WriteResponse(HttpResponseCode::NotFound, HttpContentType::kText, "Not found"sv);
diff --git a/src/zenserver/frontend/frontend.h b/src/zenserver/frontend/frontend.h
index 84ffaac42..6d8585b72 100644
--- a/src/zenserver/frontend/frontend.h
+++ b/src/zenserver/frontend/frontend.h
@@ -7,6 +7,7 @@
#include "zipfs.h"
#include <filesystem>
+#include <memory>
namespace zen {
@@ -20,9 +21,9 @@ public:
virtual void HandleStatusRequest(HttpServerRequest& Request) override;
private:
- ZipFs m_ZipFs;
- std::filesystem::path m_Directory;
- HttpStatusService& m_StatusService;
+ std::unique_ptr<ZipFs> m_ZipFs;
+ std::filesystem::path m_Directory;
+ HttpStatusService& m_StatusService;
};
} // namespace zen
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..645121693 100644
--- a/src/zenserver/frontend/zipfs.h
+++ b/src/zenserver/frontend/zipfs.h
@@ -3,23 +3,23 @@
#pragma once
#include <zencore/iobuffer.h>
+#include <zencore/thread.h>
#include <unordered_map>
namespace zen {
-//////////////////////////////////////////////////////////////////////////
class ZipFs
{
public:
- ZipFs() = default;
- ZipFs(IoBuffer&& Buffer);
+ explicit ZipFs(IoBuffer&& Buffer);
+
IoBuffer GetFile(const std::string_view& FileName) const;
- inline operator bool() const { return !m_Files.empty(); }
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)
{
diff --git a/src/zentest-appstub/xmake.lua b/src/zentest-appstub/xmake.lua
index db3ff2e2d..844ba82ef 100644
--- a/src/zentest-appstub/xmake.lua
+++ b/src/zentest-appstub/xmake.lua
@@ -6,8 +6,6 @@ target("zentest-appstub")
add_headerfiles("**.h")
add_files("*.cpp")
add_deps("zencore")
- add_packages("vcpkg::gsl-lite") -- this should ideally be propagated by the zencore dependency
- add_packages("vcpkg::mimalloc")
if is_os("linux") then
add_syslinks("pthread")