damagecount overflow crash

[Marisa the Magician]

So recently a certain crash was reported for my mod. And after some digging from me and KeksDose we figured out what the source of this madness is. Turns out that taking a non-zero amount of damage quickly followed by int.max damage will cause the damagecount variable to overflow and go into negative. This causes a crash on Windows, and rapid flashing on other platforms.

A simple actor that will reproduce the crash when summoned is provided here:

Code: Select all

class ThisWillEndYou : Actor {
    states {
    spawn:
        TNT1 A 10 nodelay {
            po = players [0].mo;
            po.DamageMobj(self, self, 200, 'PizzaMozzarella');
        }
        
        TNT1 A 10 {
            po.player.Resurrect();
            po.DamageMobj(self, self, int.max, 'LargeFriesButNotFriedChicken');
        }
        
        stop;
    }
    
    PlayerPawn po;
}

[drfrag]

I think the solution here would be to reset damagecount in Resurrect() to prevent the overflow, the crash itself is easy to prevent.
It crashes in V_AddPlayerBlend since damagecount is negative, i think it also would be a good idea to deal with negative values since DamageMobj can return a negative damage and that function is a bit messy since it has a lot of return statements.

[Marisa the Magician]

I don't think doing it in resurrect will help much. It could also happen if you, say, had 100 health, took one hit for 99 and then one for int.max. Still the same, no need to resurrect.

[drfrag]

Fixed, i think. https://github.com/coelckers/gzdoom/pull/1267

[Rachael]

Even if it isn't a proper fix, that's something that should be done, but I don't like the duplicate code, so I am going to redo that fix a little bit differently.

[Rachael]

fixed

[drfrag]

Yeah i've force pushed my branch without duplicate code: https://github.com/drfrag666/gzdoom/com ... 1ab3f16813

[Marisa the Magician]

Switching it to an uint32_t was a good idea. That extra bit actually guarantees that the overflow won't happen in DamageMobj.

[drfrag]

Are you sure? Won't it still overflow with a bigger int? Technically they don't call it an overflow but they wrap around and you'd get a very small number instead.

[Rachael]

Are you sure? Won't it still overflow with a bigger int?

There's a trick. Yes, it overflows, but it overflows to a bigger number, which satisfies the condition for damagecount >=100. Either way it still gets fixed by that conditional.

[drfrag]

It depends on the number, for instance if you have two calls to DamageMobj one with 1 and another one with uint.max (assuming that's defined) or just one with (uint.max +1) it will overflow to 1 and that's less than 100. It won't crash since it's positive but you'll die and the screen won't turn red. So rather than preventing the overflow you're dealing with it, something you could have done in v_blend.cpp anyway. Besides the code was written for ints and changing the types to unsigned regular ints will be promoted to unsigned in operations too. Have you reviewed all the code to ensure that there won't be any issues? I've also heard it's not a good thing to abuse unsigned ints.
But this is a very minor thing, int.max was a crazy value for damage.
One more thing, the overflow comes from "player->damagecount += damage;" where does it come from in the case of bonuscount? Have you managed to get a crash also in that case?

[phantombeta]

It depends on the number, for instance if you have two calls to DamageMobj one with 1 and another one with uint.max (assuming that's defined) or just one with (uint.max +1) it will overflow to 1 and that's less than 100. It won't crash since it's positive but you'll die and the screen won't turn red. So rather than preventing the overflow you're dealing with it, something you could have done in v_blend.cpp anyway. Besides the code was written for ints and changing the types to unsigned regular ints will be promoted to unsigned in operations too. Have you reviewed all the code to ensure that there won't be any issues? I've also heard it's not a good thing to abuse unsigned ints.
But this is a very minor thing, int.max was a crazy value for damage.
One more thing, the overflow comes from "player->damagecount += damage;" where does it come from in the case of bonuscount? Have you managed to get a crash also in that case?

DamageMobj takes a signed int for damage, so passing in uint.max wouldn't work. It'd get cast down to a signed int, and as such, would be -1*. Health is also a signed int, so it'd underflow even if you could pass in uint.max.

* Except, turns out... Max values for unsigned ints are wrong. GZDoom is setting them to the MSB, not the highest value they can hold. Because of this line:

Code: Select all

Symbols.AddSymbol(Create<PSymbolConstNumeric>(NAME_Max, this, (1u << ((8 * size) - 1))));

The correct expression would be

Code: Select all

Symbols.AddSymbol(Create<PSymbolConstNumeric>(NAME_Max, this, (uint32_t) ((((uint64_t) 1u) << (8 * size)) - 1)));

[drfrag]

Yeah, it was more complicated than that. And the -1 would be converted to 0 in DamageMobj so you'd take no damage.
That's why it didn't crash with int.max and you needed two calls.