[Bug-freedink] Bug#688934: Bug#688934: Another way to trigger this bug

[Bas Wijnen]

Hi,

On Sun, Oct 14, 2012 at 12:23:45AM +0200, Sylvain wrote:
> Hi Bas,
> 
> Thanks a great lot for all your effort on this nasty bug, despite my
> lack of responsiveness.

No problem, bug fixing is fun. :-)

> I took a few hours to investigate what exactly happens (adding a few
> code comments in the process), and I see that in your testcase dmod,
> the warp is an invisible sprite which is merged in the background and
> thus never found by find_sprite().

Yes, that is indeed essential to trigger the bug.

> If spr[0] is clean, the warp continues despite not finding the warp
> sprite; if spr[0] is modified, then we hit the bug, as the code
> assumes the warp animation is not finished.

Yes. And because Seth didn't like index 0, the animation of spr[0]
doesn't actually play (he uses for (crap = 1; ...)), so it will never
finish.

> I'm not sure in what way an unclean spr[0] will affect the game, but
> it might, and cleaning it would affect compatibility with the original
> Dink.

I don't think it would. Seth doesn't actually use it. He just skipped it
because he wants to start his indices at 1. However, I don't suggest it
should be cleaned. That would only hide other bugs.

> Consequently I used and documented your original fix from
> http://www.dinknetwork.com/forum.cgi?MID=168476#168476
> 
> http://git.savannah.gnu.org/cgit/freedink.git/commit/?id=fff4b7cb8d6d2bf84482fdc83c2a21fd9d0379e1
> http://git.savannah.gnu.org/cgit/freedink.git/commit/?id=402bf38f69cd7e5c47322b8087ba535d6f823283
> http://git.savannah.gnu.org/cgit/freedink.git/commit/?id=5d692b447eb7a8b3c0f1128390641992a4dff484
> 
> WDYT?

Renaming "prop" to "is_warp" is a very good idea; it makes it all much
better understandable. :-)

As for the bug, it really is several bugs. You have fixed the warp
problem by ignoring memory corruption, which is good. Still, I would
suggest that the memory should not get corrupted in the first place.
That is, if the engine tries to create a sprite, but there are no free
slots for it, it should also detect this and refuse to write into
spr[0]. I have found two places where the engine creates sprites
(add_random_blood and the creation of flying duck heads), and that may
be all, but I didn't check. In both cases, there is no check if the
sprite creation actually worked, and there should be.

> (Btw, I don't have the same line numbers in my source files, possibly
> you added test code around? ;))

I don't think I did, but I may be using an old copy.

Thanks,
Bas

signature.asc
Description: Digital signature