[Widelands-cvs] SF.net SVN: widelands: [3184] trunk/src

[<si...@us...>]

Revision: 3184
          http://widelands.svn.sourceforge.net/widelands/?rev=3184&view=rev
Author:   sigra
Date:     2008-04-19 17:04:54 -0700 (Sat, 19 Apr 2008)

Log Message:
-----------
Fix bug #1945549 reported and hunted down by Jari Hautio - jaari. In Critter_Bob::roam_update, the state pointer was used when it might be invalid after a successful call to "bool Bob::start_task_movepath". Now state->ivar is set to 0 before the call to start_task_movepath and then set to 1 again if the call failed.

Attempt to prevent this kind of error in <whatever>_update(Game *, State *) functions by combinging statements that invalidate the state pointer with the return statement (that usually follows immediately). Now it is more difficult to accidentally insert a statement that uses the state pointer between for example a call to pop_task or start_task_<whatever> and the end of control.

Modified Paths:
--------------
    trunk/src/bob.cc
    trunk/src/carrier.cc
    trunk/src/critter_bob.cc
    trunk/src/soldier.cc
    trunk/src/worker.cc

Modified: trunk/src/bob.cc
===================================================================
--- trunk/src/bob.cc	2008-04-18 23:24:07 UTC (rev 3183)
+++ trunk/src/bob.cc	2008-04-20 00:04:54 UTC (rev 3184)
@@ -490,10 +490,8 @@
 
 void Bob::idle_update(Game* g, State* state)
 {
-	if (!state->ivar1 || get_signal().size()) {
-		pop_task(g);
-		return;
-	}
+	if (!state->ivar1 || get_signal().size())
+		return pop_task(g);
 
 	if (state->ivar1 > 0)
 		schedule_act(g, state->ivar1);
@@ -714,16 +712,12 @@
 		if (curidx < index) {
 			path.truncate(index);
 			path.starttrim(curidx);
-
-			start_task_movepath(g, path, anims, forceonlast, only_step);
 		} else {
 			path.truncate(curidx);
 			path.starttrim(index);
 			path.reverse();
-
-			start_task_movepath(g, path, anims, forceonlast, only_step);
 		}
-
+		start_task_movepath(g, path, anims, forceonlast, only_step);
 		return true;
 	}
 
@@ -735,20 +729,16 @@
 {
 	if (get_signal().size()) {
 		molog("[movepath]: Interrupted by signal '%s'.\n", get_signal().c_str());
-
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	assert(state->ivar1 >= 0);
 	const Path * const path = state->path;
 
-	if (!path) {
+	if (!path)
 		// probably success; this can happen when loading a game
 		// that contains a zero-length path.
-		pop_task(g);
-		return;
-	}
+		return pop_task(g);
 
 	if
 		(static_cast<const Path::Step_Vector::size_type>(state->ivar1)
@@ -756,15 +746,12 @@
 		 path->get_nsteps())
 	{
 		assert(m_position == path->get_end());
-		pop_task(g); // success
-		return;
-	} else if (state->ivar1==state->ivar3) {
+		return pop_task(g); // success
+	} else if (state->ivar1 == state->ivar3)
 		// We have stepped all steps that we were asked for.
 		// This is some kind of success, though we do not are were we wanted
 		// to go
-		pop_task(g);
-		return;
-	}
+		return pop_task(g);
 
 	const Direction dir = (*path)[state->ivar1];
 	bool forcemove = false;
@@ -780,7 +767,7 @@
 	}
 
 	++state->ivar1;
-	start_task_move(g, dir, state->diranims, forcemove);
+	return start_task_move(g, dir, state->diranims, forcemove);
 	// Note: state pointer is invalid beyond this point
 }
 
