widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1636966-segfault-in-battle into lp:widelands.

Commit message:
Assume that we have no battle if the opponent is nullptr. Added some assertions. And a bit of refactoring.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1636966 in widelands: "Segfault in battle"
  https://bugs.launchpad.net/widelands/+bug/1636966

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1636966-segfault-in-battle/+merge/324365

After going through the code, I am making the following assumption: the opponent has died.

These are the commands sent by Battle when a soldier dies:

    soldier.start_task_die(game);
    molog("[battle] waking up winner %d\n", opponent(soldier)->serial());
    opponent(soldier)->send_signal(game, "wakeup");
    return schedule_destroy(game);

The wakeup signal then ends up leading to the part in the code that crashes. The wakeup is supposed to happen 10 ticks after the Battle is destroyed, but I guess that with very low frametime, this can get its wires crossed. This would also explain the rarity of the crash. I haven't dug really deeply into this part though.

There is already a comment in the code that running the command queue depends on frametime and that it's a bad idea.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1636966-segfault-in-battle into lp:widelands.

=== modified file 'src/logic/map_objects/tribes/battle.cc'
--- src/logic/map_objects/tribes/battle.cc	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/battle.cc	2017-05-20 19:50:11 +0000
@@ -53,23 +53,23 @@
      last_attack_hits_(false) {
 }
 
-Battle::Battle(Game& game, Soldier& First, Soldier& Second)
+Battle::Battle(Game& game, Soldier* first_soldier, Soldier* second_soldier)
    : MapObject(&g_battle_descr),
-     first_(&First),
-     second_(&Second),
+     first_(first_soldier),
+     second_(second_soldier),
      readyflags_(0),
      damage_(0),
      first_strikes_(true) {
-	assert(First.get_owner() != Second.get_owner());
+	assert(first_soldier->get_owner() != second_soldier->get_owner());
 	{
 		StreamWrite& ss = game.syncstream();
 		ss.unsigned_32(0x00e111ba);  // appears as ba111e00 in a hexdump
-		ss.unsigned_32(First.serial());
-		ss.unsigned_32(Second.serial());
+		ss.unsigned_32(first_soldier->serial());
+		ss.unsigned_32(second_soldier->serial());
 	}
 
-	// Ensures only live soldiers eganges in a battle
-	assert(First.get_current_health() && Second.get_current_health());
+	// Ensures only live soldiers engages in a battle
+	assert(first_soldier->get_current_health() && second_soldier->get_current_health());
 
 	init(game);
 }
@@ -129,7 +129,8 @@
 	return first_->get_position() == second_->get_position();
 }
 
-Soldier* Battle::opponent(Soldier& soldier) {
+Soldier* Battle::opponent(const Soldier& soldier) const {
+	assert(&soldier != nullptr);
 	assert(first_ == &soldier || second_ == &soldier);
 	Soldier* other_soldier = first_ == &soldier ? second_ : first_;
 	return other_soldier;

=== modified file 'src/logic/map_objects/tribes/battle.h'
--- src/logic/map_objects/tribes/battle.h	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/battle.h	2017-05-20 19:50:11 +0000
@@ -49,7 +49,7 @@
 	const BattleDescr& descr() const;
 
 	Battle();                           //  for loading an existing battle from a savegame
-	Battle(Game&, Soldier&, Soldier&);  //  to create a new battle in the game
+	Battle(Game&, Soldier*, Soldier*);  //  to create a new battle in the game
 
 	// Implements MapObject.
 	bool init(EditorGameBase&) override;
@@ -75,9 +75,9 @@
 	}
 
 	// Returns the other soldier involved in this battle. CHECKs that the given
-	// soldier is participating in this battle. Can return nullptr, but I have
-	// no idea what that means.
-	Soldier* opponent(Soldier&);
+	// soldier is participating in this battle. Can return nullptr, probably when the
+	// opponent has died.
+	Soldier* opponent(const Soldier&) const;
 
 	// Called by the battling soldiers once they've met on a common node and are
 	// idle.

=== modified file 'src/logic/map_objects/tribes/soldier.cc'
--- src/logic/map_objects/tribes/soldier.cc	2017-05-13 18:48:26 +0000
+++ src/logic/map_objects/tribes/soldier.cc	2017-05-20 19:50:11 +0000
@@ -1047,7 +1047,8 @@
 			for (Bob* temp_bob : soldiers) {
 				if (upcast(Soldier, temp_soldier, temp_bob)) {
 					if (temp_soldier->can_be_challenged()) {
-						new Battle(game, *this, *temp_soldier);
+						assert(temp_soldier != nullptr);
+						new Battle(game, this, temp_soldier);
 						return start_task_battle(game);
 					}
 				}
@@ -1134,7 +1135,8 @@
 
 		if (target.dist <= 1) {
 			molog("[defense] starting battle with %u!\n", target.s->serial());
-			new Battle(game, *this, *(target.s));
+			assert(target.s != nullptr);
+			new Battle(game, this, target.s);
 			return start_task_battle(game);
 		}
 
@@ -1257,7 +1259,8 @@
 		}
 	}
 
-	if (!battle_) {
+	// The opponent might have died on us
+	if (!battle_ || battle_->opponent(*this) == nullptr) {
 		if (combat_walking_ == CD_COMBAT_W) {
 			return start_task_move_in_battle(game, CD_RETURN_W);
 		}
@@ -1287,7 +1290,7 @@
 	if (stay_home()) {
 		if (this == battle_->first()) {
 			molog("[battle] stay_home, so reverse roles\n");
-			new Battle(game, *battle_->second(), *battle_->first());
+			new Battle(game, battle_->second(), battle_->first());
 			return skip_act();  //  we will get a signal via set_battle()
 		} else {
 			if (combat_walking_ != CD_COMBAT_E) {
@@ -1298,7 +1301,7 @@
 	} else {
 		if (opponent.stay_home() && (this == battle_->second())) {
 			// Wait until correct roles are assigned
-			new Battle(game, *battle_->second(), *battle_->first());
+			new Battle(game, battle_->second(), battle_->first());
 			return schedule_act(game, 10);
 		}
 
@@ -1488,7 +1491,7 @@
 		if (commit && !foundbattle && !multiplesoldiers) {
 			if (foundsoldier->owner().is_hostile(*get_owner()) && foundsoldier->can_be_challenged()) {
 				molog("[check_node_blocked] attacking a soldier (%u)\n", foundsoldier->serial());
-				new Battle(game, *this, *foundsoldier);
+				new Battle(game, this, foundsoldier);
 			}
 		}