fix zero size attachment replies (#69)

- Bugfix: Don't try to respond with zero size partial cache value when partial size is zero
- Improvement: Added more validation of data read from cache / cas

8 files changed, 84 insertions, 67 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index deb95a381..a1f2d44ad 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,9 +1,11 @@
 ##
 - Bugfix: Remove extra loop causing GetProjectFiles for project store to find all chunks once for each chunk found
 - Bugfix: Don't capture ChunkIndex variable in CasImpl::IterateChunks by reference as it causes crash
+- Bugfix: Don't try to respond with zero size partial cache value when partial size is zero
 - Improvement: Make FileCasStrategy::IterateChunks (optionally) multithreaded (improves GetProjectFiles performance)
 - Improvement: Add batch scope for adding multiple cache values from single request efficently
 - Improvement: Use temp file write and move into place for manifest/state files to avoid partial incomplete file writes
+- Improvement: Added more validation of data read from cache / cas
 
 ## 5.5.0
 - Change: GCv2 is now the default option, use `--gc-v2=false` to fall back to GCv1
diff --git a/src/zencore/compactbinarypackage.cpp b/src/zencore/compactbinarypackage.cpp
index a4fa38a1d..4d4e0951e 100644
--- a/src/zencore/compactbinarypackage.cpp
+++ b/src/zencore/compactbinarypackage.cpp
@@ -11,32 +11,45 @@ namespace zen {
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
-CbAttachment::CbAttachment(const CompressedBuffer& InValue, const IoHash& Hash) : CbAttachment(InValue.MakeOwned(), Hash)
+CbAttachment::CbAttachment(const CbObject& InValue, const IoHash* const InHash)
 {
-}
+	auto SetValue = [&](const CbObject& ValueToSet) {
+		if (InHash)
+		{
+			Value.emplace<CbObject>(ValueToSet);
+			Hash = *InHash;
+		}
+		else
+		{
+			Value.emplace<CbObject>(ValueToSet);
+			Hash = ValueToSet.GetHash();
+		}
+	};
 
-CbAttachment::CbAttachment(const SharedBuffer& InValue) : CbAttachment(CompositeBuffer(InValue))
-{
+	MemoryView View;
+	if (!InValue.IsOwned() || !InValue.TryGetSerializedView(View))
+	{
+		SetValue(CbObject::Clone(InValue));
+	}
+	else
+	{
+		SetValue(InValue);
+	}
 }
 
-CbAttachment::CbAttachment(const SharedBuffer& InValue, const IoHash& InHash) : CbAttachment(CompositeBuffer(InValue), InHash)
+CbAttachment::CbAttachment(const CompressedBuffer& InValue, const IoHash& Hash) : Hash(Hash), Value(InValue)
 {
+	ZEN_ASSERT(!std::get<CompressedBuffer>(Value).IsNull());
 }
 
-CbAttachment::CbAttachment(const CompositeBuffer& InValue)
-: Hash(InValue.IsNull() ? IoHash::Zero : IoHash::HashBuffer(InValue))
-, Value(InValue)
+CbAttachment::CbAttachment(CompressedBuffer&& InValue, const IoHash& InHash) : Hash(InHash), Value(std::move(InValue))
 {
-	if (std::get<CompositeBuffer>(Value).IsNull())
-	{
-		Value.emplace<std::nullptr_t>();
-	}
+	ZEN_ASSERT(!std::get<CompressedBuffer>(Value).IsNull());
 }
 
 CbAttachment::CbAttachment(CompositeBuffer&& InValue)
 : Hash(InValue.IsNull() ? IoHash::Zero : IoHash::HashBuffer(InValue))
 , Value(std::move(InValue))
-
 {
 	if (std::get<CompositeBuffer>(Value).IsNull())
 	{
@@ -44,7 +57,7 @@ CbAttachment::CbAttachment(CompositeBuffer&& InValue)
 	}
 }
 
-CbAttachment::CbAttachment(CompositeBuffer&& InValue, const IoHash& InHash) : Hash(InHash), Value(InValue)
+CbAttachment::CbAttachment(CompositeBuffer&& InValue, const IoHash& InHash) : Hash(InHash), Value(std::move(InValue))
 {
 	if (std::get<CompositeBuffer>(Value).IsNull())
 	{
@@ -52,40 +65,6 @@ CbAttachment::CbAttachment(CompositeBuffer&& InValue, const IoHash& InHash) : Ha
 	}
 }
 
-CbAttachment::CbAttachment(CompressedBuffer&& InValue, const IoHash& InHash) : Hash(InHash), Value(InValue)
-{
-	if (std::get<CompressedBuffer>(Value).IsNull())
-	{
-		Value.emplace<std::nullptr_t>();
-	}
-}
-
-CbAttachment::CbAttachment(const CbObject& InValue, const IoHash* const InHash)
-{
-	auto SetValue = [&](const CbObject& ValueToSet) {
-		if (InHash)
-		{
-			Value.emplace<CbObject>(ValueToSet);
-			Hash = *InHash;
-		}
-		else
-		{
-			Value.emplace<CbObject>(ValueToSet);
-			Hash = ValueToSet.GetHash();
-		}
-	};
-
-	MemoryView View;
-	if (!InValue.IsOwned() || !InValue.TryGetSerializedView(View))
-	{
-		SetValue(CbObject::Clone(InValue));
-	}
-	else
-	{
-		SetValue(InValue);
-	}
-}
-
 bool
 CbAttachment::TryLoad(IoBuffer& InBuffer, BufferAllocator Allocator)
 {
@@ -186,7 +165,7 @@ TryLoad_ArchiveFieldIntoAttachment(CbAttachment& TargetAttachment, CbField&& Fie
 		{
 			return false;
 		}
-		TargetAttachment = CbAttachment(CompositeBuffer(Buffer), BinaryAttachmentHash);
+		TargetAttachment = CbAttachment(std::move(Buffer), BinaryAttachmentHash);
 	}
 	else if (SharedBuffer Buffer = Field.AsBinary(); !Field.HasError())
 	{
@@ -201,7 +180,7 @@ TryLoad_ArchiveFieldIntoAttachment(CbAttachment& TargetAttachment, CbField&& Fie
 		else
 		{
 			// Is an uncompressed empty binary blob
-			TargetAttachment = CbAttachment(CompositeBuffer(Buffer), IoHash::HashBuffer(nullptr, 0));
+			TargetAttachment = CbAttachment(std::move(Buffer), IoHash::HashBuffer(nullptr, 0));
 		}
 	}
 	else
@@ -386,6 +365,10 @@ CbPackage::AddAttachments(std::span<const CbAttachment> InAttachments)
 	{
 		return;
 	}
+	for (const CbAttachment& Attachment : InAttachments)
+	{
+		ZEN_ASSERT(!Attachment.IsNull());
+	}
 	// Assume we have no duplicates!
 	Attachments.insert(Attachments.end(), InAttachments.begin(), InAttachments.end());
 	std::sort(Attachments.begin(), Attachments.end());
@@ -741,7 +724,7 @@ namespace legacy {
 						}
 						else
 						{
-							Package.AddAttachment(CbAttachment(CompositeBuffer(std::move(Buffer)), Hash));
+							Package.AddAttachment(CbAttachment(std::move(Buffer), Hash));
 						}
 					}
 				}
diff --git a/src/zencore/include/zencore/compactbinarypackage.h b/src/zencore/include/zencore/compactbinarypackage.h
index 16f723edc..4a6bca67a 100644
--- a/src/zencore/include/zencore/compactbinarypackage.h
+++ b/src/zencore/include/zencore/compactbinarypackage.h
@@ -46,26 +46,23 @@ public:
 	inline explicit CbAttachment(const CbObject& InValue) : CbAttachment(InValue, nullptr) {}
 
 	/** Construct a compact binary attachment. Value is cloned if not owned. Hash must match Value. */
-	inline explicit CbAttachment(const CbObject& InValue, const IoHash& Hash) : CbAttachment(InValue, &Hash) {}
+	inline CbAttachment(const CbObject& InValue, const IoHash& Hash) : CbAttachment(InValue, &Hash) {}
 
 	/** Construct a raw binary attachment. Value is cloned if not owned. */
-	ZENCORE_API explicit CbAttachment(const SharedBuffer& InValue);
+	ZENCORE_API explicit CbAttachment(const SharedBuffer& InValue) : CbAttachment(CompositeBuffer(InValue)) {}
 
 	/** Construct a raw binary attachment. Value is cloned if not owned. Hash must match Value. */
-	ZENCORE_API explicit CbAttachment(const SharedBuffer& InValue, const IoHash& Hash);
+	ZENCORE_API CbAttachment(const SharedBuffer& InValue, const IoHash& Hash) : CbAttachment(CompositeBuffer(InValue), Hash) {}
 
 	/** Construct a raw binary attachment. Value is cloned if not owned. */
-	ZENCORE_API explicit CbAttachment(const CompositeBuffer& InValue);
+	ZENCORE_API explicit CbAttachment(SharedBuffer&& InValue) : CbAttachment(CompositeBuffer(std::move(InValue))) {}
 
-	/** Construct a raw binary attachment. Value is cloned if not owned. */
-	ZENCORE_API explicit CbAttachment(CompositeBuffer&& InValue);
-
-	/** Construct a raw binary attachment. Value is cloned if not owned. */
-	ZENCORE_API explicit CbAttachment(CompositeBuffer&& InValue, const IoHash& Hash);
+	/** Construct a raw binary attachment. Value is cloned if not owned. Hash must match Value. */
+	ZENCORE_API CbAttachment(SharedBuffer&& InValue, const IoHash& Hash) : CbAttachment(CompositeBuffer(std::move(InValue)), Hash) {}
 
 	/** Construct a compressed binary attachment. Value is cloned if not owned. */
-	ZENCORE_API explicit CbAttachment(const CompressedBuffer& InValue, const IoHash& Hash);
-	ZENCORE_API explicit CbAttachment(CompressedBuffer&& InValue, const IoHash& Hash);
+	ZENCORE_API CbAttachment(const CompressedBuffer& InValue, const IoHash& Hash);
+	ZENCORE_API CbAttachment(CompressedBuffer&& InValue, const IoHash& Hash);
 
 	/** Reset this to a null attachment. */
 	inline void Reset() { *this = CbAttachment(); }
@@ -132,6 +129,8 @@ public:
 
 private:
 	ZENCORE_API CbAttachment(const CbObject& Value, const IoHash* Hash);
+	ZENCORE_API explicit CbAttachment(CompositeBuffer&& InValue);
+	ZENCORE_API CbAttachment(CompositeBuffer&& InValue, const IoHash& Hash);
 
 	IoHash																	  Hash;
 	std::variant<std::nullptr_t, CbObject, CompositeBuffer, CompressedBuffer> Value;
diff --git a/src/zenserver/cache/httpstructuredcache.cpp b/src/zenserver/cache/httpstructuredcache.cpp
index 135eee57c..449a43653 100644
--- a/src/zenserver/cache/httpstructuredcache.cpp
+++ b/src/zenserver/cache/httpstructuredcache.cpp
@@ -727,7 +727,15 @@ HttpStructuredCacheService::HandleGetCacheRecord(HttpServerRequest& Request, con
 						if (IoBuffer Chunk = m_CidStore.FindChunkByCid(AttachmentHash.AsHash()))
 						{
 							CompressedBuffer Compressed = CompressedBuffer::FromCompressedNoValidate(std::move(Chunk));
-							Package.AddAttachment(CbAttachment(Compressed, AttachmentHash.AsHash()));
+							if (Compressed)
+							{
+								Package.AddAttachment(CbAttachment(Compressed, AttachmentHash.AsHash()));
+							}
+							else
+							{
+								ZEN_WARN("invalid compressed binary returned for {}", AttachmentHash.AsHash());
+								MissingCount++;
+							}
 						}
 						else
 						{
diff --git a/src/zenserver/projectstore/projectstore.cpp b/src/zenserver/projectstore/projectstore.cpp
index f5fecce24..45a96c60d 100644
--- a/src/zenserver/projectstore/projectstore.cpp
+++ b/src/zenserver/projectstore/projectstore.cpp
@@ -3431,9 +3431,16 @@ ProjectStore::Rpc(HttpServerRequest&	 HttpReq,
 				IoBuffer ChunkBuffer = m_CidStore.FindChunkByCid(RawHash);
 				if (ChunkBuffer)
 				{
-					ResponseWriter.AddHash(RawHash);
-					ResponsePackage.AddAttachment(
-						CbAttachment(CompressedBuffer::FromCompressedNoValidate(std::move(ChunkBuffer)), RawHash));
+					CompressedBuffer Compressed = CompressedBuffer::FromCompressedNoValidate(std::move(ChunkBuffer));
+					if (Compressed)
+					{
+						ResponseWriter.AddHash(RawHash);
+						ResponsePackage.AddAttachment(CbAttachment(std::move(Compressed), RawHash));
+					}
+					else
+					{
+						ZEN_WARN("invalid compressed binary in cas store for {}", RawHash);
+					}
 				}
 			}
 			ResponseWriter.EndArray();
diff --git a/src/zenserver/projectstore/zenremoteprojectstore.cpp b/src/zenserver/projectstore/zenremoteprojectstore.cpp
index cec68111f..679c344c6 100644
--- a/src/zenserver/projectstore/zenremoteprojectstore.cpp
+++ b/src/zenserver/projectstore/zenremoteprojectstore.cpp
@@ -119,6 +119,7 @@ public:
 					IoHash			 RawHash;
 					uint64_t		 RawSize;
 					CompressedBuffer Compressed = CompressedBuffer::FromCompressed(Chunk, RawHash, RawSize);
+					ZEN_ASSERT(Compressed);
 					RequestWriter.AddHash(RawHash);
 					RequestPackage.AddAttachment(CbAttachment(Compressed, RawHash));
 				}
diff --git a/src/zenstore/cache/cacherpc.cpp b/src/zenstore/cache/cacherpc.cpp
index 6871dfd56..5bd73e902 100644
--- a/src/zenstore/cache/cacherpc.cpp
+++ b/src/zenstore/cache/cacherpc.cpp
@@ -1632,6 +1632,10 @@ CacheRpcHandler::WriteGetCacheChunksResponse([[maybe_unused]] const CacheRequest
 				if (Request.Value && !EnumHasAllFlags(Request.DownstreamPolicy, CachePolicy::SkipData))
 				{
 					auto IsPartialRangeRequest = [](const ChunkRequest& Request) {
+						if (Request.RequestedOffset == 0 && Request.RequestedSize == 0)
+						{
+							return false;
+						}
 						if ((Request.RequestedOffset != 0) || (Request.RequestedSize != ~uint64_t(0)))
 						{
 							return true;
@@ -1657,7 +1661,8 @@ CacheRpcHandler::WriteGetCacheChunksResponse([[maybe_unused]] const CacheRequest
 								FragmentRawOffset = Request.RequestedOffset;
 							}
 						}
-						Request.Value			= Request.Value.GetRange(Request.RequestedOffset, Request.RequestedSize);
+						Request.Value =
+							Request.Value.GetRange(Request.RequestedOffset, Request.RequestedSize == 0 ? 1u : Request.RequestedSize);
 						uint64_t FragmentLength = Request.Value.DecodeRawSize();
 
 						IoHashStream FragmentHashStream;
diff --git a/src/zenutil/packageformat.cpp b/src/zenutil/packageformat.cpp
index 2512351f5..963a442cd 100644
--- a/src/zenutil/packageformat.cpp
+++ b/src/zenutil/packageformat.cpp
@@ -51,6 +51,7 @@ FormatPackageMessageBuffer(const CbPackage& Data, FormatFlags Flags, void* Targe
 
 	for (IoBuffer& Buf : Message)
 	{
+		ZEN_ASSERT(Buf.GetSize() > 0);
 		Buffers.push_back(SharedBuffer(Buf));
 	}
 
@@ -200,6 +201,7 @@ FormatPackageMessage(const CbPackage& Data, FormatFlags Flags, void* TargetProce
 	// Root object
 
 	IoBuffer RootIoBuffer = Data.GetObject().GetBuffer().AsIoBuffer();
+	ZEN_ASSERT(RootIoBuffer.GetSize() > 0);
 	ResponseBuffers.push_back(RootIoBuffer);  // Root object
 
 	*AttachmentInfo++ = {.PayloadSize = RootIoBuffer.Size(), .Flags = CbAttachmentEntry::kIsObject, .AttachmentHash = Data.GetObjectHash()};
@@ -257,6 +259,7 @@ FormatPackageMessage(const CbPackage& Data, FormatFlags Flags, void* TargetProce
 
 				for (const SharedBuffer& Segment : Compressed.GetSegments())
 				{
+					ZEN_ASSERT(Segment.GetSize() > 0);
 					ResponseBuffers.push_back(Segment.AsIoBuffer());
 				}
 			}
@@ -264,6 +267,7 @@ FormatPackageMessage(const CbPackage& Data, FormatFlags Flags, void* TargetProce
 		else if (CbObject AttachmentObject = Attachment.AsObject())
 		{
 			IoBuffer ObjIoBuffer = AttachmentObject.GetBuffer().AsIoBuffer();
+			ZEN_ASSERT(ObjIoBuffer.GetSize() > 0);
 			ResponseBuffers.push_back(ObjIoBuffer);
 
 			*AttachmentInfo++ = {.PayloadSize	 = ObjIoBuffer.Size(),
@@ -307,6 +311,7 @@ FormatPackageMessage(const CbPackage& Data, FormatFlags Flags, void* TargetProce
 
 				for (const SharedBuffer& Segment : AttachmentBinary.GetSegments())
 				{
+					ZEN_ASSERT(Segment.GetSize() > 0);
 					ResponseBuffers.push_back(Segment.AsIoBuffer());
 				}
 			}
@@ -808,6 +813,13 @@ TEST_CASE("CbPackage.Serialization")
 	Reader.Finalize();
 }
 
+TEST_CASE("CbPackage.EmptyObject")
+{
+	CbPackage Pkg;
+	Pkg.SetObject({});
+	std::vector<IoBuffer> Result = FormatPackageMessage(Pkg, nullptr);
+}
+
 TEST_CASE("CbPackage.LocalRef")
 {
 	ScopedTemporaryDirectory TempDir;