@@ -815,34 +802,27 @@
 void Bob::move_update(Game* g, State* state)
 {
 	if (state->diranims) {
-		int32_t tdelta = start_walk
-			(g,
-			 static_cast<WalkingDir>(state->ivar1),
-			 state->diranims->get_animation(state->ivar1),
-			 state->ivar2);
+		int32_t const tdelta =
+			start_walk
+				(g,
+				 static_cast<WalkingDir>(state->ivar1),
+				 state->diranims->get_animation(state->ivar1),
+				 state->ivar2);
 		state->diranims = 0;
 
-		if (tdelta == -2) {
-			send_signal(g, "blocked");
-			pop_task(g);
-			return;
-		} else if (tdelta < 0) {
-			send_signal(g, "fail");
-			pop_task(g);
-			return;
-		}
-
-		schedule_act(g, tdelta);
-	} else {
+		if (tdelta < 0) {
+			send_signal(g, tdelta == -2 ? "blocked" : "fail");
+			return pop_task(g);
+		} else
+			return schedule_act(g, tdelta);
+	} else
 		if (g->get_gametime() - m_walkend >= 0) {
 			end_walk();
-			pop_task(g);
-		} else {
+			return pop_task(g);
+		} else
 			// Only end the task once we've actually completed the step
 			// Ignore signals until then
-			schedule_act(g, m_walkend - g->get_gametime());
-		}
-	}
+			return schedule_act(g, m_walkend - g->get_gametime());
 }
 
 

Modified: trunk/src/carrier.cc
===================================================================
--- trunk/src/carrier.cc	2008-04-18 23:24:07 UTC (rev 3183)
+++ trunk/src/carrier.cc	2008-04-20 00:04:54 UTC (rev 3184)
@@ -69,8 +69,7 @@
 	} else if (signal.size()) {
 		// Something else happened (probably a location signal)
 		molog("[road]: Terminated by signal '%s'\n", signal.c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	Road & road = dynamic_cast<Road &>(*get_location(g));
@@ -82,17 +81,15 @@
 	if (m_acked_ware >= 0) {
 		if (state->ivar1) {
 			state->ivar1 = 0;
-			start_task_transport(g, m_acked_ware);
+			return start_task_transport(g, m_acked_ware);
 		} else {
 			// Short delay before we move to pick up
 			molog("[road]: delay (acked for %i)\n", m_acked_ware);
 			state->ivar1 = 1;
 
 			set_animation(g, descr().get_animation("idle"));
-			schedule_act(g, 50);
+			return schedule_act(g, 50);
 		}
-
-		return;
 	}
 
 	// Move into idle position if necessary
@@ -148,8 +145,7 @@
 		signal_handled();
 	} else if (signal.size()) {
 		molog("[transport]: Interrupted by signal '%s'\n", signal.c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	Road & road = dynamic_cast<Road &>(*get_location(g));
@@ -186,8 +182,6 @@
 			// Drop the item, possible exchanging it with another one
 			drop_item(g, state);
 	}
-
-	return;
 }
 
 

Modified: trunk/src/critter_bob.cc
===================================================================
--- trunk/src/critter_bob.cc	2008-04-18 23:24:07 UTC (rev 3183)
+++ trunk/src/critter_bob.cc	2008-04-20 00:04:54 UTC (rev 3184)
@@ -299,8 +299,7 @@
 {
 	if (get_signal().size()) {
 		molog("[program]: Interrupted by signal '%s'\n", get_signal().c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	const Critter_BobAction* action;
@@ -342,36 +341,30 @@
 
 void Critter_Bob::roam_update(Game* g, State* state)
 {
-	if (get_signal().size()) {
-		pop_task(g);
-		return;
-	}
+	if (get_signal().size())
+		return pop_task(g);
 
 	// alternately move and idle
+	Time idle_time_min = 1000;
+	Time idle_time_rnd = CRITTER_MAX_WAIT_TIME_BETWEEN_WALK;
 	if (state->ivar1) {
+		idle_time_min =    1;
+		idle_time_rnd = 1000;
+		state->ivar1  =    0;
 		if
 			(start_task_movepath
 			 	(g,
 			 	 g->random_location(get_position(), 2), //  Pick a random target.
 			 	 3,
 			 	 descr().get_walk_anims()))
-		{
-			state->ivar1 = 0;
 			return;
-		}
-
-		//molog("        Failed\n");
-
-		start_task_idle(g, descr().get_animation("idle"), 1 + g->logic_rand()%1000);
 	}
-	else
-	{
-		state->ivar1 = 1;
-
-		//molog("[roam]: Idle\n");
-
-		start_task_idle(g, descr().get_animation("idle"), 1000 + g->logic_rand() % CRITTER_MAX_WAIT_TIME_BETWEEN_WALK);
-	}
+	state->ivar1 = 1;
+	return
+		start_task_idle
+			(g,
+			 descr().get_animation("idle"),
+			 idle_time_min + g->logic_rand() % idle_time_rnd);
 }
 
 void Critter_Bob::init_auto_task(Game *g) {

Modified: trunk/src/soldier.cc
===================================================================
--- trunk/src/soldier.cc	2008-04-18 23:24:07 UTC (rev 3183)
+++ trunk/src/soldier.cc	2008-04-20 00:04:54 UTC (rev 3184)
@@ -518,13 +518,11 @@
 				state->objvar1 = 0;
 			} else {
 				molog("[attack] unexpected fail\n");
-				pop_task(g);
-				return;
+				return pop_task(g);
 			}
 		} else {
 			molog("[attack] cancelled by unexpected signal '%s'\n", signal.c_str());
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 	}
 
@@ -535,39 +533,34 @@
 	if (imm == location) {
 		if (!enemy) {
 			molog("[attack] returned home\n");
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 
-		start_task_leavebuilding(g, false);
-		return;
+		return start_task_leavebuilding(g, false);
 	}
 
-	if (m_battle) {
-		startTaskBattle(g);
-		return;
-	}
+	if (m_battle)
+		return startTaskBattle(g);
 
-	if (signal == "blocked") {
+	if (signal == "blocked")
 		// Wait before we try again. Note that this must come *after*
 		// we check for a battle
-		start_task_idle(g, get_animation("idle"), 5000);
-		return;
-	}
+		return start_task_idle(g, get_animation("idle"), 5000);
 
 	if (!location) {
 		molog("[attack] our location disappeared during a battle\n");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	if (!enemy) {
 		Flag* baseflag = location->get_base_flag();
-		if (imm == baseflag) {
-			start_task_move
-				(g, WALK_NW, &descr().get_right_walk_anims(does_carry_ware()), true);
-			return;
-		}
+		if (imm == baseflag)
+			return
+				start_task_move
+					(g,
+					 WALK_NW,
+					 &descr().get_right_walk_anims(does_carry_ware()),
+					 true);
 
 		molog("[attack] return home\n");
 		start_task_movepath
@@ -585,8 +578,7 @@
 		}
 
 		state->objvar1 = 0;
-		schedule_act(g, 10);
-		return;
+		return schedule_act(g, 10);
 	}
 
 	// At this point, we know that the enemy building still stands,
@@ -603,10 +595,8 @@
 	assert(attackable);
 
 	molog("[attack] attacking target building\n");
-	if (attackable->attack(this))
-		schedule_act(g, 1000); // give the enemy soldier some time to act
-	else
-		schedule_act(g, 10);
+	//  give the enemy soldier some time to act
+	schedule_act(g, attackable->attack(this) ? 1000 : 10);
 }
 
 void Soldier::attack_pop(Game* g, State*)
@@ -656,20 +646,16 @@
 	PlayerImmovable* location = get_location(g);
 	BaseImmovable* position = g->map()[get_position()].get_immovable();
 	if (m_battle) {
-		if (position == location) {
-			start_task_leavebuilding(g, false);
-			return;
-		}
+		if (position == location)
+			return start_task_leavebuilding(g, false);
 
 		state->ivar2 = 0;
-		startTaskBattle(g);
-		return;
+		return startTaskBattle(g);
 	}
 
 	if (!location) {
 		molog("[defense] location disappeared during battle\n");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	if (signal == "blocked") {
@@ -681,20 +667,21 @@
 
 	if (position == location) {
 		molog("[defense] returned home\n");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	Flag* baseflag = location->get_base_flag();
 	if (position == baseflag) {
 		if (state->ivar1 && !state->ivar2) {
 			state->ivar2 = 1;
-			start_task_idle(g, get_animation("idle"), 250);
-			return;
+			return start_task_idle(g, get_animation("idle"), 250);
 		}
-		start_task_move
-			(g, WALK_NW, &descr().get_right_walk_anims(does_carry_ware()), true);
-		return;
+		return
+			start_task_move
+				(g,
+				 WALK_NW,
+				 &descr().get_right_walk_anims(does_carry_ware()),
+				 true);
 	}
 
 	molog("[defense] return home\n");

Modified: trunk/src/worker.cc
===================================================================
--- trunk/src/worker.cc	2008-04-18 23:24:07 UTC (rev 3183)
+++ trunk/src/worker.cc	2008-04-20 00:04:54 UTC (rev 3184)
@@ -1204,8 +1204,7 @@
 	// but this assumption may fail when loading a corrupted savegame.
 	if (!location) {
 		send_signal(g, "location");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	// The request is no longer valid, the task has failed
@@ -1213,8 +1212,7 @@
 		molog("[transfer]: Fail (without transfer)\n");
 
 		send_signal(g, "fail");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	// Signal handling
@@ -1230,8 +1228,7 @@
 			signal_handled();
 		} else {
 			molog("[transfer]: Cancel due to signal '%s'\n", signal.c_str());
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 	}
 
@@ -1248,10 +1245,8 @@
 			if (upcast(Flag, flag, position)) {
 				location = flag;
 				set_location(flag);
-			} else {
-				set_location(0);
-				return;
-			}
+			} else
+				return set_location(0);
 		}
 	} else if (upcast(Flag, flag, location)) {
 		BaseImmovable * const position = map[get_position()].get_immovable();
@@ -1261,10 +1256,8 @@
 				Building* building = static_cast<Building*>(position);
 				set_location(building);
 				location = building;
-			} else {
-				set_location(0);
-				return;
-			}
+			} else
+				return set_location(0);
 		}
 	}
 
@@ -1283,15 +1276,14 @@
 			pop_task(g);
 
 			t->has_finished();
-			return;
 		} else {
 			molog("[transfer]: Failed\n");
 			send_signal(g, "fail");
 			pop_task(g);
 
 			t->has_failed();
-			return;
 		}
+		return;
 	}
 
 	// Initiate the next step
@@ -1447,25 +1439,16 @@
 	signal_handled();
 
 	if (state->ivar1 == 1)
-	{
-		if (signal == "fail")
-			state->ivar1 = 2;
-		else
-			state->ivar1 = 0;
-	}
+		state->ivar1 = (signal == "fail") * 2;
 
 	// Return to building, if necessary
 	Building* building = dynamic_cast<Building*>(get_location(g));
-	if (!building) {
-		pop_task(g);
-		return;
-	}
+	if (!building)
+		return pop_task(g);
 
 	if (g->map().get_immovable(get_position()) != building) {
 		molog("[buildingwork]: Something went wrong, return home.\n");
-
-		start_task_return(g, false); // don't drop item
-		return;
+		return start_task_return(g, false); // don't drop item
 	}
 
 	// Get the new job
@@ -1533,8 +1516,7 @@
 
 	if (signal == "location") {
 		molog("[return]: Interrupted by signal '%s'\n", signal.c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	molog("[return]: Ignoring and blocking signal '%s'\n", signal.c_str());
@@ -1544,8 +1526,7 @@
 	if (BaseImmovable * const pos = g->map().get_immovable(get_position())) {
 		if (pos == &location) {
 			set_animation(g, 0);
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 
 		if (upcast(Flag, flag, pos)) {
@@ -1560,16 +1541,18 @@
 						flag->add_item(g, item);
 
 						set_animation(g, descr().get_animation("idle"));
-						schedule_act(g, 20); // rest a while
-						return;
+						return schedule_act(g, 20); // rest a while
 					}
 				}
 
 				molog("[return]: Move back into building\n");
 
-				start_task_move
-					(g, WALK_NW, &descr().get_right_walk_anims(does_carry_ware()), true);
-				return;
+				return
+					start_task_move
+						(g,
+						 WALK_NW,
+						 &descr().get_right_walk_anims(does_carry_ware()),
+						 true);
 			}
 		}
 	}
@@ -1587,9 +1570,7 @@
 		 	 descr().get_right_walk_anims(does_carry_ware())))
 	{
 		molog("[return]: Failed to return\n");
-
-		set_location(0);
-		return;
+		return set_location(0);
 	}
 }
 
@@ -1626,8 +1607,7 @@
 {
 	if (get_signal().size()) {
 		molog("[program]: Interrupted by signal '%s'\n", get_signal().c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	const Action* action;
@@ -1638,8 +1618,7 @@
 
 		if (state->ivar1 >= program->get_size()) {
 			molog("  End of program\n");
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 
 		action = program->get_action(state->ivar1);
@@ -1677,8 +1656,7 @@
 
 	if (!location) {
 		send_signal(g, "location");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	// Signal handling
@@ -1694,8 +1672,7 @@
 			signal_handled();
 		} else {
 			molog("[gowarehouse]: cancel for signal '%s'\n", signal.c_str());
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 	}
 
@@ -1885,8 +1862,7 @@
 	if (!get_carried_item(g) && !state->ivar1) {
 		if (dynamic_cast<Building const *>(location)) {
 			molog("[fetchfromflag]: move from building to flag\n");
-			start_task_leavebuilding(g, false);
-			return;
+			return start_task_leavebuilding(g, false);
 		}
 
 		state->ivar1 = 1; // force return to building
@@ -1899,8 +1875,7 @@
 			set_carried_item(g, item);
 
 		set_animation(g, descr().get_animation("idle"));
-		schedule_act(g, 20);
-		return;
+		return schedule_act(g, 20);
 	}
 
 	// Go back into the building
@@ -1974,8 +1949,7 @@
 	if (signal.size()) {
 		if (signal == "wakeup")
 			signal_handled();
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	skip_act(); // wait indefinitely
@@ -2052,8 +2026,7 @@
 		signal_handled();
 	} else if (signal.size()) {
 		molog("[leavebuilding]: Interrupted by signal '%s'\n", signal.c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	if (upcast(Building, building, g->map().get_immovable(get_position()))) {
@@ -2061,8 +2034,7 @@
 
 		if (!building->leave_check_and_wait(g, this)) {
 			molog("[leavebuilding]: Wait\n");
-			skip_act();
-			return;
+			return skip_act();
 		}
 
 		molog
@@ -2072,10 +2044,14 @@
 		if (state->ivar1)
 			set_location(building->get_base_flag());
 
-		start_task_move
-			(g, WALK_SE, &descr().get_right_walk_anims(does_carry_ware()), true);
+		return
+			start_task_move
+				(g,
+				 WALK_SE,
+				 &descr().get_right_walk_anims(does_carry_ware()),
+				 true);
 	} else
-		pop_task(g);
+		return pop_task(g);
 }
 
 
@@ -2151,8 +2127,7 @@
 {
 	if (get_signal().size()) {
 		molog("[fugitive]: interrupted by signal '%s'\n", get_signal().c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	Map & map = g->map();
@@ -2161,10 +2136,8 @@
 	if (location && location->get_owner() == get_owner()) {
 		molog("[fugitive]: we're on location\n");
 
-		if (dynamic_cast<Warehouse const *>(location)) {
-			schedule_incorporate(g);
-			return;
-		}
+		if (dynamic_cast<Warehouse const *>(location))
+			return schedule_incorporate(g);
 
 		set_location(0);
 		location = 0;
@@ -2178,8 +2151,7 @@
 			 flag->economy().get_nr_warehouses())
 		{
 			set_location(flag);
-			pop_task(g);
-			return;
+			return pop_task(g);
 		}
 	}
 
@@ -2222,7 +2194,6 @@
 					 	 best->get_position(),
 					 	 0,
 					 	 descr().get_right_walk_anims(does_carry_ware())))
-
 					return;
 			}
 		}
@@ -2230,8 +2201,7 @@
 
 	if (state->ivar1 < g->get_gametime()) {//  time to die?
 		molog("[fugitive]: die\n");
-		schedule_destroy(g);
-		return;
+		return schedule_destroy(g);
 	}
 
 	molog("[fugitive]: wander randomly\n");
@@ -2244,7 +2214,7 @@
 		 	 descr().get_right_walk_anims(does_carry_ware())))
 		return;
 
-	start_task_idle(g, descr().get_animation("idle"), 50);
+	return start_task_idle(g, descr().get_animation("idle"), 50);
 }
 
 
@@ -2289,8 +2259,7 @@
 		signal_handled();
 	} else if (signal.size()) {
 		molog("[geologist]: Interrupted by signal '%s'\n", signal.c_str());
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	//
@@ -2315,8 +2284,7 @@
 			molog("[geologist]: Starting program '%s'\n", state->svar1.c_str());
 
 			--state->ivar1;
-			start_task_program(g, state->svar1);
-			return;
+			return start_task_program(g, state->svar1);
 		}
 
 		// Find a suitable field and walk towards it
@@ -2378,8 +2346,7 @@
 
 					molog("[geologist]: BUG: couldn't find path\n");
 					send_signal(g, "fail");
-					pop_task(g);
-					return;
+					return pop_task(g);
 				}
 				return;
 			}
@@ -2391,8 +2358,7 @@
 
 	if (get_position() == owner_area) {
 		molog("[geologist]: We're home\n");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 
 	molog("[geologist]: Return home\n");
@@ -2403,8 +2369,7 @@
 	{
 		molog("[geologist]: Couldn't find path home\n");
 		send_signal(g, "fail");
-		pop_task(g);
-		return;
+		return pop_task(g);
 	}
 }
 


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.