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

Add safety measure to Scene_CommandObjectList to prevent crash #3904

Merged

Conversation

inspectredc
Copy link
Contributor

@inspectredc inspectredc commented Feb 4, 2024

On debug versions it was reported that going between certain rooms in a scene would cause a crash. I gave this branch for someone to test and it seems to fix the issue. I believe what was happening was just that cmdObj->objects[k] was indexing past the size of objects, however, I was never able to replicate the original crash on my machine

Build Artifacts

Copy link
Contributor

@Archez Archez left a comment

Choose a reason for hiding this comment

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

Comparing this against the original code with the while loops, I think this is missing something.

s16* objectEntry = SEGMENTED_TO_VIRTUAL(cmd->objectList.segment);
void* nextPtr;
k = 0;
i = play->objectCtx.unk_09;
firstStatus = &play->objectCtx.status[0];
status = &play->objectCtx.status[i];
while (i < play->objectCtx.num) {
if (status->id != *objectEntry) {
status2 = &play->objectCtx.status[i];
for (j = i; j < play->objectCtx.num; j++) {
status2->id = OBJECT_INVALID;
status2++;
}
play->objectCtx.num = i;
func_80031A28(play, &play->actorCtx);
continue;
}
i++;
k++;
objectEntry++;
status++;
}

From here I interpret that the if (status->id != *objectEntry) handles when play->objectCtx.num is larger than the amount of objects to be loaded by the command. Then it basically invalidates all the extra objects and calls the actor kill all func.

With how the for loop is written/with your change, it seems like the invalidate/actor kill all wont run if cmdObj->objects.size() is smaller than play->objectCtx.num.

I think we need to re-work this for loop to handle that condition. I've provided a suggestion below that I think works.

soh/soh/z_scene_otr.cpp Outdated Show resolved Hide resolved
Co-authored-by: Archez <Archez@users.noreply.github.com>
@inspectredc
Copy link
Contributor Author

Thanks for the suggestion! Definitely makes sense that way and matches the behaviour I expect from the function. I hadn't thought about needing to clear the remaining slots in the bank to match the original behaviour, so this is good, thank you!

@garrettjoecox garrettjoecox merged commit 107a365 into HarbourMasters:develop-macready Feb 16, 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.

None yet

3 participants