Skip to content

Commit

Permalink
SCI: Add workaround for uninit read during wordsearch in castlebrain
Browse files Browse the repository at this point in the history 
During the wordsearch puzzle (room 320 click left door) the
game will crash because of an uninitalized read of temp
variables in word::dispatchEvent (which gets called a lot),
if the player clicks the same letter or different letters
aggressively or holds down the enter key.

Fixes Trac#9783.
  • Loading branch information
@dafioram @csnover
dafioram authored and csnover committed Oct 7, 2017
1 parent 45d1177 commit ec1cfcb
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions engines/sci/engine/workarounds.cpp
View file
Expand Up @@ -273,6 +273,7 @@ static const uint16 sig_uninitread_sq1_1[] = {
const SciWorkaroundEntry uninitializedReadWorkarounds[] = {
{ GID_CAMELOT, 40, 40, 0, "Rm40", "handleEvent", NULL, 0, { WORKAROUND_FAKE, 0 } }, // when looking at the ground at the pool of Siloam - bug #6401
{ GID_CASTLEBRAIN, 280, 280, 0, "programmer", "dispatchEvent", NULL, 0, { WORKAROUND_FAKE, 0xf } }, // pressing 'q' on the computer screen in the robot room, and closing the help dialog that pops up (bug #5143). Moves the cursor to the view with the ID returned (in this case, the robot hand)
{ GID_CASTLEBRAIN, 320, 325, 0, "word", "dispatchEvent", NULL, -1, { WORKAROUND_FAKE, 0 } }, // holding down enter key during the word search puzzle, temp 14 and 15 - bug #9783

This comment has been minimized.

Sign in to view

Copy link
@m-kiewitz

m-kiewitz Oct 7, 2017

Contributor

Snover, why do you accept this, although my review was not done and my questions were also not answered?
I will now have to revert this, check the issue and make a proper patch.
Simply going for all temps without checking what's actually happening internally is not the way to go.

This comment has been minimized.

Sign in to view

Copy link
@csnover

csnover Oct 7, 2017 •
edited

Member

What review are you talking about? There is no comment from you in the ticket or in the pull request that I can see.

This comment has been minimized.

Sign in to view

Copy link
@m-kiewitz

m-kiewitz Oct 7, 2017 •
edited

Contributor

???
#1032
m-kiewitz started a review
and then there is also my comment right under the added entry:
"For just 2 variables, there should simply be 2 entries." ...

This comment has been minimized.

Sign in to view

Copy link
@wjp

wjp Oct 7, 2017

Contributor

That is not something we can see until you finish it.

This comment has been minimized.

Sign in to view

Copy link
@m-kiewitz

m-kiewitz Oct 7, 2017

Contributor

Argh @csnover
crappy design on github's part. https://i.imgur.com/kEOD5xZ.png

This comment has been minimized.

Sign in to view

Copy link
@m-kiewitz

m-kiewitz Oct 7, 2017 •
edited

Contributor

And I mean they could at the very least say somewhere that I started a review.
I also thought "pending" was the status of the review, not that everything is completely hidden. Like some sort of status "it's pending here, not yet okay'd". What the hell.
How is this even supposed to work out? Let's say there is a huge pull request like my AGI rewrite. So someone starts a review, no one sees it until it's submitted. How is that someone even supposed to get answers to a question?
This is so terrible design, I can't believe it.

This comment has been minimized.

Sign in to view

Copy link
@csnover

csnover Oct 7, 2017

Member

GitHub’s UI design could certainly be made clearer.

How is this even supposed to work out? Let's say there is a huge pull request like my AGI rewrite. So someone starts a review, no one sees it until it's submitted. How is that someone even supposed to get answers to a question?

The review feature allows you to review an entire change set without having to do it all in one sitting, and without concern over sending review comments that are incomplete/nonsense because they’re based on an incomplete understanding of the whole change set (because you had not yet read the entire change set). It is especially relevant for such huge pull requests, which often require more than one pass through to be able to review the code changes properly.

As discovered here, the one big problem with this design is that you must know that you need to submit the review once you are finished with it in order for it to be visible, which was not known in this case. Now that it is known that this is how it works, everyone will be able to see all of the reviews once they are finished in the future. :)

This comment has been minimized.

Sign in to view

Copy link
@m-kiewitz

m-kiewitz Oct 7, 2017

Contributor

Sure, but that's still stupid design.
I mean let's say you and I are both doing a review. You won't know about mine until it's submitted. I won't know about yours until it's submitted.
Now you are done, submit it and accept it afterwards. Mine was not done, and there was no way for you to know about it.

Why can't they at the very least state somewhere that X and Y are currently reviewing and have not giving okay?
It would be trivial to implement.
{ GID_CNICK_KQ, -1, 0, 1, "Character", "say", NULL, -1, { WORKAROUND_FAKE, 0 } }, // checkers/backgammon, like in hoyle 3 - temps 504 and 505 - bug #6255
{ GID_CNICK_KQ, -1, 700, 0, "gcWindow", "open", NULL, -1, { WORKAROUND_FAKE, 0 } }, // when entering the control menu, like in hoyle 3
{ GID_CNICK_KQ, 300, 303, 0, "theDoubleCube", "<noname520>", NULL, 5, { WORKAROUND_FAKE, 0 } }, // while playing backgammon with doubling enabled - bug #6426 (same as the theDoubleCube::make workaround for Hoyle 3)
Expand Down

0 comments on commit ec1cfcb

Please sign in to comment.