widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1734088-asan-fleet-active into lp:widelands.

Commit message:
Fixed heap-use-after-free Fleet::active()

The problem was that a when a fleet is initialized, it looks for another fleet to merge with. When that happens, the fleet can become the deleted one, so it's impossible to ask it whether it's active then.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1734088 in widelands: "heap-use-after-free Fleet::active()"
  https://bugs.launchpad.net/widelands/+bug/1734088

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1734088-asan-fleet-active/+merge/334276

The fruits of today's labor: Fixed heap-use-after-free Fleet::active() :)

The test suite is still producing memory leaks from the graphics system, so all the jobs where this issue was fixed will still fail.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1734088-asan-fleet-active into lp:widelands.

=== modified file 'src/economy/fleet.cc'
--- src/economy/fleet.cc	2017-08-19 22:22:20 +0000
+++ src/economy/fleet.cc	2017-11-25 16:07:33 +0000
@@ -102,13 +102,7 @@
 		return false;
 	}
 
-	find_other_fleet(egbase);
-
-	if (active()) {
-		update(egbase);
-		return true;
-	}
-	return false;
+	return find_other_fleet(egbase);
 }
 
 struct StepEvalFindFleet {
@@ -137,7 +131,7 @@
  * Search the map, starting at our ships and ports, for another fleet
  * of the same player.
  */
-void Fleet::find_other_fleet(EditorGameBase& egbase) {
+bool Fleet::find_other_fleet(EditorGameBase& egbase) {
 	MapAStar<StepEvalFindFleet> astar(*egbase.mutable_map(), StepEvalFindFleet());
 	for (const Ship* temp_ship : ships_) {
 		astar.push(temp_ship->get_position());
@@ -164,8 +158,7 @@
 						    dock->dockpoints_.front().y);
 					}
 					if (dock->get_fleet() != this && dock->get_owner() == get_owner()) {
-						dock->get_fleet()->merge(egbase, this);
-						return;
+						return dock->get_fleet()->merge(egbase, this);
 					}
 				}
 			}
@@ -178,21 +171,29 @@
 			if (upcast(Ship, ship, bob)) {
 				if (ship->get_fleet() != nullptr && ship->get_fleet() != this &&
 				    ship->get_owner() == get_owner()) {
-					ship->get_fleet()->merge(egbase, this);
-					return;
+					return ship->get_fleet()->merge(egbase, this);
 				}
 			}
 		}
 	}
+	if (active()) {
+		update(egbase);
+		return true;
+	}
+	return false;
 }
 
 /**
  * Merge the @p other fleet into this fleet, and remove the other fleet.
+ *
+ * Returns true if 'other' is the resulting fleet and "false" if 'this' is
+ * the resulting fleet. The values are reversed because we originally call this from
+ * another 'other' for efficiency reasons.
  */
-void Fleet::merge(EditorGameBase& egbase, Fleet* other) {
+bool Fleet::merge(EditorGameBase& egbase, Fleet* other) {
 	if (ports_.empty() && !other->ports_.empty()) {
 		other->merge(egbase, this);
-		return;
+		return true;
 	}
 
 	while (!other->ships_.empty()) {
@@ -223,6 +224,7 @@
 	other->remove(egbase);
 
 	update(egbase);
+	return false;
 }
 
 /**

=== modified file 'src/economy/fleet.h'
--- src/economy/fleet.h	2017-06-24 08:47:46 +0000
+++ src/economy/fleet.h	2017-11-25 16:07:33 +0000
@@ -115,8 +115,8 @@
 	void act(Game&, uint32_t data) override;
 
 private:
-	void find_other_fleet(EditorGameBase& egbase);
-	void merge(EditorGameBase& egbase, Fleet* other);
+	bool find_other_fleet(EditorGameBase& egbase);
+	bool merge(EditorGameBase& egbase, Fleet* other);
 	void check_merge_economy();
 	void connect_port(EditorGameBase& egbase, uint32_t idx);