CL16570338: CompactBinary: Added validation to LoadCompactBinary and removed asserts from the other load functions

5 files changed, 120 insertions, 45 deletions

diff --git a/zencore/compactbinary.cpp b/zencore/compactbinary.cpp
index 62c60418c..73ca9832f 100644
--- a/zencore/compactbinary.cpp
+++ b/zencore/compactbinary.cpp
@@ -2,6 +2,7 @@
 
 #include "zencore/compactbinary.h"
 
+#include "zencore/compactbinaryvalidation.h"
 #include <zencore/endian.h>
 #include <zencore/stream.h>
 #include <zencore/trace.h>
@@ -1072,7 +1073,10 @@ LoadCompactBinary(BinaryReader& Ar, BufferAllocator Allocator)
 		{
 			break;
 		}
-		ZEN_ASSERT(FieldSize > 0, "Failed to load from invalid compact binary data.");
+		if (FieldSize == 0)
+		{
+			return CbField();
+		}
 	}
 
 	// Allocate the buffer, copy the header, and read the remainder of the field.
@@ -1085,6 +1089,10 @@ LoadCompactBinary(BinaryReader& Ar, BufferAllocator Allocator)
 	{
 		Ar.Read(View.GetData(), View.GetSize());
 	}
+	if (ValidateCompactBinary(Buffer, CbValidateMode::Default) != CbValidateError::None)
+	{
+	return CbField();
+	}
 	return CbField(SharedBuffer(std::move(Buffer)));
 }
 
diff --git a/zencore/compactbinarypackage.cpp b/zencore/compactbinarypackage.cpp
index 4a1ea65d2..b0dbd2918 100644
--- a/zencore/compactbinarypackage.cpp
+++ b/zencore/compactbinarypackage.cpp
@@ -97,27 +97,34 @@ CbAttachment::AsCompactBinary() const
 	return CompactBinary ? CbFieldIterator::MakeRangeView(CompactBinary, Buffer) : CbFieldIterator();
 }
 
-void
-CbAttachment::Load(IoBuffer& InBuffer, BufferAllocator Allocator)
+bool
+CbAttachment::TryLoad(IoBuffer& InBuffer, BufferAllocator Allocator)
 {
 	MemoryInStream InStream(InBuffer.Data(), InBuffer.Size());
 	BinaryReader   Reader(InStream);
 
-	Load(Reader, Allocator);
+	return TryLoad(Reader, Allocator);
 }
 
-void
-CbAttachment::Load(CbFieldIterator& Fields)
+bool
+CbAttachment::TryLoad(CbFieldIterator& Fields)
 {
-	ZEN_ASSERT(Fields.IsBinary());	//, TEXT("Attachments must start with a binary field."));
 	const MemoryView View = Fields.AsBinaryView();
+	if (Fields.HasError())
+	{
+		return false;
+	}
+
 	if (View.GetSize() > 0)
 	{
 		Buffer = SharedBuffer::MakeView(View, Fields.GetOuterBuffer());
 		Buffer.MakeOwned();
 		++Fields;
 		Hash = Fields.AsAttachment();
-		ZEN_ASSERT(!Fields.HasError());	 // TEXT("Attachments must be a non-empty binary value with a content hash."));
+		if (Fields.HasError())
+		{
+			return false;
+		}
 		if (Fields.IsObjectAttachment())
 		{
 			CompactBinary = CbFieldViewIterator::MakeRange(Buffer);
@@ -131,14 +138,19 @@ CbAttachment::Load(CbFieldIterator& Fields)
 		CompactBinary.Reset();
 		Hash = IoHash::Zero;
 	}
+
+	return true;
 }
 
-void
-CbAttachment::Load(BinaryReader& Reader, BufferAllocator Allocator)
+bool
+CbAttachment::TryLoad(BinaryReader& Reader, BufferAllocator Allocator)
 {
-	CbField BufferField = LoadCompactBinary(Reader, Allocator);
-	ZEN_ASSERT(BufferField.IsBinary(), "Attachments must start with a binary field");
-	const MemoryView View = BufferField.AsBinaryView();
+	CbField			 BufferField = LoadCompactBinary(Reader, Allocator);
+	const MemoryView View		 = BufferField.AsBinaryView();
+	if (BufferField.HasError())
+	{
+		return false;
+	}
 	if (View.GetSize() > 0)
 	{
 		Buffer = SharedBuffer::MakeView(View, BufferField.GetOuterBuffer());
@@ -151,7 +163,10 @@ CbAttachment::Load(BinaryReader& Reader, BufferAllocator Allocator)
 			 return UniqueBuffer::MakeMutableView(HashBuffer.data(), Size);
 		 });
 		Hash						   = HashField.AsAttachment();
-		ZEN_ASSERT(!HashField.HasError(), "Attachments must be a non-empty binary value with a content hash.");
+		if (HashField.HasError() || IoHash::HashMemory(Buffer) != Hash)
+		{
+			return false;
+		}
 		if (HashField.IsObjectAttachment())
 		{
 			CompactBinary = CbFieldViewIterator::MakeRange(Buffer);
@@ -163,6 +178,7 @@ CbAttachment::Load(BinaryReader& Reader, BufferAllocator Allocator)
 		CompactBinary.Reset();
 		Hash = IoHash::Zero;
 	}
+	return true;
 }
 
 void
@@ -301,17 +317,17 @@ CbPackage::GatherAttachments(const CbFieldViewIterator& Fields, AttachmentResolv
 	});
 }
 
-void
-CbPackage::Load(IoBuffer& InBuffer, BufferAllocator Allocator, AttachmentResolver* Mapper)
+bool
+CbPackage::TryLoad(IoBuffer& InBuffer, BufferAllocator Allocator, AttachmentResolver* Mapper)
 {
 	MemoryInStream InStream(InBuffer.Data(), InBuffer.Size());
 	BinaryReader   Reader(InStream);
 
-	Load(Reader, Allocator, Mapper);
+	return TryLoad(Reader, Allocator, Mapper);
 }
 
-void
-CbPackage::Load(CbFieldIterator& Fields)
+bool
+CbPackage::TryLoad(CbFieldIterator& Fields)
 {
 	*this = CbPackage();
 	while (Fields)
@@ -324,19 +340,25 @@ CbPackage::Load(CbFieldIterator& Fields)
 		else if (Fields.IsBinary())
 		{
 			CbAttachment Attachment;
-			Attachment.Load(Fields);
+			Attachment.TryLoad(Fields);
 			AddAttachment(Attachment);
 		}
 		else
 		{
-			ZEN_ASSERT(Fields.IsObject(), TEXT("Expected Object, Binary, or Null field when loading a package."));
 			Object = Fields.AsObject();
+			if (Fields->HasError())
+			{
+				return false;
+			}
 			Object.MakeOwned();
 			++Fields;
 			if (Object.CreateIterator())
 			{
 				ObjectHash = Fields.AsObjectAttachment();
-				ZEN_ASSERT(!Fields.HasError(), TEXT("Object must be followed by a CompactBinaryReference with the object hash."));
+				if (Fields.HasError())
+				{
+					return false;
+				}
 				++Fields;
 			}
 			else
@@ -345,10 +367,12 @@ CbPackage::Load(CbFieldIterator& Fields)
 			}
 		}
 	}
+
+	return true;
 }
 
-void
-CbPackage::Load(BinaryReader& Reader, BufferAllocator Allocator, AttachmentResolver* Mapper)
+bool
+CbPackage::TryLoad(BinaryReader& Reader, BufferAllocator Allocator, AttachmentResolver* Mapper)
 {
 	uint8_t	   StackBuffer[64];
 	const auto StackAllocator = [&Allocator, &StackBuffer](uint64_t Size) -> UniqueBuffer {
@@ -365,9 +389,13 @@ CbPackage::Load(BinaryReader& Reader, BufferAllocator Allocator, AttachmentResol
 	for (;;)
 	{
 		CbField ValueField = LoadCompactBinary(Reader, StackAllocator);
+		if (!ValueField)
+		{
+			return false;
+		}
 		if (ValueField.IsNull())
 		{
-			break;
+			return true;
 		}
 		else if (ValueField.IsBinary())
 		{
@@ -399,14 +427,20 @@ CbPackage::Load(BinaryReader& Reader, BufferAllocator Allocator, AttachmentResol
 		}
 		else
 		{
-			ZEN_ASSERT(ValueField.IsObject(), "Expected Object, Binary, or Null field when loading a package");
 			Object = ValueField.AsObject();
+			if (ValueField.HasError())
+			{
+				return false;
+			}
 			Object.MakeOwned();
 			if (Object.CreateViewIterator())
 			{
 				CbField HashField = LoadCompactBinary(Reader, StackAllocator);
 				ObjectHash		  = HashField.AsObjectAttachment();
-				ZEN_ASSERT(!HashField.HasError(), "Object must be followed by a ObjectAttachment with the object hash.");
+				if (HashField.HasError() || Object.GetHash() != ObjectHash)
+				{
+					return false;
+				}
 			}
 			else
 			{
@@ -466,14 +500,14 @@ TEST_CASE("usonpackage")
 		CHECK(ValidateObjectAttachment(MakeMemoryView(WriteStream), CbValidateMode::All) == CbValidateError::None);
 
 		CbAttachment FromFields;
-		FromFields.Load(Fields);
+		FromFields.TryLoad(Fields);
 		CHECK(!bool(Fields));
 		CHECK(FromFields == Attachment);
 
 		CbAttachment   FromArchive;
 		MemoryInStream InStream(MakeMemoryView(WriteStream));
 		BinaryReader   Reader(InStream);
-		FromArchive.Load(Reader);
+		FromArchive.TryLoad(Reader);
 		CHECK(Reader.CurrentOffset() == InStream.Size());
 		CHECK(FromArchive == Attachment);
 	};
@@ -564,7 +598,7 @@ TEST_CASE("usonpackage")
 		Attachment.Save(Writer);
 		CbFieldIterator Fields	   = Writer.Save();
 		CbFieldIterator FieldsView = CbFieldIterator::MakeRangeView(CbFieldViewIterator(Fields));
-		Attachment.Load(FieldsView);
+		Attachment.TryLoad(FieldsView);
 
 		CHECK_FALSE(Attachment.IsNull());
 		CHECK(bool(Attachment));
@@ -590,7 +624,7 @@ TEST_CASE("usonpackage")
 		CbFieldIterator Fields	   = Writer.Save();
 		CbFieldIterator FieldsView = CbFieldIterator::MakeRangeView(CbFieldViewIterator(Fields));
 
-		Attachment.Load(FieldsView);
+		Attachment.TryLoad(FieldsView);
 
 		CHECK_FALSE(Attachment.IsNull());
 		CHECK(bool(Attachment));
@@ -669,14 +703,14 @@ TEST_CASE("usonpackage.serialization")
 		CHECK(ValidateCompactBinaryPackage(MakeMemoryView(MemStream), CbValidateMode::All) == CbValidateError::None);
 
 		CbPackage FromFields;
-		FromFields.Load(Fields);
+		FromFields.TryLoad(Fields);
 		CHECK_FALSE(bool(Fields));
 		CHECK(FromFields == Package);
 
 		CbPackage	   FromArchive;
 		MemoryInStream ReadMemStream(MakeMemoryView(MemStream));
 		BinaryReader   ReadAr(ReadMemStream);
-		FromArchive.Load(ReadAr);
+		FromArchive.TryLoad(ReadAr);
 		CHECK(ReadAr.CurrentOffset() == ReadMemStream.Size());
 		CHECK(FromArchive == Package);
 	};
@@ -869,7 +903,7 @@ TEST_CASE("usonpackage.serialization")
 
 		CbFieldIterator Fields = Writer.Save();
 		CbPackage		FromFields;
-		FromFields.Load(Fields);
+		FromFields.TryLoad(Fields);
 
 		const CbAttachment* const Level2Attachment = FromFields.FindAttachment(Level2Hash);
 		REQUIRE(Level2Attachment);
@@ -902,7 +936,7 @@ TEST_CASE("usonpackage.serialization")
 		CbPackage	   FromArchive;
 		MemoryInStream ReadStream(MakeMemoryView(WriteStream));
 		BinaryReader   ReadAr(ReadStream);
-		FromArchive.Load(ReadAr);
+		FromArchive.TryLoad(ReadAr);
 
 		Writer.Reset();
 		FromArchive.Save(Writer);
diff --git a/zencore/include/zencore/compactbinary.h b/zencore/include/zencore/compactbinary.h
index 2a4d250cb..4c9b48d3a 100644
--- a/zencore/include/zencore/compactbinary.h
+++ b/zencore/include/zencore/compactbinary.h
@@ -1262,6 +1262,17 @@ CbField::AsArray() &&
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
+/**
+ * Load a compact binary field from an archive.
+ *
+ * The field may be an array or an object, which the caller can convert to by using AsArray or
+ * AsObject as appropriate. The buffer allocator is called to provide the buffer for the field
+ * to load into once its size has been determined.
+ *
+ * @param Ar Archive to read the field from. An error state is set on failure.
+ * @param Allocator Allocator for the buffer that the field is loaded into.
+ * @return A field with a reference to the allocated buffer, or a default field on failure.
+ */
 ZENCORE_API CbField LoadCompactBinary(BinaryReader& Ar, BufferAllocator Allocator);
 
 inline CbObject
diff --git a/zencore/include/zencore/compactbinarypackage.h b/zencore/include/zencore/compactbinarypackage.h
index 03098f494..d7a8ea7fc 100644
--- a/zencore/include/zencore/compactbinarypackage.h
+++ b/zencore/include/zencore/compactbinarypackage.h
@@ -83,17 +83,17 @@ public:
 	 *
 	 * The iterator is advanced as attachment fields are consumed from it.
 	 */
-	ZENCORE_API void Load(CbFieldIterator& Fields);
+	ZENCORE_API bool TryLoad(CbFieldIterator& Fields);
 
 	/**
 	 * Load the attachment from compact binary as written by Save.
 	 */
-	ZENCORE_API void Load(BinaryReader& Reader, BufferAllocator Allocator = UniqueBuffer::Alloc);
+	ZENCORE_API bool TryLoad(BinaryReader& Reader, BufferAllocator Allocator = UniqueBuffer::Alloc);
 
 	/**
 	 * Load the attachment from compact binary as written by Save.
 	 */
-	ZENCORE_API void Load(IoBuffer& Buffer, BufferAllocator Allocator = UniqueBuffer::Alloc);
+	ZENCORE_API bool TryLoad(IoBuffer& Buffer, BufferAllocator Allocator = UniqueBuffer::Alloc);
 
 	/** Save the attachment into the writer as a stream of compact binary fields. */
 	ZENCORE_API void Save(CbWriter& Writer) const;
@@ -277,11 +277,11 @@ public:
 	 *
 	 * The iterator is advanced as object and attachment fields are consumed from it.
 	 */
-	ZENCORE_API void Load(CbFieldIterator& Fields);
+	ZENCORE_API bool TryLoad(CbFieldIterator& Fields);
 
-	ZENCORE_API void Load(IoBuffer& Buffer, BufferAllocator Allocator = UniqueBuffer::Alloc, AttachmentResolver* Mapper = nullptr);
+	ZENCORE_API bool TryLoad(IoBuffer& Buffer, BufferAllocator Allocator = UniqueBuffer::Alloc, AttachmentResolver* Mapper = nullptr);
 
-	ZENCORE_API void Load(BinaryReader& Reader, BufferAllocator Allocator = UniqueBuffer::Alloc, AttachmentResolver* Mapper = nullptr);
+	ZENCORE_API bool TryLoad(BinaryReader& Reader, BufferAllocator Allocator = UniqueBuffer::Alloc, AttachmentResolver* Mapper = nullptr);
 
 	/** Save the object and attachments into the writer as a stream of compact binary fields. */
 	ZENCORE_API void Save(CbWriter& Writer) const;
diff --git a/zenserver/projectstore.cpp b/zenserver/projectstore.cpp
index 750ecdb6a..534983347 100644
--- a/zenserver/projectstore.cpp
+++ b/zenserver/projectstore.cpp
@@ -530,15 +530,34 @@ ProjectStore::Oplog::AppendNewOplogEntry(CbPackage OpPackage)
 
 	// Persist attachments
 
+	uint64_t AttachmentBytes	= 0;
+	uint64_t NewAttachmentBytes = 0;
+
 	auto Attachments = OpPackage.GetAttachments();
 
 	for (const auto& Attach : Attachments)
 	{
-		IoBuffer AttachmentData = Attach.AsBinaryView().AsIoBuffer();
-		m_CasStore.InsertChunk(AttachmentData, Attach.GetHash());
+		IoBuffer			   AttachmentData = Attach.AsBinaryView().AsIoBuffer();
+		CasStore::InsertResult Result		  = m_CasStore.InsertChunk(AttachmentData, Attach.GetHash());
+
+		const uint64_t AttachmentSize = AttachmentData.Size();
+
+		if (Result.New)
+		{
+			NewAttachmentBytes += AttachmentSize;
+		}
+
+		AttachmentBytes += AttachmentSize;
 	}
 
-	return RegisterOplogEntry(Core, OpEntry, kUpdateNewEntry);
+	const uint32_t EntryId = RegisterOplogEntry(Core, OpEntry, kUpdateNewEntry);
+
+	Log().debug("oplog entry #{} attachments: {}B new, {}B total",
+				EntryId,
+				zen::NiceBytes(NewAttachmentBytes),
+				zen::NiceBytes(AttachmentBytes));
+
+	return EntryId;
 }
 
 //////////////////////////////////////////////////////////////////////////
@@ -1367,7 +1386,10 @@ HttpProjectService::HttpProjectService(CasStore& Store, ProjectStore* Projects)
 			};
 
 			CbPackage Package;
-			Package.Load(Payload, &UniqueBuffer::Alloc, &Resolver);
+			if (!Package.TryLoad(Payload, &UniqueBuffer::Alloc, &Resolver))
+			{
+				return HttpReq.WriteResponse(HttpResponse::BadRequest, HttpContentType::kText, "Invalid package");
+			}
 
 			if (!IsValid)
 			{