Skip to content

Fix 2.5.0 parsing errors - #38

Open
scharissis wants to merge 1 commit into
ryantaylor:masterfrom
scharissis:coh3-2.5.0
Open

Fix 2.5.0 parsing errors#38
scharissis wants to merge 1 commit into
ryantaylor:masterfrom
scharissis:coh3-2.5.0

Conversation

@scharissis

Copy link
Copy Markdown

Patch 2.5.0 (game version 48652) broke parsing in two independent places. Every replay recorded after the patch fails, at the same offset:

Parsing Failure: Error { input: LocatedSpan { offset: 5180, ... }, code: Eof }

1. The per-player trailing list

parse_player skips four bytes with the comment "players in this chunk version or later have an extra 4 bytes between them for some reason". Those four bytes are a count, and it was zero in every replay up to build 46673 — which is why it read as padding. 2.5.0 populates it with one 6-byte record per entry (u32 pbgid, u16 slot), which lines up with the patch's lobby and battlegroup-selection rework.

Consuming only the count leaves the reader inside a record, so the next player's UTF-16 name length is read from the middle of a pbgid — a 16-million-character name, and Eof.

Reading it as a list needs no version gate: a count of zero consumes exactly the four bytes the old code did. I checked every player in 9 automatch replays on 46673 and the value is zero in all of them.

2. The message tick's leading u32 is a kind, not a message count

MessageTick::parse_message peeks the first u32 of the payload and treats it as a number of messages: 0 means empty, anything else means "read a 20-byte header, then that many messages". 2.5.0 introduced kind 2, whose entire body is 4 bytes:

tick_type=1 payload_len=12
payload: 02 00 00 00 | 04 00 00 00 | 00 00 00 00
         kind          body length   body

The content path then reads 20 bytes out of a 12-byte payload and fails the replay. (Chat is kind 1: 01 00 00 00 | 1e 00 00 00 | slot u32 | sender u64 | name | message, and the second u32 is the length of everything after it in every sample I have.)

Across a 2.4.2 corpus that leading value is only ever 1 (52 ticks) or 0 (3), so kind and count are indistinguishable from data alone — but two messages do not fit in four bytes. This PR dispatches on the kind, parses chat inside the body's own declared length, and skips an unrecognised kind by that same length, so a future kind cannot break parsing either.

Verification

  • cargo test --features serde,raw: 19 passed, 0 failed (regression needs replays/regression, which isn't in the repo).
  • Chat output over the 17 replays in replays/ plus 9 automatch replays on 46673: 108 message lines, byte-identical before and after.
  • A separate consumer's 9 committed conformance snapshots (full normalized parse of each replay, serialized to JSON) reproduce byte for byte.
  • 7 replays on 48652: total failure → every player and command parsed.

The tradeoff worth flagging: for kind 1 this reads exactly one message per tick rather than many_m_n(1, count, ...). Every chat tick I can observe carries exactly one and its body length accounts for it precisely, but if you know of a tick that legitimately carries several, say so and I'll rework it to consume messages until the body is exhausted.

I did not add a test replay because the only 2.5.0 replays I have are my own and carry my Steam ID — happy to send one privately, or to add a fixture if you'd rather have one in the repo.

Patch 2.5.0 (game version 48652) broke parsing in two independent places, and
every replay recorded after it failed at the same offset.

The per-player trailing list. What this parser skipped as "an extra 4 bytes
between players" is a count, and it was zero in every replay up to build 46673 --
which is why it read as padding. 2.5.0 populates it with one 6-byte record per
entry (u32 pbgid, u16 slot), consistent with the patch's lobby and
battlegroup-selection rework. Consuming only the count left the reader inside a
record, so the next player's UTF-16 name length was read from the middle of a
pbgid: a 16-million-character name, and Eof. Parsing the list needs no version
gate, because a count of zero consumes exactly the four bytes the old code did.

The message tick's leading u32 is a kind, not a message count. 2.5.0 added kind
2, whose whole body is four bytes -- shorter than the 20-byte header the "content"
path reads unconditionally -- so it ran off the end of a length-delimited payload
and failed the replay. Across a 2.4.2 corpus that value is only ever 1 (52 ticks)
or 0 (3), so kind and count were indistinguishable from data alone; two messages
do not fit in four bytes. Chat is now parsed inside the body's own declared
length, and an unrecognised kind is skipped by that same length, so the next new
kind cannot break parsing either.

Verified on the 17 replays in replays/ plus 9 automatch replays on build 46673 and
7 on 48652: cargo test --features serde,raw passes 19/19, chat message output is
byte-identical before and after, and the 48652 replays go from total failure to
parsing every player and command.
@scharissis

Copy link
Copy Markdown
Author

Hey @ryantaylor , sending this Claude (Opus 5 - High) generated PR in case it's of assistance. Keen for the 2.5.0 updates for a side-project of mine.
Feel free to reject/close if it's not. Or, let me know what else I can do to help push it through.

Cheers!

@ryantaylor

Copy link
Copy Markdown
Owner

Hey @scharissis ! Appreciate the PR! I'll take a look, but just a heads up that in the near future this repo will be deprecated in favour of https://github.com/ryantaylor/cohlib, which is a holistic CoH3 tooling library that I've been working on and running at cohdb in production over the past few months.

Interestingly, I haven't noticed any parsing errors in 2.5.0 on cohdb so far, but I only ingest automatch games so I wonder if this only affects lobby matches considering the changes are lobby-specific.

@scharissis

Copy link
Copy Markdown
Author

Ah, good to know!

I've not played any automatches this patch yet, so I tested with 'Final Stand' "replays". When I get a proper replay I'll test with that. Future PR's will go to 'cohlib' instead.

@scharissis

Copy link
Copy Markdown
Author

I don't think this is lobby-specific, fwiw.

Also, 'cohlib' has no LICENSE; you may want to add one.

@scharissis

Copy link
Copy Markdown
Author

@ryantaylor - FYI, I did some checks and then moved this over to cohlib: ryantaylor/cohlib#8

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.

2 participants