Skip to content
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

GROOVIE: Fix #2 possible crash with incorrect seeks to endpos in roq.cpp #4257

Merged
merged 6 commits into from Sep 25, 2022

Conversation

lolovo
Copy link
Contributor

@lolovo lolovo commented Sep 6, 2022

Fix bug - https://bugs.scummvm.org/ticket/13832

This fix is related to this commit - 78a19f1

I've got crash on puzzle according to this issue. This fix resolves it. Break is needed, to avoid artifactes on screen.

@lolovo
Copy link
Contributor Author

lolovo commented Sep 10, 2022

Comment about last changes/optimization - we need remain debug info and also need set _dirty to false to avoid artifacts on screen in case if we've got wrong skipBytes in ROQPlayer::processBlockQuadVector that will skipped right after switch in this case and warning will be.

I've completed the localized 11th hour and well as original GOG version. All work fine without any issues. Also tested Clandestiny a little bit - also works fine.

@Die4Ever Die4Ever self-assigned this Sep 17, 2022
int32 skipBytes = endpos -_file->pos();
int64 skipBytes = endpos -_file->pos();
if (abs(skipBytes) > 2)
return false;
Copy link
Member

@Die4Ever Die4Ever Sep 17, 2022

Choose a reason for hiding this comment

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

any idea if skipBytes is ever 0 or 1 here? below we have

		if (skipBytes != 2) {
			warning("Groovie::ROQ: Skipped %d bytes", skipBytes);
		}

maybe we should move the warning up here and return false whenever skipBytes != 2

Copy link
Contributor Author

@lolovo lolovo Sep 17, 2022

Choose a reason for hiding this comment

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

If I understand it correct skipBytes equal to 0 or 2 in normal case after traverse. It can be done like you propose (move warning up and return false in this case), but I also don't know if there is normal situation when skipBytes can be 1 at least. I've finished the game with this fix and there weren't any issues with this check/return that lead to crash (may be this case was only one). The crash issue in the begining of pyramid puzzle in RU localization (save file is provided here) due to end assertion fail in seek when after traverse _file->pos() didn't changed, so skipBytes are equal to blockHeader.size.

So, when after traverse our _file->pos() is changed we need skip rest bytes (_skipBytes) here by this logic to make it equal to end otherwise we need skip bytes somehow and avoid assertion failure like in this case with pyramid puzzle. I've just realized that may be appropriate check on wrong situation with skipBytes would be the following (it will be more correct in this case to detect this situation when traverse didn't change _file->pos()):

...
	int64 skipBytes = endpos -_file->pos();
	if (skipBytes > 0) {
		if (skipBytes == blockHeader.size)
			return false;
		_file->skip(skipBytes);
		if (skipBytes != 2) {
			warning("Groovie::ROQ: Skipped %d bytes", skipBytes);
		}
	}
...

I can make this change if you approve it.

As to warning when skipBytes != 2, may be it can be deleted after some time due to useless if there won't be detected other skipBytes value which would affect on something after skip :).

P.S. I'm just thinking that in case of we just check skipBytes != 2 and return without checking _file->pos() is changed during traverse or not will produce incorrect next block processing and may be will lead to crash on end assertion fail in case of unknown block type.

Copy link
Member

@Die4Ever Die4Ever Sep 21, 2022

Choose a reason for hiding this comment

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

maybe instead of if (skipBytes == blockHeader.size) it should be >= ?

Copy link
Contributor Author

@lolovo lolovo Sep 21, 2022

Choose a reason for hiding this comment

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

@Die4Ever For this particular case (puzzle issue - when it is equal) >= will work fine, but I don't know if it will crash on end assertion or not after returning if we've got skipBytes > blockHeader.size..... I think it can crash due to we will skip not the rest bytes but pos of block then as skipping offset here:

	if (endpos != _file->pos()) {
		warning("Groovie::ROQ: BLOCK %04x Should have ended at %d, and has ended at %d", blockHeader.type, endpos, (int)_file->pos());
		warning("Ensure you've copied the files correctly according to the wiki.");
		_file->seek(MIN(_file->pos(), endpos));
	}

So, the only way to be sure we are not failed in this when _file->pos() didn't changed (i.e. skipBytes == blockHeader.size). You can correct me if I'm wrong.

Btw, I've realized now during debugging this issue once again, that after traverse block code we've got _file->eos() == 1 (it is ended), but skipBytes > 0.... so may be the final version of checking will be this, but I'm not sure if it's correct way to check end of stream in this case:

...
	if (skipBytes > 0) {
		if (_file->eos()) 
			return false;
		_file->skip(skipBytes);
		if (skipBytes != 2)
			warning("Groovie::ROQ: Skipped %d bytes", skipBytes);
	}
...

What do you think?

UPD: I've checked first issue with MIN() as solution... so there is also _file->eos() == 1. So, maybe the whole fix of this should be checking eos() and returning before seek/skip. For example, this works fine (so, we need check eos before _file->skip(skipBytes); and _file->seek(MIN(_file->pos(), endpos));):

...
	case 0x1011: // Quad vector quantised video frame
		ok = processBlockQuadVector(blockHeader);
		_dirty = true;
		endframe = true;
		debugC(3, kDebugVideo, "Groovie::ROQ:   Decoded Quad Vector frame.");
		break;
...
	if (endpos != _file->pos()) {
		if (_file->eos())
			return false;
		warning("Groovie::ROQ: BLOCK %04x Should have ended at %d, and has ended at %d", blockHeader.type, endpos, (int)_file->pos());
		warning("Ensure you've copied the files correctly according to the wiki.");
		_file->seek(MIN(_file->pos(), endpos));
	}

Copy link
Member

@Die4Ever Die4Ever Sep 23, 2022

Choose a reason for hiding this comment

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

That last one seems good

I do wonder if the file object has a way to check if a seek is gonna work or a non-crashing seek function

edit: I don't see a function for it

Copy link
Contributor Author

@lolovo lolovo Sep 23, 2022

Choose a reason for hiding this comment

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

OK. I've made last commit. This should works fine.

@Die4Ever
Copy link
Member

Die4Ever commented Sep 25, 2022

I'm gonna test this with all 4 ROQ games

do you use Discord? we have a fan club for these games and we also have a channel for dev chat, if you're interested https://discord.gg/Hss5cJg

@Die4Ever Die4Ever merged commit 835f415 into scummvm:master Sep 25, 2022
8 checks passed
tag2015 pushed a commit to tag2015/scummvm that referenced this pull request Sep 25, 2022
…in roq.cpp (scummvm#4257)

* GROOVIE: Fix scummvm#2 possible crash with incorrect seeks to endpos in roq.cpp
Fix bug - https://bugs.scummvm.org/ticket/13809

* Optimization

* Next optimization & maintan appropriate debug log

* GROOVIE: Check end of stream before seek to avoid crash in roq.cpp

* GROOVIE: resolve %ld conflict

Co-authored-by: Die4Ever <30947252+Die4Ever@users.noreply.github.com>
einstein95 pushed a commit to einstein95/scummvm that referenced this pull request Sep 30, 2022
…in roq.cpp (scummvm#4257)

* GROOVIE: Fix scummvm#2 possible crash with incorrect seeks to endpos in roq.cpp
Fix bug - https://bugs.scummvm.org/ticket/13809

* Optimization

* Next optimization & maintan appropriate debug log

* GROOVIE: Check end of stream before seek to avoid crash in roq.cpp

* GROOVIE: resolve %ld conflict

Co-authored-by: Die4Ever <30947252+Die4Ever@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants