[fix][8967] Division by zero in Unit::CalcAbsorbResist

[Sarjuuk]

What bug does the patch fix?

Units with this aura being oneshotted by magic damage

(yes this one is actually used by SD2 (or at least should be^^))

For which repository revision was the patch created?

r8958

Is there a thread in the bug report section or at lighthouse?

was reported via IRC

Who has been writing this patch?

me

diff --git a/src/game/Unit.cpp b/src/game/Unit.cpp
index 70837d2..7817993 100644
--- a/src/game/Unit.cpp
+++ b/src/game/Unit.cpp
@@ -1995,12 +1995,18 @@ void Unit::CalcAbsorbResist(Unit *pVictim,SpellSchoolMask schoolMask, DamageEffe
            currentAbsorb = RemainingDamage;

        float manaMultiplier = (*i)->GetSpellProto()->EffectMultipleValue[(*i)->GetEffIndex()];
-        if(Player *modOwner = pVictim->GetSpellModOwner())
-            modOwner->ApplySpellMod((*i)->GetId(), SPELLMOD_MULTIPLE_VALUE, manaMultiplier);
+        if(manaMultiplier)
+        {
+            if(Player *modOwner = pVictim->GetSpellModOwner())
+                modOwner->ApplySpellMod((*i)->GetId(), SPELLMOD_MULTIPLE_VALUE, manaMultiplier);
+
+            int32 maxAbsorb = int32(pVictim->GetPower(POWER_MANA) / manaMultiplier);
+            if (currentAbsorb > maxAbsorb)
+                currentAbsorb = maxAbsorb;

-        int32 maxAbsorb = int32(pVictim->GetPower(POWER_MANA) / manaMultiplier);
-        if (currentAbsorb > maxAbsorb)
-            currentAbsorb = maxAbsorb;
+            int32 manaReduction = int32(currentAbsorb * manaMultiplier);
+            pVictim->ApplyPowerMod(POWER_MANA, manaReduction, false);
+        }

        (*i)->GetModifier()->m_amount -= currentAbsorb;
        if((*i)->GetModifier()->m_amount <= 0)
@@ -2009,9 +2015,6 @@ void Unit::CalcAbsorbResist(Unit *pVictim,SpellSchoolMask schoolMask, DamageEffe
            next = vManaShield.begin();
        }

-        int32 manaReduction = int32(currentAbsorb * manaMultiplier);
-        pVictim->ApplyPowerMod(POWER_MANA, manaReduction, false);
-
        RemainingDamage -= currentAbsorb;
    }

ok, that implies, that an EffectMultipleValue of zero means, that no mana should be drained... hmm

/e: on second thought. Maybe it should be checked if the Unit has POWER_MANA..?

[balrok]

for clarification:

int(1/float(0.0)) will be -2147483648

(haven't knewn this before)

[XTZGZoReX]

Uhm, last I checked the C++ standard, it stated that division by zero throws an FPE...

[Sarjuuk]

I don't know why it does not here either...

In any case, EffectMultipleValue can be zero and this case has to be caught somehow or unfortunate things will happen :]

[Lynx3d]

A float division by zero is perfectly legal and results in +inf or -inf depending on signs obviously.

Only 0.0/0.0 is treated differently and results in NaN (not a number). That still doesn't mean you program throws an exception though, to my knowledge/experience it will produce a quiet NaN that "infests" everything that touches it.

Apparently, casting inf to integer results in the smallest/largest representable value, but i don't know what the standard exactly says about this.

Kinda hard to google this specific case...and i think it doesn't really matter in this case anyway.

[False.Genesis]

afaik it depeds on the compiler setting.. at least VC71 - VC90 have ths option where you can select whether floating point exceptions should be used or not. if not, it sets inf/NaN silently.

i remember this beeing an issue when players could not be saved because the save query contained NaN, which made mysql reject the query.

guess why there is FiniteAlways() littered everywhere in Player::SaveToDB()

so it looks like these exceptions were actually never enabled at mangos.

[Lynx3d]

Hm okay after reading some more, yes the IEEE standard defines five exceptions for floating point operations, but at least on x86 Linux and Windows by default they are not trapped, the CPU recovers and sets appropriate exception flags. Most operations have a defined result anyway (unlike an integer division by zero for example). Each compiler/platform has its own options and built-ins to change this (if any).

The behaviour of casting INF to integer is in fact undefined from what i could find out, but it "only" causes the invalid exception (like operations that result in NaN).

But the same happens when the float value is simply too large for the integer type.

As we now know, x86 hardware chooses the largest magnitude integer...

In any case, these are conditions you should check in code IMHO, because depending on the specific case, accepting exceptional values may be desired or not....and at least C99 added function to check if a floating point exception occured, so no need to force trap, emit a signal and possibly invoke expensive exception handling.

E.g. enabling trapping might cause the vmap code interrupt the core ever so often, but may be written in a way to handle the occurence of INF and NaN just fine. At least my raytracing code explicitly handled such situations in several places...

[XTZGZoReX]

Come to think of it, when I tested this, I divided by 0 (not 0.0) -- this might make a difference.

[balrok]

yes i've tested with

1/0 = exception

1/0.0 = -infininite

was on linux a c-program with g++ compiled

quite tricky those floats.. i've also thought until now that this would throw an exception..

[darkstalker]

1/0.0 = -infininite

floating point numbers have positive and negative zero

[VladimirMangos]

In [8967]. Thank you