widelands-dev team mailing list archive

[Miroslav Remák <miroslavr256@xxxxxxxxx>]

Miroslav Remák has proposed merging lp:~miroslavr256/widelands/bug-1550568-restool_undo_crash into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1550568 in widelands: "editor crashes upon placing resources"
  https://bugs.launchpad.net/widelands/+bug/1550568

For more details, see:
https://code.launchpad.net/~miroslavr256/widelands/bug-1550568-restool_undo_crash/+merge/289007

The cause of this bug is dereferencing a past-the-end iterator in EditorSetResourcesTool::handle_undo_impl and setting a field's resource index to this undefined value.

In certain situations the 'increase/decrease resources' implementation can skip storing some of the old resource indices and amounts, which the undo implementation does not expect.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~miroslavr256/widelands/bug-1550568-restool_undo_crash into lp:widelands.

=== modified file 'src/editor/tools/editor_decrease_resources_tool.cc'
--- src/editor/tools/editor_decrease_resources_tool.cc	2016-01-16 12:55:14 +0000
+++ src/editor/tools/editor_decrease_resources_tool.cc	2016-03-15 05:03:53 +0000
@@ -50,10 +50,11 @@
 		if (amount < 0)
 			amount = 0;
 
+		args->orgResT.push_back(mr.location().field->get_resources());
+		args->orgRes.push_back(mr.location().field->get_resources_amount());
+
 		if (mr.location().field->get_resources() == args->cur_res &&
 			map->is_resource_valid(world, mr.location(), args->cur_res)) {
-			args->orgResT.push_back(mr.location().field->get_resources());
-			args->orgRes.push_back(mr.location().field->get_resources_amount());
 			map->initialize_resources(mr.location(), args->cur_res, amount);
 		}
 

=== modified file 'src/editor/tools/editor_increase_resources_tool.cc'
--- src/editor/tools/editor_increase_resources_tool.cc	2016-01-14 22:09:24 +0000
+++ src/editor/tools/editor_increase_resources_tool.cc	2016-03-15 05:03:53 +0000
@@ -47,11 +47,12 @@
 		if (amount > max_amount)
 			amount = max_amount;
 
+		args->orgResT.push_back(mr.location().field->get_resources());
+		args->orgRes.push_back(mr.location().field->get_resources_amount());
+
 		if ((mr.location().field->get_resources() == args->cur_res ||
 				!mr.location().field->get_resources_amount()) &&
 				map->is_resource_valid(world, mr.location(), args->cur_res)) {
-			args->orgResT.push_back(mr.location().field->get_resources());
-			args->orgRes.push_back(mr.location().field->get_resources_amount());
 			map->initialize_resources(mr.location(), args->cur_res, amount);
 		}
 	} while (mr.advance(*map));