From 57a4abe246c1e5133d118077c6cd8d48edca422f Mon Sep 17 00:00:00 2001 From: Billy Tifft Date: Wed, 16 Sep 2026 12:01:02 -0600 Subject: [PATCH 1/2] Fix 4-byte out-of-bounds write in BIQ CITY parser --- EngineTests/BiqSectionSizeTests.cs | 27 +++++++++++++++ QueryCiv3/Biq.cs | 53 +++++++++++++++++------------- QueryCiv3/QueryCiv3.csproj | 3 ++ 3 files changed, 60 insertions(+), 23 deletions(-) create mode 100644 EngineTests/BiqSectionSizeTests.cs diff --git a/EngineTests/BiqSectionSizeTests.cs b/EngineTests/BiqSectionSizeTests.cs new file mode 100644 index 000000000..1fd76e4b8 --- /dev/null +++ b/EngineTests/BiqSectionSizeTests.cs @@ -0,0 +1,27 @@ +using System.Runtime.InteropServices; +using QueryCiv3; +using QueryCiv3.Biq; +using Xunit; + +namespace EngineTests; + +public class BiqSectionSizeTests { + // The BIQ reader in QueryCiv3 splits dynamic sections into fixed-size chunks using the + // [SECTION]_LEN_* constants; each constant is an offset bracket into the section's struct. + // The sum of the constants must therefore equal the struct's size. A mismatch means a + // Buffer.MemoryCopy can write past the end of the struct's backing memory. This happened + // for CITY (38 + 36 = 74 vs. sizeof(CITY) = 70), writing 4 bytes past every city array + // element and into the GC heap past the last one. + [Fact] + public void DynamicSectionLengthConstants_SumToStructSize() { + Assert.Equal(Marshal.SizeOf(), BiqData.GOVT_LEN_1 + BiqData.GOVT_LEN_2); + Assert.Equal(Marshal.SizeOf(), BiqData.TERR_LEN_1 + BiqData.TERR_LEN_2); + Assert.Equal(Marshal.SizeOf(), BiqData.RACE_LEN_1 + BiqData.RACE_LEN_2 + BiqData.RACE_LEN_3 + BiqData.RACE_LEN_4); + Assert.Equal(Marshal.SizeOf(), BiqData.CITY_LEN_1 + BiqData.CITY_LEN_2); + Assert.Equal(Marshal.SizeOf(), BiqData.WMAP_LEN_1 + BiqData.WMAP_LEN_2); + Assert.Equal(Marshal.SizeOf(), BiqData.PRTO_LEN_1 + BiqData.PRTO_LEN_2); + Assert.Equal(Marshal.SizeOf(), BiqData.LEAD_LEN_1 + BiqData.LEAD_LEN_2 + BiqData.LEAD_LEN_3); + Assert.Equal(Marshal.SizeOf(), BiqData.RULE_LEN_1 + BiqData.RULE_LEN_2 + BiqData.RULE_LEN_3); + Assert.Equal(Marshal.SizeOf(), BiqData.GAME_LEN_1 + BiqData.GAME_LEN_2 + BiqData.GAME_LEN_3); + } +} diff --git a/QueryCiv3/Biq.cs b/QueryCiv3/Biq.cs index 4fd8f7d24..e9d3527af 100644 --- a/QueryCiv3/Biq.cs +++ b/QueryCiv3/Biq.cs @@ -62,29 +62,31 @@ public class BiqData { // Dynamic sections need to have their static subcomponents read in as discrete chunks, which these length constants help with // The sum of the LEN constants for each section equals the total size of that section's struct // eg. GOVT_LEN_1 + GOVT_LEN_2 == sizeof(GOVT) - private const int GOVT_LEN_1 = 400; - private const int GOVT_LEN_2 = 76; - private const int TERR_LEN_1 = 8; - private const int TERR_LEN_2 = 225; - private const int RACE_LEN_1 = 8; - private const int RACE_LEN_2 = 4; - private const int RACE_LEN_3 = 208; - private const int RACE_LEN_4 = 92; - private const int CITY_LEN_1 = 38; - private const int CITY_LEN_2 = 36; - private const int WMAP_LEN_1 = 8; - private const int WMAP_LEN_2 = 164; - private const int PRTO_LEN_1 = 238; - private const int PRTO_LEN_2 = 21; - private const int LEAD_LEN_1 = 56; - private const int LEAD_LEN_2 = 8; - private const int LEAD_LEN_3 = 33; - private const int RULE_LEN_1 = 104; - private const int RULE_LEN_2 = 164; - private const int RULE_LEN_3 = 32; - private const int GAME_LEN_1 = 16; - private const int GAME_LEN_2 = 5304; - private const int GAME_LEN_3 = 2017; + // This invariant is enforced by the BiqSectionSizeTests test in EngineTests; a mismatch means a + // section's Buffer.MemoryCopy writes past the end of its struct's backing memory. + internal const int GOVT_LEN_1 = 400; + internal const int GOVT_LEN_2 = 76; + internal const int TERR_LEN_1 = 8; + internal const int TERR_LEN_2 = 225; + internal const int RACE_LEN_1 = 8; + internal const int RACE_LEN_2 = 4; + internal const int RACE_LEN_3 = 208; + internal const int RACE_LEN_4 = 92; + internal const int CITY_LEN_1 = 38; + internal const int CITY_LEN_2 = 32; // On-disk records have 4 more trailing bytes per city than this (36); they are skipped by the +4 advance below + internal const int WMAP_LEN_1 = 8; + internal const int WMAP_LEN_2 = 164; + internal const int PRTO_LEN_1 = 238; + internal const int PRTO_LEN_2 = 21; + internal const int LEAD_LEN_1 = 56; + internal const int LEAD_LEN_2 = 8; + internal const int LEAD_LEN_3 = 33; + internal const int RULE_LEN_1 = 104; + internal const int RULE_LEN_2 = 164; + internal const int RULE_LEN_3 = 32; + internal const int GAME_LEN_1 = 16; + internal const int GAME_LEN_2 = 5304; + internal const int GAME_LEN_3 = 2017; public string Title; public string Description; @@ -136,6 +138,11 @@ public unsafe void Load(byte[] biqBytes) { City = new CITY[count]; CityBuilding = new int[count][]; int buildingRowLength = 0; + // On-disk records are 38 + 4 bytes per building + 36, but the CITY struct is only 70 bytes + // (CITY_LEN_1 + CITY_LEN_2 == sizeof(CITY)). Copying the full 36-byte tail would write + // 4 bytes past the struct's backing memory (into the next array element, or past the array + // end for the last city), corrupting the GC heap. CITY_LEN_2 is the struct-modeled tail; + // the 4 trailing on-disk bytes per record are skipped by the +4 below. fixed (void* ptr = City) { byte* cityPtr = (byte*)ptr; diff --git a/QueryCiv3/QueryCiv3.csproj b/QueryCiv3/QueryCiv3.csproj index b54d5860c..e507e6094 100644 --- a/QueryCiv3/QueryCiv3.csproj +++ b/QueryCiv3/QueryCiv3.csproj @@ -9,5 +9,8 @@ + + + From 9580c4f0b7d37ee9b37534cb1f6bc03212d13327 Mon Sep 17 00:00:00 2001 From: Billy Tifft Date: Mon, 21 Sep 2026 09:46:19 -0600 Subject: [PATCH 2/2] Update Biq.cs --- QueryCiv3/Biq.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/QueryCiv3/Biq.cs b/QueryCiv3/Biq.cs index e9d3527af..6b51188cd 100644 --- a/QueryCiv3/Biq.cs +++ b/QueryCiv3/Biq.cs @@ -73,7 +73,7 @@ public class BiqData { internal const int RACE_LEN_3 = 208; internal const int RACE_LEN_4 = 92; internal const int CITY_LEN_1 = 38; - internal const int CITY_LEN_2 = 32; // On-disk records have 4 more trailing bytes per city than this (36); they are skipped by the +4 advance below + internal const int CITY_LEN_2 = 32; internal const int WMAP_LEN_1 = 8; internal const int WMAP_LEN_2 = 164; internal const int PRTO_LEN_1 = 238; @@ -138,11 +138,6 @@ public unsafe void Load(byte[] biqBytes) { City = new CITY[count]; CityBuilding = new int[count][]; int buildingRowLength = 0; - // On-disk records are 38 + 4 bytes per building + 36, but the CITY struct is only 70 bytes - // (CITY_LEN_1 + CITY_LEN_2 == sizeof(CITY)). Copying the full 36-byte tail would write - // 4 bytes past the struct's backing memory (into the next array element, or past the array - // end for the last city), corrupting the GC heap. CITY_LEN_2 is the struct-modeled tail; - // the 4 trailing on-disk bytes per record are skipped by the +4 below. fixed (void* ptr = City) { byte* cityPtr = (byte*)ptr;