Skip to content

bugfix(replay): Validate next frame value in RecorderClass::readNextFrame for replay playback #3090

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.

Sign up for GitHub

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

Open
Caball009 wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −2
Open

bugfix(replay): Validate next frame value in RecorderClass::readNextFrame for replay playback #3090

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1277,8 +1277,19 @@ AsciiString RecorderClass::readAsciiString() {
*/
void RecorderClass::readNextFrame() {
Int bytesRead = m_file->read(&m_nextFrame, sizeof(m_nextFrame));
if (bytesRead != sizeof(m_nextFrame)) {
DEBUG_LOG(("RecorderClass::readNextFrame - read failed on frame %d", TheGameLogic->getFrame()));

// TheSuperHackers @bugfix Check whether the next frame value is within a reasonable range
// to avoid prolonging playback due to potentially corrupted data.
const Bool validFrameValue = (m_mode == RECORDERMODETYPE_NONE

@Caball009 Caball009 Aug 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

m_mode isn't updated yet when this called for the first time.

Comment thread
Caball009 marked this conversation as resolved.
|| (m_nextFrame >= TheGameLogic->getFrame() && m_nextFrame < TheGameLogic->getFrame() + 3600 * LOGICFRAMES_PER_SECOND));

@Caball009 Caball009 Aug 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I can lower the maximum if one hour is too extreme.

@Skyaero42 Skyaero42 Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Open for discussion:

  • CRC value is written every 100 frames iirc?
  • How likely is it that it will be changed to a much higher value? IMHO it would defeat the purpose of CRC
  • Late game replays can run slowly, even close to 1:1 (i.e. FF doesn't do anything). A margin of 1 hour means it may still take an hour before the replay ends.

Based on the above, my suggestion would be somewhere between 1 and 10 minutes.

Alternatively - and maybe the better option. Create a parameter in options.ini that sets it.

@xezon xezon Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe a way to make this more robust would be to also read the next message in advance if the next frame is far away and see if that reads correctly or fails. This then gives higher confidence that the message is broken.

@Caball009 Caball009 Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I can see a couple of complications with that:

  1. We'd need to hold on to the message data, or seek in the file stream.
  2. It's not so obvious to me what an invalid message would look like. I suppose if the type value exceeds MSG_END_NETWORK_MESSAGES, or the player index exceeds MAX_PLAYER_COUNT, those could be relatively straightforward signs.

@xezon xezon Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Having validate methods for all messages would perhaps be a nice thing to have in general. Validate all their bounds, and if they fail validation, then reject them. Right now we hope that the various message handlers deal with validation. But they probably do not sufficiently and there are holes.

@Caball009 Caball009 Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It'd be a nice thing, but not something I intend to do at this point. I'd rather close this PR and focus on other work if that's the desired change.


if (bytesRead != sizeof(m_nextFrame) || !validFrameValue) {
if (bytesRead != sizeof(m_nextFrame)) {
DEBUG_LOG(("RecorderClass::readNextFrame - read failed on frame %d", TheGameLogic->getFrame()));
} else {
DEBUG_CRASH(("RecorderClass::readNextFrame - current frame %d, next frame %d in the replay appears invalid",
TheGameLogic->getFrame(), m_nextFrame));
}
m_nextFrame = -1;
stopPlayback();
}
Expand Down
Loading