<URL: http://bugs.freeciv.org/Ticket/Display.html?id=39668 >
On 04/09/2007, Marko Lindqvist wrote:
>
> Calling script_signal_emit() means that almost anything can happen.
> Most definitely cities or units can disappear.
Actually, they can't disappear just yet. Scripting improvements allowing
city destruction and unit death are waiting for this ticket.
> I just glanced where it has been called, and fixed most obvious cases
> where some now potentially invalid pointer was used immediately after
> script_signal_emit().
Updated against current S2_2 svn. Untested, as #40166 prevented testing.
- ML
diff -Nurd -X.diff_ignore freeciv/common/city.c freeciv/common/city.c
--- freeciv/common/city.c 2008-03-12 21:58:28.000000000 +0200
+++ freeciv/common/city.c 2008-03-24 16:14:28.000000000 +0200
@@ -2592,3 +2592,17 @@
memset(pcity, 0, sizeof(*pcity)); /* ensure no pointers remain */
free(pcity);
}
+
+/**************************************************************************
+ Check if city with given id still exist. Use this before using
+ old city pointers when city might have disappeared.
+**************************************************************************/
+bool city_exist(int id)
+{
+ /* Check if city exist in game */
+ if (game_find_city_by_number(id)) {
+ return TRUE;
+ }
+
+ return FALSE;
+}
diff -Nurd -X.diff_ignore freeciv/common/city.h freeciv/common/city.h
--- freeciv/common/city.h 2008-03-12 21:58:28.000000000 +0200
+++ freeciv/common/city.h 2008-03-24 15:40:04.000000000 +0200
@@ -610,6 +610,8 @@
int *pollu_prod, int *pollu_pop, int *pollu_mod);
int city_pollution(const struct city *pcity, int shield_total);
+bool city_exist(int id);
+
/*
* Iterates over all improvements, skipping those not yet built in the
* given city.
diff -Nurd -X.diff_ignore freeciv/server/cityturn.c freeciv/server/cityturn.c
--- freeciv/server/cityturn.c 2008-03-12 21:58:27.000000000 +0200
+++ freeciv/server/cityturn.c 2008-03-24 16:01:55.000000000 +0200
@@ -584,6 +584,7 @@
struct tile *pcenter = city_tile(pcity);
struct player *powner = city_owner(pcity);
struct impr_type *pimprove = pcity->production.value.building;
+ int saved_id = pcity->id;
if (!city_can_grow_to(pcity, pcity->size + 1)) { /* need improvement */
if (get_current_construction_bonus(pcity, EFT_SIZE_ADJ, RPT_CERTAIN) > 0
@@ -655,8 +656,10 @@
pcity->size);
script_signal_emit("city_growth", 2,
API_TYPE_CITY, pcity, API_TYPE_INT, pcity->size);
-
- sanity_check_city(pcity);
+ if (city_exist(saved_id)) {
+ /* Script didn't destroy this city */
+ sanity_check_city(pcity);
+ }
sync_cities();
}
@@ -781,14 +784,30 @@
struct universal target;
bool success = FALSE;
int i;
+ int saved_id = pcity->id;
+ bool city_checked = TRUE; /* This is used to avoid spurious city_exist() calls */
if (worklist_is_empty(&pcity->worklist))
/* Nothing in the worklist; bail now. */
return FALSE;
i = 0;
- while (!success && worklist_peek_ith(&pcity->worklist, &target, i++)) {
- success = can_city_build_now(pcity, target);
+ while (!success) {
+
+ if (!city_checked) {
+ if (!city_exist(saved_id)) {
+ /* Some script has removed useless city that cannot build
+ * what it is told to! */
+ return FALSE;
+ }
+ city_checked = TRUE;
+ }
+
+ if (worklist_peek_ith(&pcity->worklist, &target, i++)) {
+ success = can_city_build_now(pcity, target);
+ } else {
+ success = FALSE;
+ }
if (success) {
break; /* while */
}
@@ -811,6 +830,7 @@
API_TYPE_UNIT_TYPE, ptarget,
API_TYPE_CITY, pcity,
API_TYPE_STRING, "need_tech");
+ city_checked = FALSE;
break;
}
success = can_city_build_unit_later(pcity, pupdate);
@@ -829,8 +849,13 @@
API_TYPE_UNIT_TYPE, ptarget,
API_TYPE_CITY, pcity,
API_TYPE_STRING, "never");
- /* Purge this worklist item. */
- worklist_remove(&pcity->worklist, --i);
+ if (city_exist(saved_id)) {
+ city_checked = TRUE;
+ /* Purge this worklist item. */
+ worklist_remove(&pcity->worklist, --i);
+ } else {
+ city_checked = FALSE;
+ }
} else {
/* Yep, we can go after pupdate instead. Joy! */
notify_player(pplayer, pcity->tile, E_WORKLIST,
@@ -860,9 +885,13 @@
API_TYPE_BUILDING_TYPE, ptarget,
API_TYPE_CITY, pcity,
API_TYPE_STRING, "never");
-
- /* Purge this worklist item. */
- worklist_remove(&pcity->worklist, --i);
+ if (city_exist(saved_id)) {
+ city_checked = TRUE;
+ /* Purge this worklist item. */
+ worklist_remove(&pcity->worklist, --i);
+ } else {
+ city_checked = FALSE;
+ }
break;
}
@@ -1012,7 +1041,16 @@
};
break;
}
+
+ /* Almost all cases emit signal in the end, so city check needed. */
+ if (!city_exist(saved_id)) {
+ /* Some script has removed city */
+ return FALSE;
+ }
+ city_checked = TRUE;
+
} requirement_vector_iterate_end;
+
if (!known) {
/* This shouldn't happen...
FIXME: make can_city_build_improvement_now() return a reason enum. */
@@ -1252,6 +1290,7 @@
bool space_part;
int mod;
struct impr_type *pimprove = pcity->production.value.building;
+ int saved_id = pcity->id;
if (city_production_has_flag(pcity, IF_GOLD)) {
assert(pcity->surplus[O_SHIELD] >= 0);
@@ -1326,6 +1365,11 @@
API_TYPE_BUILDING_TYPE, pimprove,
API_TYPE_CITY, pcity);
+ if (!city_exist(saved_id)) {
+ /* Script removed city */
+ return FALSE;
+ }
+
/* Call this function since some buildings may change the
* the vision range of a city */
city_refresh_vision(pcity);
@@ -1390,19 +1434,22 @@
_("%s is building %s, which is no longer available."),
city_name(pcity),
utype_name_translation(utype));
- script_signal_emit("unit_cant_be_built", 3,
- API_TYPE_UNIT_TYPE, utype,
- API_TYPE_CITY, pcity,
- API_TYPE_STRING, "unavailable");
+
+ /* Log before signal emitting, so pointers are certainly valid */
freelog(LOG_VERBOSE, "%s %s tried to build %s, which is not available.",
nation_rule_name(nation_of_city(pcity)),
city_name(pcity),
utype_rule_name(utype));
+ script_signal_emit("unit_cant_be_built", 3,
+ API_TYPE_UNIT_TYPE, utype,
+ API_TYPE_CITY, pcity,
+ API_TYPE_STRING, "unavailable");
return TRUE;
}
if (pcity->shield_stock >= utype_build_shield_cost(utype)) {
int pop_cost = utype_pop_value(utype);
struct unit *punit;
+ int saved_city_id = pcity->id;
/* Should we disband the city? -- Massimo */
if (pcity->size == pop_cost
@@ -1452,8 +1499,10 @@
script_signal_emit("unit_built",
2, API_TYPE_UNIT, punit, API_TYPE_CITY, pcity);
- /* Done building this unit; time to move on to the next. */
- choose_build_target(pplayer, pcity);
+ if (city_exist(saved_city_id)) {
+ /* Done building this unit; time to move on to the next. */
+ choose_build_target(pplayer, pcity);
+ }
}
return TRUE;
}
@@ -1746,6 +1795,7 @@
struct tile *ptile = pcity->tile;
struct city *rcity=NULL;
struct unit_type *utype = pcity->production.value.utype;
+ int saved_id = pcity->id;
/* find closest city other than pcity */
rcity = find_closest_owned_city(pplayer, ptile, FALSE, pcity);
@@ -1761,7 +1811,12 @@
API_TYPE_UNIT_TYPE, utype,
API_TYPE_CITY, pcity,
API_TYPE_STRING, "pop_cost");
- return FALSE;
+ if (!city_exist(saved_id)) {
+ /* Script decided to remove even the last city */
+ return TRUE;
+ } else {
+ return FALSE;
+ }
}
(void) create_unit(pplayer, ptile, utype,
diff -Nurd -X.diff_ignore freeciv/server/unittools.c freeciv/server/unittools.c
--- freeciv/server/unittools.c 2008-03-12 21:58:28.000000000 +0200
+++ freeciv/server/unittools.c 2008-03-24 16:06:24.000000000 +0200
@@ -2255,7 +2255,9 @@
{
struct player *pplayer = unit_owner(punit);
enum hut_behavior behavior = unit_class(punit)->hut_behavior;
-
+
+ /* FIXME: Should we still run "hut_enter" script when
+ * hut_behavior is HUT_NOTHING or HUT_FRIGHTEN? */
if (behavior == HUT_NOTHING) {
return;
}
@@ -2642,6 +2644,7 @@
struct unit *ptransporter = NULL;
struct vision *old_vision = punit->server.vision;
struct vision *new_vision;
+ int saved_id = punit->id;
conn_list_do_buffer(pplayer->connections);
@@ -2811,10 +2814,17 @@
} square_iterate_end;
unit_move_consequences(punit, psrctile, pdesttile);
+
+ /* FIXME: Should signal emit be after sentried units have been
+ * waken up in case script causes unit death. */
script_signal_emit("unit_moved", 3,
API_TYPE_UNIT, punit,
API_TYPE_TILE, psrctile,
API_TYPE_TILE, pdesttile);
+ if (!unit_alive(saved_id)) {
+ /* Script caused unit to die */
+ return FALSE;
+ }
wakeup_neighbor_sentries(punit);
if (!unit_survive_autoattack(punit)) {
conn_list_do_unbuffer(pplayer->connections);
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
