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

GLK: ADVSYS: Fix "crash" when matching with empty nouns array #6173

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

antoniou79
Copy link
Contributor

This fixes the case for "Starship Columbus" IF game

This game seems to call VM::opMATCH() multiple times per line, and very often would cause an assertion fault for out of bounds access to an array (_nouns). Inputs like "open", "inventory","help" would trigger the assertion fault. Debugging shows that in those cases, the idx var in opMATCH would be "-1" and also the _nouns array would be empty. I've added a check for either in an if clause that essentially fails the match and prevents the out of bounds array access attempt.

The issue was first reported on the forums here: https://forums.scummvm.org/viewtopic.php?p=99929&sid=1d010aa5065115367a5dc3a2c4236434#p99929

This fixes the case for "Starship Columbus" IF game

This game seems to call VM::opMATCH() multiple times per line, and very often would cause an assertion fault for out of bounds access to an array (_nouns). Inputs like "open", "inventory","help" would trigger the assertion fault. Debugging shows that in those cases, the idx var in opMATCH would be "-1" and also the _nouns array would be empty. I've added a check for either in an if clause that essentially fails the match and prevents the out of bounds array access attempt.

The issue was first reported on the forums here: https://forums.scummvm.org/viewtopic.php?p=99929&sid=1d010aa5065115367a5dc3a2c4236434#p99929
@bluegr
Copy link
Member

bluegr commented Oct 2, 2024

It's quite odd that the game is exhibiting this bug - that part is the same as the original advsys code:
https://github.com/dbetz/advsys/blob/a62bd563b8f66f4b93e35fc35bdc4aabaa1fd4b1/advexe.c#L384

So, perhaps there's another underlying issue here, with out implementation?
How is the original working in this case?

In any case, a sanity check like this is fine, but we need to figure out if there's an issue with our implementation

@bluegr bluegr merged commit af68eb0 into scummvm:master Oct 2, 2024
8 checks passed
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