Fix 4-byte out-of-bounds write in BIQ CITY parser (#865) - #996
Open
Billytifft wants to merge 1 commit into
Open
Billytifft wants to merge 1 commit into
Billytifft wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #865
Summary
The BIQ reader in
QueryCiv3parses the dynamicCITYsection withBuffer.MemoryCopyover raw pointers. The documented split wasCITY_LEN_1(38) +CITY_LEN_2(36) = 74 bytes, butsizeof(CITY)is only 70 bytes (Pack=1). The 36-byte tail copy therefore wrote 4 bytes past every city's struct: into the next array element'sLengthfield, and past the end of theCITY[]array for the last city. That final 4-byte spill lands in the GC heap, corrupting whichever object happened to be allocated right after the array.This is a strong candidate root cause for #865: the corruption surfaces later, deep inside CoreCLR, as "memory corruption" / malloc-guard failures (
getPgoInstrumentationResults,VirtualCallStubManager,nanov2_guard_corruption_detected/_xzm_xzone_malloc_freelist_outlined), depending on what allocation was hit.Fix
Set
CITY_LEN_2 = 32soCITY_LEN_1 + CITY_LEN_2 == sizeof(CITY)= 70. On-disk records are still 38 + 4 per building + 36 bytes; the 4 unmodeled trailing bytes per record are already accounted for by the existing+ 4in the record advance (dataPtr += City[i].Length + 4), so parse positions and field values are unchanged.Regression guard
Added
EngineTests/BiqSectionSizeTests, which asserts the documented invariant ("the sum of theLENconstants equals the struct size") for every dynamic BIQ section (GOVT, TERR, RACE, CITY, WMAP, PRTO, LEAD, RULE, GAME).CITY_LEN_*are nowinternalfor the test. This would have caught the bug the moment either the constant or the struct changed, and it runs on every CI. No other dynamic section has this mismatch; the SAV parser already usessizeof-based generic copies.Verification
dotnet build C7/C7.sln- 0 errorsdotnet test C7/C7.sln(withCIV3_HOME) - 71/71 pass, including loading2 Rise of Rome.biqand all Conquest scenarios and playing turnsdotnet format C7/C7.sln whitespace --verify-no-changes- clean