bugfix(replay): Validate next frame value in RecorderClass::readNextFrame for replay playback - #3090
Conversation
|
|
||
| // 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 |
There was a problem hiding this comment.
m_mode isn't updated yet when this called for the first time.
| // 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 | ||
| || (m_nextFrame >= TheGameLogic->getFrame() && m_nextFrame < TheGameLogic->getFrame() + 3600 * LOGICFRAMES_PER_SECOND)); |
There was a problem hiding this comment.
I can lower the maximum if one hour is too extreme.
There was a problem hiding this comment.
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.
| // 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 | ||
| || (m_nextFrame >= TheGameLogic->getFrame() && m_nextFrame < TheGameLogic->getFrame() + 3600 * LOGICFRAMES_PER_SECOND)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can see a couple of complications with that:
- We'd need to hold on to the message data, or seek in the file stream.
- 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 exceedsMAX_PLAYER_COUNT, those could be relatively straightforward signs.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
This PR adds a sanity check to
RecorderClass::readNextFrameto prevent replays with corrupt data from taking forever to complete. I came across a handful of GenTool replays that had this issue.Non-headless mode has a fail-safe that partially mitigates the issue there. The script engine triggers a call to
GameLogic::exitGameon the final victory / defeat screen, and this adds aGameMessage::MSG_CLEAR_GAME_DATAmessage to the message stream. This ends replay playback. The message stream is ignored in headless mode, so this doesn't apply there. It's also not a complete fix because the corrupted data may occur before a victory or defeat takes place.The maximum next frame value is set one hour away, which seems more than reasonable to me. The only way this generates a false positive is if no game action is taken for an hour or more and the CRC interval is set to an hour or more (it's a couple of seconds by default).
TODO:
@Skyaero42 Tagging you so you're aware of this PR.