EXC_BAD_ACCESS in bitmap::AlphaPriorityBlit about ivan HOT 8 CLOSED

I ran this with AddressSanitizer and found something:

This might have something to do with this line. For humanoids, GetDrawDisplacement simply returns a v2(0, 0), but for kamikaze dwarves it has been overridden to return something more interesting, e.g. v2(0, -1), which actually causes an out-of-bounds access here (maybe elsewhere too) because B.Dest.Y is -1.

There's some kind of validation block earlier in AlphaPriorityBlit which is run if FastFlag is not set. If I make it unconditional then femath::Clip fixes the invalid B.Dest.Y value and the crash seems to go away. But that block clearly shouldn't be unconditional so maybe FastFlag just needs to be unset when drawing kamikaze dwarf bodyparts?

from ivan.

I could reproduce this error, but it took maybe >10 dwarfs. I wonder why GetDrawDisplacement is necessary at all? I suppose it is there to make dwarfs look like shorties, because the GetDrawDisplacement array for dwarfs looks like body part indicies, and dwarf's legs don't touch the bottom of their bounding box.
Dwarf bodyparts don't need this property when they are detached from the torso... Does the crash happen when a dwarf explodes?

from ivan.

Actually, try to use the 'go' command so that the explosion happens while you're 'go'ing. That way it seems to happen about 50% of the time.

And yes the crash seems to happen only when they explode. But my guess is that the undefined behavior starts earlier (here as reported by AddressSanitizer), and for some reason only triggers the crash when drawing the flying bodyparts.

from ivan.

I am becoming suspicious of burning bodyparts. Does the problem go away if in material.dat we set DWARF_FLESH interaction flags to: InteractionFlags = Base; instead of: InteractionFlags = Base|CAN_BURN; ?

This would prevent them from burning and therefore trying to animate flames.

from ivan.

Hmm, removing CAN_BURN still doesn't prevent the out-of-bounds access at line 1812. Maybe if we fix that then the crash will go away as well.

from ivan.

It seems that femath::Clip should be called on the blitdata whenever it would otherwise draw outside the actual bitmap. This seems to be the case whenever GetDrawDisplacement returns something else than v2(0, 0).

One way to make the problem go away would be to check BlitData.Dest in bodypart::Draw and if it's non-zero then deactivate FastFlag before calling the blit function, so that inside the blit function femath::Clip is called. F.ex. something like this:

if(BlitData.Dest.X < 0 || BlitData.Dest.Y < 0)
  P->DeactivateFastFlag();

P->AlphaPriorityBlit(BlitData);

P->ActivateFastFlag();

But this is probably not how (De)activateFastFlag should be used, and also if FastFlag was already deactivated before that operation then we wouldn't want activate it in the end. But there's no GetFastFlagState or similar so again, it shouldn't probably be used that way.

Any better ideas?

from ivan.

Sorry for the hiatus on this, I went away for the weekend and forgot to get back to it.

What if we work higher up in the chain and make it conditional on a kamikaze dwarf that he has to be in one piece to have these displacements. So that dismembered bodyparts always return ZERO_V2.
Trouble is at present GetDrawDisplacement() seems to work at the character level and not bodypart level.

I don't really understand the way this array corresponds to the drawn bodyparts. If I summon a kamikaze dwarf in cheat mode New Attnam, I can see his right arm (on the left) is moved up slightly, and both legs don't touch the bottom of the bounding box, and it looks like his groin is transparent because I can see grass on the other side. Either that or he's wearing a grass skirt.

I suggest we turf the special parts of the code that mangle the kamikaze dwarf bitmaps and just edit the humanoid.pcx to get the same result. I looked through char.dat and the leg and arm bodyparts are all unique to the dwarf. Check out #66.

from ivan.

NP! Yeah I have noticed that transparent groin too, it's probably a bug.

from ivan.