Repository navigation
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.
Fix BasePropertyDesc minimum bound flag parsing
Summary
Corrects a conditional parsing defect in
BasePropertyDesc.Unpack()where the minimum-value field was incorrectly controlled by thehasMaxflag instead of its ownhasMinflag.Problem
During a semantic audit of
ACE.DatLoader, we identified thatBasePropertyDesc.csreads thehasMinflag but subsequently checkshasMaxwhen determining whether to consume the minimum-value field.When the minimum and maximum presence flags differ, this causes two possible parsing failures:
hasMax = false,hasMin = true: The minimum value is not read, leaving bytes unconsumed and producing an incorrect decoded value.hasMax = true,hasMin = false: The parser attempts to read a minimum value that does not exist, potentially misaligning subsequent fields or causing anEndOfStreamException.Changes
BasePropertyDesc.Unpack()to checkhasMinwhen reading the minimum-value field.BasePropertyDescMinMaxTests.cscontaining four data-driven regression scenarios covering every combination of the two flags.Validation
The defect was reproduced using synthetic binary records before applying the correction.
Before the fix: 2 tests passed, 2 failed.
After the fix: All 4 regression tests passed.
An independent diagnostic also verified that all four flag combinations now decode correctly without leaving unread bytes.
Additionally, inspection of the End of Retail
MasterPropertydata identified 383 property definitions:This explains why the issue was not exposed by existing End of Retail DAT decoding tests.
Impact
This change improves parser correctness and compatibility with property definitions that specify only a minimum or only a maximum value.
Existing End of Retail property definitions remain unaffected because their minimum and maximum presence flags are either both set or both unset.
The change is limited to correcting the conditional check and adding regression coverage. No DAT format changes are introduced.