damagecount overflow crash

Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.

Post a reply

Smilies
:D :) :( :o :shock: :? 8-) :lol: :x :P :oops: :cry: :evil: :twisted: :roll: :wink: :geek: :ugeek: :!: :?: :idea: :arrow: :| :mrgreen: :3: :wub: >:( :blergh:
View more smilies

BBCode is OFF
Smilies are ON

Topic review
   

Expand view Topic review: damagecount overflow crash

Re: damagecount overflow crash

by drfrag » Wed Jan 06, 2021 9:05 am

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.

Re: damagecount overflow crash

by phantombeta » Wed Jan 06, 2021 5:09 am

drfrag wrote: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)));

Re: damagecount overflow crash

by drfrag » Wed Jan 06, 2021 4:33 am

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?

Re: damagecount overflow crash

by Rachael » Tue Jan 05, 2021 5:59 pm

drfrag wrote: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.

Re: damagecount overflow crash

by drfrag » Tue Jan 05, 2021 12:32 pm

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.

Re: damagecount overflow crash

by Marisa the Magician » Tue Jan 05, 2021 11:55 am

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

Re: damagecount overflow crash

by drfrag » Tue Jan 05, 2021 10:30 am

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

Re: damagecount overflow crash

by Rachael » Tue Jan 05, 2021 10:10 am

Re: damagecount overflow crash

by Rachael » Tue Jan 05, 2021 10:04 am

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.

Re: damagecount overflow crash

by drfrag » Tue Jan 05, 2021 9:22 am

Re: damagecount overflow crash

by Marisa the Magician » Tue Jan 05, 2021 4:43 am

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.

Re: damagecount overflow crash

by drfrag » Tue Jan 05, 2021 4:14 am

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.

damagecount overflow crash

by Marisa the Magician » Mon Jan 04, 2021 5:08 pm

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;
}

Top