widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1815613-map-origin-segfault into lp:widelands.

Commit message:
Fix number overflow in Map::set_origin and clean up the function.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1815613 in widelands: "Editor crash when setting new 0,0 coordinates"
  https://bugs.launchpad.net/widelands/+bug/1815613

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1815613-map-origin-segfault/+merge/363080

The fix is replacing decltype with size_t, because the positive range of int16_t is too small. The rest is cleanup.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1815613-map-origin-segfault into lp:widelands.

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2018-12-13 07:24:01 +0000
+++ src/logic/map.cc	2019-02-12 17:49:27 +0000
@@ -330,60 +330,66 @@
 	assert(0 <= new_origin.y);
 	assert(new_origin.y < height_);
 
+	const size_t field_size = width_ * height_;
+
 	for (uint8_t i = get_nrplayers(); i;) {
 		starting_pos_[--i].reorigin(new_origin, extent());
 	}
 
-	std::unique_ptr<Field[]> new_field_order(new Field[width_ * height_]);
-	memset(new_field_order.get(), 0, sizeof(Field) * width_ * height_);
+	std::unique_ptr<Field[]> new_field_order(new Field[field_size]);
+	memset(new_field_order.get(), 0, sizeof(Field) * field_size);
 
 	// Rearrange The fields
-	// NOTE because of the triangle design, we have to take special care about cases
+	// NOTE because of the triangle design, we have to take special care of cases
 	// NOTE where y is changed by an odd number
 	bool yisodd = (new_origin.y % 2) != 0;
 	for (Coords c(Coords(0, 0)); c.y < height_; ++c.y) {
 		bool cyisodd = (c.y % 2) != 0;
 		for (c.x = 0; c.x < width_; ++c.x) {
 			Coords temp;
-			if (yisodd && cyisodd)
+			if (yisodd && cyisodd) {
 				temp = Coords(c.x + new_origin.x + 1, c.y + new_origin.y);
-			else
+			} else {
 				temp = Coords(c.x + new_origin.x, c.y + new_origin.y);
+			}
 			normalize_coords(temp);
 			new_field_order[get_index(c, width_)] = operator[](temp);
 		}
 	}
 	// Now that we restructured the fields, we just overwrite the old order
-	for (decltype(width_) ind = 0; ind < width_ * height_; ind++) {
+	for (size_t ind = 0; ind < field_size; ind++) {
 		fields_[ind] = new_field_order[ind];
 	}
 
 	//  Inform immovables and bobs about their new coordinates.
-	for (FCoords c(Coords(0, 0), fields_.get()); c.y < height_; ++c.y)
+	for (FCoords c(Coords(0, 0), fields_.get()); c.y < height_; ++c.y) {
 		for (c.x = 0; c.x < width_; ++c.x, ++c.field) {
 			assert(c.field == &operator[](c));
-			if (upcast(Immovable, immovable, c.field->get_immovable()))
+			if (upcast(Immovable, immovable, c.field->get_immovable())) {
 				immovable->position_ = c;
+			}
 			for (Bob* bob = c.field->get_first_bob(); bob; bob = bob->get_next_bob()) {
 				bob->position_.x = c.x;
 				bob->position_.y = c.y;
 				bob->position_.field = c.field;
 			}
 		}
+	}
 
-	// Take care about port spaces
+	// Take care of port spaces
 	PortSpacesSet new_port_spaces;
 	for (PortSpacesSet::iterator it = port_spaces_.begin(); it != port_spaces_.end(); ++it) {
 		Coords temp;
-		if (yisodd && ((it->y % 2) == 0))
+		if (yisodd && ((it->y % 2) == 0)) {
 			temp = Coords(it->x - new_origin.x - 1, it->y - new_origin.y);
-		else
+		} else {
 			temp = Coords(it->x - new_origin.x, it->y - new_origin.y);
+		}
 		normalize_coords(temp);
-		log("(%i,%i) -> (%i,%i)\n", it->x, it->y, temp.x, temp.y);
 		new_port_spaces.insert(temp);
 	}
 	port_spaces_ = new_port_spaces;
+	log("Map origin was shifted by (%d, %d)\n", new_origin.x, new_origin.y);
 }
 
 /*