[Fixed] damagecount overflow crash

Bugs that have been investigated and resolved somehow.

Moderator: GZDoom Developers

damagecount overflow crash

Postby Marisa Kirisame » 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 allExpand view
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;
}
User avatar
Marisa Kirisame
ZScript Crimester
 
 
 
Joined: 08 Feb 2008
Location: Vigo, Galicia
Discord: 霧雨魔理沙#1666
Twitch ID: magusmarisa
Github ID: OrdinaryMagician
Operating System: Other Linux 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

Postby 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.
User avatar
drfrag
Os voy a romper a pedazos!
Vintage GZDoom Developer
 
Joined: 23 Apr 2004
Location: Spain
Discord: drfrag#3555
Github ID: drfrag666

Re: damagecount overflow crash

Postby Marisa Kirisame » 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.
User avatar
Marisa Kirisame
ZScript Crimester
 
 
 
Joined: 08 Feb 2008
Location: Vigo, Galicia
Discord: 霧雨魔理沙#1666
Twitch ID: magusmarisa
Github ID: OrdinaryMagician
Operating System: Other Linux 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

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

User avatar
drfrag
Os voy a romper a pedazos!
Vintage GZDoom Developer
 
Joined: 23 Apr 2004
Location: Spain
Discord: drfrag#3555
Github ID: drfrag666

Re: damagecount overflow crash

Postby 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.
User avatar
Rachael
Webmaster
 
Joined: 13 Jan 2004
Discord: Rachael#3767
Twitch ID: madamerachelle
Github ID: madame-rachelle
Operating System: Windows 10/8.1/8/201x 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

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

User avatar
Rachael
Webmaster
 
Joined: 13 Jan 2004
Discord: Rachael#3767
Twitch ID: madamerachelle
Github ID: madame-rachelle
Operating System: Windows 10/8.1/8/201x 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

Postby 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
User avatar
drfrag
Os voy a romper a pedazos!
Vintage GZDoom Developer
 
Joined: 23 Apr 2004
Location: Spain
Discord: drfrag#3555
Github ID: drfrag666

Re: damagecount overflow crash

Postby Marisa Kirisame » 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.
User avatar
Marisa Kirisame
ZScript Crimester
 
 
 
Joined: 08 Feb 2008
Location: Vigo, Galicia
Discord: 霧雨魔理沙#1666
Twitch ID: magusmarisa
Github ID: OrdinaryMagician
Operating System: Other Linux 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

Postby 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.
User avatar
drfrag
Os voy a romper a pedazos!
Vintage GZDoom Developer
 
Joined: 23 Apr 2004
Location: Spain
Discord: drfrag#3555
Github ID: drfrag666

Re: damagecount overflow crash

Postby 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.
User avatar
Rachael
Webmaster
 
Joined: 13 Jan 2004
Discord: Rachael#3767
Twitch ID: madamerachelle
Github ID: madame-rachelle
Operating System: Windows 10/8.1/8/201x 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

Postby 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?
User avatar
drfrag
Os voy a romper a pedazos!
Vintage GZDoom Developer
 
Joined: 23 Apr 2004
Location: Spain
Discord: drfrag#3555
Github ID: drfrag666

Re: damagecount overflow crash

Postby 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 allExpand view
Symbols.AddSymbol(Create<PSymbolConstNumeric>(NAME_Max, this, (1u << ((8 * size) - 1))));

The correct expression would be
Code: Select allExpand view
Symbols.AddSymbol(Create<PSymbolConstNumeric>(NAME_Max, this, (uint32_t) ((((uint64_t) 1u) << (8 * size)) - 1)));
User avatar
phantombeta
In the meadow of sinful thoughts, every flower's a perfect one
 
Joined: 02 May 2013
Location: Brazil, South America, Earth, Orion-Cygnus Arm, Milky Way
Discord: phantombeta#2461
Twitch ID: phantombeta_
Github ID: Doom2fan
Operating System: Windows 10/8.1/8/201x 64-bit
OS Test Version: No (Using Stable Public Version)
Graphics Processor: nVidia with Vulkan support

Re: damagecount overflow crash

Postby 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.
User avatar
drfrag
Os voy a romper a pedazos!
Vintage GZDoom Developer
 
Joined: 23 Apr 2004
Location: Spain
Discord: drfrag#3555
Github ID: drfrag666


Return to Closed Bugs

Who is online

Users browsing this forum: No registered users and 2 guests