Skip to content

Fix BasePropertyDesc minimum bound flag parsing - #4514

Open
KT053374 wants to merge 1 commit into
ACEmulator:masterfrom
KT053374:feature/datloader-property-min-flag
Open

KT053374 wants to merge 1 commit into
ACEmulator:masterfrom
KT053374:feature/datloader-property-min-flag

Conversation

@KT053374

Copy link
Copy Markdown
Contributor

Fix BasePropertyDesc minimum bound flag parsing

Summary

Corrects a conditional parsing defect in BasePropertyDesc.Unpack() where the minimum-value field was incorrectly controlled by the hasMax flag instead of its own hasMin flag.

Problem

During a semantic audit of ACE.DatLoader, we identified that BasePropertyDesc.cs reads the hasMin flag but subsequently checks hasMax when 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 an EndOfStreamException.

Changes

  • Updated BasePropertyDesc.Unpack() to check hasMin when reading the minimum-value field.
  • Added comments explaining why the minimum and maximum presence flags must be evaluated independently.
  • Added BasePropertyDescMinMaxTests.cs containing four data-driven regression scenarios covering every combination of the two flags.
  • Regression tests validate decoded values and ensure the parser consumes the expected number of bytes.

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 MasterProperty data identified 383 property definitions:

  • 374 with neither minimum nor maximum specified.
  • 9 with both minimum and maximum specified.
  • 0 with differing minimum and maximum presence flags.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant