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

SCI32: Fix QFG4 copy protection crash, bug #10773 #1431

Merged
merged 1 commit into from Dec 3, 2018

Conversation

Projects
None yet
3 participants
@sluicebox
Contributor

sluicebox commented Dec 3, 2018

No description provided.

@digitall digitall requested review from bluegr and m-kiewitz Dec 3, 2018

@bluegr

bluegr approved these changes Dec 3, 2018

@bluegr

This comment has been minimized.

Member

bluegr commented Dec 3, 2018

Nice work, thanks! Merging

@bluegr bluegr merged commit b9c6055 into scummvm:master Dec 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -8217,6 +8217,46 @@ static const uint16 qfg4ConditionalVoidPatch[] = {
PATCH_END
};
// The copy protection in floppy versions has a script bug which uses disposed
// objects and crashes our interpreter. This appears to work in Sierra's
// interpreter although they fixed the script bug in the CD version.

This comment has been minimized.

@m-kiewitz

m-kiewitz Dec 3, 2018

Contributor

This part of the comment should get changed.

The Sierra interpreter used straight offsets directly to the data and allowed further access, which sometimes caused severe corruption.
We use a different approach. When references to objects or other data are free'd, they will cause an error when accessing that data reference afterwards (which is of course way better, that way we can detect script bugs).

This here is definitely a script bug and the original interpreter simply ignored the issue (as many others too).

This comment has been minimized.

@sluicebox

sluicebox Dec 3, 2018

Contributor

What would you prefer? I don't see what's inaccurate about that high level description. Sierra fixed this script bug by adding logic to not dispose these objects so that they wouldn't use disposed objects, so I don't know how this is anything other than a use-after-free class of bug that appears to work in the original.

Is your concern that the comment implies that it's a bug to re-use any object after disposal? I get that that's okay, but in this case that's specifically the bug: disposing sets View:plane to 0 so using that View it leads to kAddScreenItem erroring. If "uses" were changed to "displays" would that be better?

This comment has been minimized.

@m-kiewitz

m-kiewitz Dec 3, 2018

Contributor

What would you prefer? I don't see what's inaccurate about that high level description.

Well, I personally don't like it when something says "crashes". For me "crashing" is an actual crash as in the code fully breaking, and not an error detection message. I don't think our interpreter actually crashes, but detects an error and gives out a detailed error message (correct me, if I'm wrong here, I haven't tested this one out personally. If it actually crashes that should get fixed, the interpreter should never really crash, no exception should ever occur).
And it should also mention that it works in the original interpreter because Sierra allowed accessing already disposed objects to some degree and we don't.

Is your concern that the comment implies that it's a bug to re-use any object after disposal?
No, that's exactly right.
It is of course a script bug, when its trying to access an already disposed object. We already had several of those.

My concern is that it sounds like our interpreter would actually crash (as in literally crash, not end the game and showing an error message) and that it somehow works in the original interpreter (which implies that there is something wrong in ours, and not the other way).

At some point someone may read this comment and go "wait, is there some bug inside this interpreter? Why did it crash in this one while it worked fine in Sierras? Let's look into this and waste time).

We check and detect tons of things that Sierra did not check or care for. That's why some games had some random bugs happening at some random point in the game, which don't (shouldn't) happen in our interpreter at least after all of these script bugs are fixed/worked around.

This comment has been minimized.

@sluicebox

sluicebox Dec 4, 2018

Contributor

Gotcha, makes sense, and I agree. I went back and forth on that word. Updated description in #1433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment