New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wesnoth crash when units have negative hitpoints: overflow in ai attack calculations #3042

Closed
ePaul opened this Issue May 6, 2018 · 18 comments

Comments

Projects
None yet
6 participants
@ePaul
Contributor

ePaul commented May 6, 2018

I'm running v1.14.0+dev (43f30cd-Clean) (self-compiled), with the UMC "A troop in a war". Somehow I got enemy units with negative hitpoints (which is likely a bug in that addon), but then Wesnoth simply crashes without any indication why.

Reproduction

  • (possibly: install "A troop in a war" 0.1.6d, not sure if this is needed for reproducing.)
  • Load this savegame AG-Skirmish Runde 3-c.gz
  • Click "End turn".
    → Wesnoth crashes (window disappears, exit code 134).

The following is printed on the console (stdout/stderr):

Battle for Wesnoth v1.14.0+dev (43f30cdd613-Clean)
Started on Sun May  6 12:43:29 2018

Automatically found a possible data directory at /home/USER/projektoj/wesnoth

Data directory:               /home/USER/projektoj/wesnoth
User configuration directory: /home/USER/.config/wesnoth
User data directory:          /home/USER/.local/share/wesnoth/1.14
Cache directory:              /home/USER/.cache/wesnoth

Setting mode to 1920x1026
Checking lua scripts... ok
20180506 12:43:36 error display: Tile at 16,7 isn't on the map, can't scroll to the tile.
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
[... some thousand more of those lines ...]
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
20180506 12:43:40 warning ai/general: overflow in ai attack calculation
wesnoth: src/attack_prediction.cpp:1764: double {anonymous}::calculate_probability_of_debuff(double, bool, double, double, bool, double): Assertion `initial_prob >= 0.0 && initial_prob <= 1.0' failed.
Abgebrochen (Speicherabzug geschrieben)

(I didn't find the mentioned core dump, not sure where that would be.)

Expected behavior
If WML content creates an invalid state, Wesnoth should show a useful error message, and potentially go back to the main screen, not simply crash.

@ePaul

This comment has been minimized.

Contributor

ePaul commented May 6, 2018

When editing this save-game to not have negative hitpoints, the issue doesn't happen.

@jyrkive

This comment has been minimized.

Member

jyrkive commented May 6, 2018

I'm planning to fix this by disallowing UMC authors from creating units with negative HP.

Do you think that alone would be sufficient, or should I also block players from loading saves which have such units?

@Wedge009 Wedge009 added Bug AI labels May 6, 2018

@ePaul

This comment has been minimized.

Contributor

ePaul commented May 6, 2018

My opinion (which doesn't necessarily count, as I'm not a developer) would be to give a useful error message when trying to load this. Though an alternative would be to just have a graceful fallback when the AI calculation in question here encounters this.

@gfgtdf

This comment has been minimized.

Contributor

gfgtdf commented May 6, 2018

afaik the engine itself also creates units with negative hitpoints during an attack, so disallowing setting umc negative hp woudl result in codes like unit.hitpoints = unit.hitpoints actually changing the hitpoints if it happens during an attack related event where a unit has negative hitpoints, which is very counterintuitive.

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented May 7, 2018

Uhh, why would the engine be creating units with negative hitpoints?

@Vultraz

This comment has been minimized.

Member

Vultraz commented May 7, 2018

I think what he means is units may have negative hitpoints after an attack when events like attack_end fire. Pretty sure I've observed this myself.

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented May 7, 2018

That seems rather counter-intuitive… is there a reason for it?

@gfgtdf

This comment has been minimized.

Contributor

gfgtdf commented May 7, 2018

I think what he means is units may have negative hitpoints after an attack when events like attack_end fire. Pretty sure I've observed this myself.

yes thats what i meant.

That seems rather counter-intuitive… is there a reason for it?

note sure, but umc might rely on it to detemine how much damage has 'overflowed', i have seen umc doing crazy things.

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented May 7, 2018

Yeah, and I could see providing that information to be a valid reason for this unintuitive behaviour. It should definitely be documented on the wiki though if it's not already.

@jyrkive

This comment has been minimized.

Member

jyrkive commented May 9, 2018

All right, debugging shows why exactly negative HP ends up triggering this assertion.

Omae wa mou shindeiru.

In this savegame, the AI simulates a situation where unit A attacks unit B. Unit B has a poisoning attack, but its HP is negative. Meanwhile, unit A is so close to level-up that killing unit B will level up unit A and heal the poison status. Damage calculation takes both the poisoning attack and the upcoming level-up into account when calculating the probability that unit A will be poisoned after the attack.

The problem is that due to the negative HP, damage calculation code thinks that B is already dead before the fight (HP <= 0), but that B may not be dead after the fight (HP == 0). Thus, the probability that A kills B is considered to be negative, and the fact that killing B heals poison is considered to increase the probability that A is poisoned after the fight. Thus, the "A is poisoned" probability creeps to 105 %, which causes the crash if the AI simulates another fight for A after this one.

Next, time to start writing the fix...

jyrkive added a commit that referenced this issue May 9, 2018

Disallow units with negative HP
Damage calculation code can't tolerate presence of such units.

Fixes #3042.

@jyrkive jyrkive closed this in ef60dea May 9, 2018

@gfgtdf

This comment has been minimized.

Contributor

gfgtdf commented May 11, 2018

@jyrkive did you completely ignore the discussion above? You code will break addons even if they don't change the units hitpoints at all.

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented May 11, 2018

How could it break addons?

@gfgtdf

This comment has been minimized.

Contributor

gfgtdf commented May 11, 2018

the engine generates units with negative hp, the umc wml unstores it, then his change breaks to main menu (which is a really strange way to handle invalid umc api parameters in the first place)

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented May 11, 2018

Ah. Yeah, I'd say at least it should just zero the hitpoints if they're found to be negative (or something similar).

jyrkive added a commit that referenced this issue May 14, 2018

Allow modifying dead units in last_breath and die event handlers
Fixes an unintentional API change from commit 3c344e8, discussed in
comments of #3042.

jyrkive added a commit to jyrkive/wesnoth that referenced this issue May 14, 2018

Allow modifying dead units in last_breath and die event handlers
Fixes an unintentional API change from commit 3c344e8, discussed in
comments of wesnoth#3042.
@jyrkive

This comment has been minimized.

Member

jyrkive commented May 14, 2018

@gfgtdf Addressed in commit dc68187.

@gfgtdf

This comment has been minimized.

Contributor

gfgtdf commented May 14, 2018

@jyrkive this also effects atacke/defender_hits/misses etc events though. And still crashing to tiltescreen (VALIDATE) is not the correct way to handle invalid api parmaeters in particular as it afaik gives the umc authot no hint were it happens (ulike lua exeptions)

jyrkive added a commit that referenced this issue May 15, 2018

Throw a Lua exception when creating a negative-HP unit in a Lua context
Instead of throwing a WML error. This allows the UMC author to get a stack
trace if the unit creation was triggered from Lua.

Requested by @gfgtdf in a comment of #3042.

jyrkive added a commit to jyrkive/wesnoth that referenced this issue May 15, 2018

Throw a Lua exception when creating a negative-HP unit in a Lua context
Instead of throwing a WML error. This allows the UMC author to get a stack
trace if the unit creation was triggered from Lua.

Requested by @gfgtdf in a comment of wesnoth#3042.
@jyrkive

This comment has been minimized.

Member

jyrkive commented May 15, 2018

Changed to a Lua exception (but obviously only if Lua code is running).

@jyrkive

This comment has been minimized.

Member

jyrkive commented May 16, 2018

@gfgtdf Commit b0c1382 addresses those four events. As far as I can tell, these six are the only events which can be called when the engine has set HP to a negative value.

jyrkive added a commit that referenced this issue May 28, 2018

Fix #3042: attack prediction gives wrong results for HP <= 0 units
One_strike_fight() assumed that if HP distribution hadn't been calculated,
the unit is alive. It would normally be a valid assumption, but the Wesnoth
engine allows units with negative HP (although things aren't guaranteed to
work correctly in the presence of such units).

The assumption, together with a completely wrong calculation for the
probability that the opponent will counterattack, resulted in badly
incorrect results. That, in turn, caused the calculated probability that
the opponent to kill us to become negative (I observed -75 % when
debugging), making the calculated probability to be poisoned/slowed to
exceed 100 %, and that finally caused an assert if the AI simulated another
fight for the same unit.

I have now fixed those issues. I also noticed that rounding error allowed
the probability to be killed to still become slightly negative, and thus
changed std::min() to utils::clamp() to limit the value to the allowed
range.

jyrkive added a commit to jyrkive/wesnoth that referenced this issue May 28, 2018

Fix wesnoth#3042: attack prediction gives wrong results for HP <= 0 u…
…nits

One_strike_fight() assumed that if HP distribution hadn't been calculated,
the unit is alive. It would normally be a valid assumption, but the Wesnoth
engine allows units with negative HP (although things aren't guaranteed to
work correctly in the presence of such units).

The assumption, together with a completely wrong calculation for the
probability that the opponent will counterattack, resulted in badly
incorrect results. That, in turn, caused the calculated probability that
the opponent to kill us to become negative (I observed -75 % when
debugging), making the calculated probability to be poisoned/slowed to
exceed 100 %, and that finally caused an assert if the AI simulated another
fight for the same unit.

I have now fixed those issues. I also noticed that rounding error allowed
the probability to be killed to still become slightly negative, and thus
changed std::min() to utils::clamp() to limit the value to the allowed
range.

jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018

Disallow units with negative HP
Damage calculation code can't tolerate presence of such units.

Fixes wesnoth#3042.

jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018

Allow modifying dead units in last_breath and die event handlers
Fixes an unintentional API change from commit 3c344e8, discussed in
comments of wesnoth#3042.

jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018

Throw a Lua exception when creating a negative-HP unit in a Lua context
Instead of throwing a WML error. This allows the UMC author to get a stack
trace if the unit creation was triggered from Lua.

Requested by @gfgtdf in a comment of wesnoth#3042.

jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018

Fix wesnoth#3042: attack prediction gives wrong results for HP <= 0 u…
…nits

One_strike_fight() assumed that if HP distribution hadn't been calculated,
the unit is alive. It would normally be a valid assumption, but the Wesnoth
engine allows units with negative HP (although things aren't guaranteed to
work correctly in the presence of such units).

The assumption, together with a completely wrong calculation for the
probability that the opponent will counterattack, resulted in badly
incorrect results. That, in turn, caused the calculated probability that
the opponent to kill us to become negative (I observed -75 % when
debugging), making the calculated probability to be poisoned/slowed to
exceed 100 %, and that finally caused an assert if the AI simulated another
fight for the same unit.

I have now fixed those issues. I also noticed that rounding error allowed
the probability to be killed to still become slightly negative, and thus
changed std::min() to utils::clamp() to limit the value to the allowed
range.

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

Disallow units with negative HP
Damage calculation code can't tolerate presence of such units.

Fixes wesnoth#3042.

(cherry-picked from commit ef60dea)

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

Allow modifying dead units in last_breath and die event handlers
Fixes an unintentional API change from commit 3c344e8, discussed in
comments of wesnoth#3042.

(cherry-picked from commit 15446ac)

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

Throw a Lua exception when creating a negative-HP unit in a Lua context
Instead of throwing a WML error. This allows the UMC author to get a stack
trace if the unit creation was triggered from Lua.

Requested by @gfgtdf in a comment of wesnoth#3042.

(cherry-picked from commit 258a0e4)

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

Fix wesnoth#3042: attack prediction gives wrong results for HP <= 0 u…
…nits

One_strike_fight() assumed that if HP distribution hadn't been calculated,
the unit is alive. It would normally be a valid assumption, but the Wesnoth
engine allows units with negative HP (although things aren't guaranteed to
work correctly in the presence of such units).

The assumption, together with a completely wrong calculation for the
probability that the opponent will counterattack, resulted in badly
incorrect results. That, in turn, caused the calculated probability that
the opponent to kill us to become negative (I observed -75 % when
debugging), making the calculated probability to be poisoned/slowed to
exceed 100 %, and that finally caused an assert if the AI simulated another
fight for the same unit.

I have now fixed those issues. I also noticed that rounding error allowed
the probability to be killed to still become slightly negative, and thus
changed std::min() to utils::clamp() to limit the value to the allowed
range.

(cherry-picked from commit 472b0cb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment