widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1636966-one-soldier-crash into lp:widelands/build19.

Commit message:
Abort battle if opponent is nullptr to avoid segfaults.

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-one-soldier-crash/+merge/309445

There is no easy way to reproduce this, so I have followed the solution suggested in the bug.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1636966-one-soldier-crash into lp:widelands/build19.

=== modified file 'src/logic/map_objects/tribes/soldier.cc'
--- src/logic/map_objects/tribes/soldier.cc	2016-08-04 15:49:05 +0000
+++ src/logic/map_objects/tribes/soldier.cc	2016-10-27 08:33:31 +0000
@@ -1253,8 +1253,14 @@
 	}
 
 	Map& map = game.map();
-	Soldier& opponent = *battle_->opponent(*this);
-	if (opponent.get_position() != get_position()) {
+	Soldier* opponent = battle_->opponent(*this);
+	// If the opponent disappeared from under us, cancel the battle.
+	// TODO(GunChleoc): Analyze why the opponent can be nullptr.
+	if (!opponent) {
+		battle_->cancel(game, *this);
+		return;
+	}
+	if (opponent->get_position() != get_position()) {
 		if (is_a(Building, map[get_position()].get_immovable())) {
 			// Note that this does not use the "leavebuilding" task,
 			// because that task is geared towards orderly workers leaving
@@ -1273,19 +1279,19 @@
 			return skip_act();  //  we will get a signal via set_battle()
 		} else {
 			if (combat_walking_ != CD_COMBAT_E) {
-				opponent.send_signal(game, "wakeup");
+				opponent->send_signal(game, "wakeup");
 				return start_task_move_in_battle(game, CD_WALK_E);
 			}
 		}
 	} else {
-		if (opponent.stay_home() && (this == battle_->second())) {
+		if (opponent->stay_home() && (this == battle_->second())) {
 			// Wait until correct roles are assigned
 			new Battle(game, *battle_->second(), *battle_->first());
 			return schedule_act(game, 10);
 		}
 
-		if (opponent.get_position() != get_position()) {
-			Coords dest = opponent.get_position();
+		if (opponent->get_position() != get_position()) {
+			Coords dest = opponent->get_position();
 
 			if (upcast(Building, building, map[dest].get_immovable()))
 				dest = building->base_flag().get_position();
@@ -1318,8 +1324,8 @@
 					    static_cast<unsigned int>(owner().player_number()) % get_position().x %
 					    get_position().y %
 					    (immovable_position ? immovable_position->descr().descname().c_str() : ("no")) %
-					    opponent.descr().descname().c_str() % opponent.serial() %
-					    static_cast<unsigned int>(opponent.owner().player_number()) % dest.x % dest.y %
+					    opponent->descr().descname().c_str() % opponent->serial() %
+					    static_cast<unsigned int>(opponent->owner().player_number()) % dest.x % dest.y %
 					    (immovable_dest ? immovable_dest->descr().descname().c_str() : ("no")) %
 					    descr().descname().c_str())
 					      .str();
@@ -1329,22 +1335,22 @@
 					            "images/ui_basic/menu_help.png", _("Logic error"),
 					            (boost::format("<rt><p font-size=12>%s</p></rt>") % messagetext).str(),
 					            get_position(), serial_));
-					opponent.owner().add_message(
+					opponent->owner().add_message(
 					   game, *new Message(
 					            Message::Type::kGameLogic, game.get_gametime(), descr().descname(),
 					            "images/ui_basic/menu_help.png", _("Logic error"),
 					            (boost::format("<rt><p font-size=12>%s</p></rt>") % messagetext).str(),
-					            opponent.get_position(), serial_));
+					            opponent->get_position(), serial_));
 					game.game_controller()->set_desired_speed(0);
 					return pop_task(game);
 				}
 			}
 		} else {
-			assert(opponent.get_position() == get_position());
-			assert(battle_ == opponent.get_battle());
+			assert(opponent->get_position() == get_position());
+			assert(battle_ == opponent->get_battle());
 
-			if (opponent.is_walking()) {
-				molog("[battle]: Opponent '%d' is walking, sleeping\n", opponent.serial());
+			if (opponent->is_walking()) {
+				molog("[battle]: Opponent '%d' is walking, sleeping\n", opponent->serial());
 				// We should be woken up by our opponent, but add a timeout anyway for robustness
 				return start_task_idle(game, descr().get_animation("idle"), 5000);
 			}
@@ -1352,13 +1358,13 @@
 			if (battle_->first()->serial() == serial()) {
 				if (combat_walking_ != CD_COMBAT_W) {
 					molog("[battle]: Moving west\n");
-					opponent.send_signal(game, "wakeup");
+					opponent->send_signal(game, "wakeup");
 					return start_task_move_in_battle(game, CD_WALK_W);
 				}
 			} else {
 				if (combat_walking_ != CD_COMBAT_E) {
 					molog("[battle]: Moving east\n");
-					opponent.send_signal(game, "wakeup");
+					opponent->send_signal(game, "wakeup");
 					return start_task_move_in_battle(game, CD_WALK_E);
 				}
 			}