Skip to content

[GEN][ZH] Fix replay header mismatch between builds using 32 bit or 64 bit wide time_t #765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 28, 2025

Conversation

helmutbuhler
Copy link

Another day goes by, and another bug is discovered. But this time, it's not a bug in the retail version, but in our build with VS22.
The replay header contains (among other) these elements:

  • starttime
  • endtime
  • number of frames
  • mismatch flag
  • quitEarly flag
  • disconnect flag (for each player)
  • replayName (string)

start and endtime are of type time_t. On VC6 this was 32-bit, but on newer compilers it's 64-bit. In order to remain compatible, we need to load and save them as 32-bit. This PR fixes that.

Now you may ask: Wait, why did we not notice until now? Why are replays working?
Well, the replayName syncs everything up. It's serialized with a special character to indicate its end, and since all other header elements very likely don't contain that character, the serialization is automatically fixed there. But all the replay header elements listed above are indeed parsed and written out corrupted. We also didn't notice that, because, as it turns out, all those elements are invisible to the user.

We should still fix it though, so we don't spread the internet with corrupted replays. I'd also like to use the mismatch flag to harvest some non-mismatched replays for automatic testing in the near future...

@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Fix Is fixing something labels Apr 26, 2025
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Needs to be replicated in Generals.

@xezon
Copy link

xezon commented Apr 28, 2025

This 64 bit time_t breaks GenTool replay uploads in VS 2022 builds, because it cannot read the replay header.

@xezon xezon changed the title [ZH] Fix replay header memory corruption on VS22 [ZH] Fix replay header mismatch between builds using 32 bit or 64 bit wide time_t Apr 28, 2025
@xezon xezon added the Stability Concerns stability of the runtime label Apr 28, 2025
@xezon
Copy link

xezon commented Apr 28, 2025

I locally replicated this change to Generals but I am unable to push this to the remote branch

"git" push --recurse-submodules=check --progress "helmutbuhler" refs/heads/fix_replay_header_sh:refs/heads/fix_replay_header_sh
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
ERROR: Permission to helmutbuhler/CnC_Generals_Zero_Hour.git denied to xezon.

@xezon xezon force-pushed the fix_replay_header_sh branch from d1ab2ba to c340968 Compare April 28, 2025 21:53
@xezon xezon added this to the Stability fixes milestone Apr 28, 2025
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replicated in Generals. Looks good.

@xezon xezon merged commit 59e5b4e into TheSuperHackers:main Apr 28, 2025
18 checks passed
@xezon xezon deleted the fix_replay_header_sh branch April 28, 2025 22:37
xezon pushed a commit that referenced this pull request Apr 28, 2025
@xezon xezon changed the title [ZH] Fix replay header mismatch between builds using 32 bit or 64 bit wide time_t [GEN][ZH] Fix replay header mismatch between builds using 32 bit or 64 bit wide time_t Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Is fixing something Minor Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants