LoadCompactBinary gracefully handles read failures and sizes larger than the archive (#165)

* add failing test
* CompactBinary: Fixed LoadCompactBinary to gracefully handle read failures and sizes larger than the archive
From https://p4-swarm.epicgames.net/changes/21983905
* changelog

3 files changed, 72 insertions, 25 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6cd94a99a..57b655c00 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,7 @@
 ##
 - Feature: Adding a `.json` extension to the `--abslog` option will make zenserver log in json format to file
-- Feature: Create release in Sentry and use `sentry_options_set_release` to associate the executable 
+- Feature: Create release in Sentry and use `sentry_options_set_release` to associate the executable
+- Bugfix: CompactBinary: Fixed LoadCompactBinary to gracefully handle read failures and sizes larger than the archive. From https://p4-swarm.epicgames.net/changes/21983905
 
 ## v0.1.5
 - Bugfix: Don't fail entire request if GetCacheValue from Horde fails for a single value
diff --git a/zencore/compactbinary.cpp b/zencore/compactbinary.cpp
index 375a97fc5..0e4a46fa1 100644
--- a/zencore/compactbinary.cpp
+++ b/zencore/compactbinary.cpp
@@ -1355,39 +1355,42 @@ LoadCompactBinary(BinaryReader& Ar, BufferAllocator Allocator)
 	CbFieldType			 FieldType;
 	uint64_t			 FieldSize = 1;
 
-	// Read in small increments until the total field size is known, to avoid reading too far.
-	for (;;)
+	for (const int64_t StartPos = Ar.CurrentOffset(); FieldSize > 0;)
 	{
-		const int32_t ReadSize	 = int32_t(FieldSize - HeaderBytes.size());
-		const size_t  ReadOffset = HeaderBytes.size();
+		// Read in small increments until the total field size is known, to avoid reading too far.
+		const int32_t ReadSize = int32_t(FieldSize - HeaderBytes.size());
+		if (Ar.CurrentOffset() + ReadSize > Ar.GetSize())
+		{
+			break;
+		}
+
+		const size_t ReadOffset = HeaderBytes.size();
 		HeaderBytes.resize(ReadOffset + ReadSize);
 
 		Ar.Read(HeaderBytes.data() + ReadOffset, ReadSize);
 		if (TryMeasureCompactBinary(MakeMemoryView(HeaderBytes), FieldType, FieldSize))
 		{
+			if (FieldSize <= uint64_t(Ar.Size() - StartPos))
+			{
+				UniqueBuffer Buffer = Allocator(FieldSize);
+				ZEN_ASSERT(Buffer.GetSize() == FieldSize);
+				MutableMemoryView View = Buffer.GetMutableView();
+				memcpy(View.GetData(), HeaderBytes.data(), HeaderBytes.size());
+				View.RightChopInline(HeaderBytes.size());
+				if (!View.IsEmpty())
+				{
+					// Read the remainder of the field.
+					Ar.Read(View.GetData(), View.GetSize());
+				}
+				if (ValidateCompactBinary(Buffer, CbValidateMode::Default) == CbValidateError::None)
+				{
+					return CbField(SharedBuffer(std::move(Buffer)));
+				}
+			}
 			break;
 		}
-		if (FieldSize == 0)
-		{
-			return CbField();
-		}
 	}
-
-	// Allocate the buffer, copy the header, and read the remainder of the field.
-	UniqueBuffer Buffer = Allocator(FieldSize);
-	ZEN_ASSERT(Buffer.GetSize() == FieldSize);
-	MutableMemoryView View = Buffer.GetMutableView();
-	memcpy(View.GetData(), HeaderBytes.data(), HeaderBytes.size());
-	View.RightChopInline(HeaderBytes.size());
-	if (!View.IsEmpty())
-	{
-		Ar.Read(View.GetData(), View.GetSize());
-	}
-	if (ValidateCompactBinary(Buffer, CbValidateMode::Default) != CbValidateError::None)
-	{
-		return CbField();
-	}
-	return CbField(SharedBuffer(std::move(Buffer)));
+	return CbField();
 }
 
 CbObject
@@ -2290,6 +2293,7 @@ TEST_CASE("json.uson")
 		CHECK(Cb["value"sv].AsHash() == Hash);
 	}
 }
+
 #endif
 
 }  // namespace zen
diff --git a/zencore/compactbinarypackage.cpp b/zencore/compactbinarypackage.cpp
index 6eaca5fdf..a25466bed 100644
--- a/zencore/compactbinarypackage.cpp
+++ b/zencore/compactbinarypackage.cpp
@@ -1279,6 +1279,48 @@ TEST_CASE("usonpackage.serialization")
 	}
 }
 
+TEST_CASE("usonpackage.invalidpackage")
+{
+	const auto TestLoad = [](std::initializer_list<uint8_t> RawData, BufferAllocator Allocator = UniqueBuffer::Alloc) {
+		const MemoryView RawView = MakeMemoryView(RawData);
+		CbPackage		 FromArchive;
+		BinaryReader	 ReadAr(RawView);
+		CHECK_FALSE(FromArchive.TryLoad(ReadAr, Allocator));
+	};
+	const auto AllocFail = [](uint64_t) -> UniqueBuffer {
+		FAIL_CHECK("Allocation is not expected");
+		return UniqueBuffer();
+	};
+	SUBCASE("Empty") { TestLoad({}, AllocFail); }
+	SUBCASE("Invalid Initial Field")
+	{
+		TestLoad({uint8_t(CbFieldType::None)});
+		TestLoad({uint8_t(CbFieldType::Array)});
+		TestLoad({uint8_t(CbFieldType::UniformArray)});
+		TestLoad({uint8_t(CbFieldType::Binary)});
+		TestLoad({uint8_t(CbFieldType::String)});
+		TestLoad({uint8_t(CbFieldType::IntegerPositive)});
+		TestLoad({uint8_t(CbFieldType::IntegerNegative)});
+		TestLoad({uint8_t(CbFieldType::Float32)});
+		TestLoad({uint8_t(CbFieldType::Float64)});
+		TestLoad({uint8_t(CbFieldType::BoolFalse)});
+		TestLoad({uint8_t(CbFieldType::BoolTrue)});
+		TestLoad({uint8_t(CbFieldType::ObjectAttachment)});
+		TestLoad({uint8_t(CbFieldType::BinaryAttachment)});
+		TestLoad({uint8_t(CbFieldType::Uuid)});
+		TestLoad({uint8_t(CbFieldType::DateTime)});
+		TestLoad({uint8_t(CbFieldType::TimeSpan)});
+		TestLoad({uint8_t(CbFieldType::ObjectId)});
+		TestLoad({uint8_t(CbFieldType::CustomById)});
+		TestLoad({uint8_t(CbFieldType::CustomByName)});
+	}
+	SUBCASE("Size Out Of Bounds")
+	{
+		TestLoad({uint8_t(CbFieldType::Object), 1}, AllocFail);
+		TestLoad({uint8_t(CbFieldType::Object), 0xff, 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc, 0xde, 0xf0}, AllocFail);
+	}
+}
+
 #endif
 
 }  // namespace zen