From 7912987cb4d95d92c1363bac39116a701255ab9b Mon Sep 17 00:00:00 2001 From: Ihnatus Date: Fri, 6 May 2022 00:18:18 +0300 Subject: [PATCH] Catch pointers dangling after signals (other than require resolving in HRM#849859) See OSDN#44312 Signed-off-by: Ihnatus --- ai/default/aicity.c | 15 ++- ai/default/aiferry.c | 3 +- ai/default/aihunt.c | 26 +++-- ai/default/aisettler.c | 9 +- ai/default/aitools.c | 1 + ai/default/aiunit.c | 4 + server/barbarian.c | 2 + server/citytools.c | 37 ++++--- server/citytools.h | 10 +- server/cityturn.c | 164 ++++++++++++++++++----------- server/diplomats.c | 158 ++++++++++++++++++--------- server/edithand.c | 14 +-- server/gamehand.c | 2 +- server/maphand.c | 4 + server/plrhand.c | 35 +++--- server/ruleset.c | 1 + server/scripting/api_server_edit.c | 10 +- server/techtools.c | 16 +-- server/unithand.c | 84 +++++++++------ server/unittools.c | 15 ++- 20 files changed, 395 insertions(+), 215 deletions(-) diff --git a/ai/default/aicity.c b/ai/default/aicity.c index 6aec2912dd..ce7e3514ab 100644 --- a/ai/default/aicity.c +++ b/ai/default/aicity.c @@ -379,6 +379,7 @@ static void dai_upgrade_units(struct city *pcity, int limit, bool military) dai_calc_data(pplayer, NULL, &expenses, NULL); + /* FIXME: thaw signals per HRM#849859, or check if all right each time */ unit_list_iterate(pcity->tile->units, punit) { if (pcity->owner == punit->owner) { /* Only upgrade units you own, not allied ones */ @@ -527,6 +528,7 @@ static void dai_spend_gold(struct ai_type *ait, struct player *pplayer) int buycost; int limit = cached_limit; /* cached_limit is our gold reserve */ struct city *pcity = NULL; + int cityid; struct ai_city *city_data; /* Find highest wanted item on the buy list */ @@ -548,6 +550,7 @@ static void dai_spend_gold(struct ai_type *ait, struct player *pplayer) break; } + cityid = pcity->id; city_data = def_ai_city_data(pcity, ait); /* Not dealing with this city a second time */ @@ -569,6 +572,10 @@ static void dai_spend_gold(struct ai_type *ait, struct player *pplayer) } /* Upgrade only military units now */ dai_upgrade_units(pcity, upgrade_limit, TRUE); + if (!player_city_by_number(pplayer, cityid)) { + /* Destroyed or lost */ + continue; + } } } @@ -655,9 +662,11 @@ static void dai_spend_gold(struct ai_type *ait, struct player *pplayer) if (!war_footing) { /* Civilian upgrades now */ - city_list_iterate(pplayer->cities, pcity) { - dai_upgrade_units(pcity, cached_limit, FALSE); - } city_list_iterate_end; + city_list_iterate_safe(pplayer->cities, pcity) { + if (city_owner(pcity) == pplayer) { + dai_upgrade_units(pcity, cached_limit, FALSE); + } + } city_list_iterate_safe_end; } log_base(LOG_BUY, "%s wants to keep %d in reserve (tax factor %d)", diff --git a/ai/default/aiferry.c b/ai/default/aiferry.c index 0d3fae74a6..ba75e53449 100644 --- a/ai/default/aiferry.c +++ b/ai/default/aiferry.c @@ -667,7 +667,8 @@ bool dai_amphibious_goto_constrained(struct ai_type *ait, if (path) { dai_log_path(passenger, path, ¶meter->combined); /* Sea leg */ - alive = adv_follow_path(ferry, path, ptile); + alive = adv_follow_path(ferry, path, ptile) + && unit_is_alive(pass_id); if (alive && unit_tile(passenger) != ptile) { /* Ferry has stopped; it is at the landing beach or * has run out of movement points */ diff --git a/ai/default/aihunt.c b/ai/default/aihunt.c index 3f51c08a36..c4d14c5f89 100644 --- a/ai/default/aihunt.c +++ b/ai/default/aihunt.c @@ -301,15 +301,17 @@ bool dai_hunter_qualify(struct player *pplayer, struct unit *punit) Try to shoot our target with a missile. Also shoot down anything that might attempt to intercept _us_. We assign missiles to a hunter in ai_unit_new_role(). + Returns TRUE iff punit lives **************************************************************************/ -static void dai_hunter_try_launch(struct ai_type *ait, +static bool dai_hunter_try_launch(struct ai_type *ait, struct player *pplayer, struct unit *punit, struct unit *target) { - int target_sanity = target->id; + int target_sanity = target->id, hunter_id = punit->id; struct pf_parameter parameter; struct pf_map *pfm; + struct unit_type *ptype = unit_type_get(punit); unit_list_iterate_safe(unit_tile(punit)->units, missile) { struct unit *sucker = NULL; @@ -323,6 +325,8 @@ static void dai_hunter_try_launch(struct ai_type *ait, pfm = pf_map_new(¶meter); pf_map_move_costs_iterate(pfm, ptile, move_cost, FALSE) { + /* FIXME: Why divide on SINGLE_MOVE? We might just subtract it + * when we are bothered by tired attack */ if (move_cost > missile->moves_left / SINGLE_MOVE) { break; } @@ -333,7 +337,6 @@ static void dai_hunter_try_launch(struct ai_type *ait, unit_list_iterate(ptile->units, victim) { enum diplstate_type ds = player_diplstate_get(pplayer, unit_owner(victim))->type; - struct unit_type *ptype; struct unit_type *victim_type; if (ds != DS_WAR) { @@ -348,7 +351,6 @@ static void dai_hunter_try_launch(struct ai_type *ait, } victim_type = unit_type_get(victim); - ptype = unit_type_get(punit); if (ATTACK_POWER(victim_type) > DEFENSE_POWER(ptype) && dai_unit_can_strike_my_unit(victim, punit)) { @@ -376,11 +378,19 @@ static void dai_hunter_try_launch(struct ai_type *ait, dai_unit_attack(ait, missile, unit_tile(sucker)); } } - target = game_unit_by_number(target_sanity); /* Sanity */ + if (target) { + target = game_unit_by_number(target_sanity); /* Sanity */ + } + if (!unit_is_alive(hunter_id)) { + return FALSE; /* May be destroyed by a script */ + /* FIXME: our units might also be scattered from the tile */ + } break; /* try next missile, if any */ } } /* if */ } unit_list_iterate_safe_end; + + return TRUE; } /************************************************************************** @@ -556,7 +566,11 @@ int dai_hunter_manage(struct ai_type *ait, struct player *pplayer, dai_unit_new_task(ait, punit, AIUNIT_HUNTER, unit_tile(target)); /* Check if we can nuke it */ - dai_hunter_try_launch(ait, pplayer, punit, target); + if (!dai_hunter_try_launch(ait, pplayer, punit, target)) { + /* Died during rocket fight, probably by script */ + pf_map_destroy(pfm); + return 0; + } /* Check if we have nuked it */ if (target != game_unit_by_number(sanity_target)) { diff --git a/ai/default/aisettler.c b/ai/default/aisettler.c index 3f57cd22f3..ec9c61e11e 100644 --- a/ai/default/aisettler.c +++ b/ai/default/aisettler.c @@ -1243,6 +1243,7 @@ static enum cb_error_level dai_do_build_city(struct ai_type *ait, { struct tile *ptile = unit_tile(punit); struct city *pcity; + int id = punit->id; fc_assert_ret_val(pplayer == unit_owner(punit), FALSE); unit_activity_handling(punit, ACTIVITY_IDLE); @@ -1262,7 +1263,7 @@ static enum cb_error_level dai_do_build_city(struct ai_type *ait, 0, city_name_suggestion(pplayer, ptile), ACTION_FOUND_CITY); pcity = tile_city(ptile); - if (!pcity && punit) { + if (!pcity && unit_is_alive(id)) { enum ane_kind reason = action_not_enabled_reason(punit, ACTION_FOUND_CITY, ptile, NULL, NULL); @@ -1273,8 +1274,9 @@ static enum cb_error_level dai_do_build_city(struct ai_type *ait, log_debug("%s: Failed to build city at (%d, %d)", player_name(pplayer), TILE_XY(ptile)); } else { - /* The request was illegal to begin with. */ - log_error("%s: Failed to build city at (%d, %d). Reason id: %d", + /* The request was illegal to begin with. + * Or became one during action_started_unit_tile that we could not foresee */ + log_error("%s: Failed to build city at (%d, %d). Reason id: %d (caused by scripting?)", player_name(pplayer), TILE_XY(ptile), reason); return CBE_FATAL; } @@ -1284,6 +1286,7 @@ static enum cb_error_level dai_do_build_city(struct ai_type *ait, /* We have to rebuild at least the cache for this city. This event is * rare enough we might as well build the whole thing. Who knows what * else might be cached in the future? */ + /* NOTE: in future versions, action_finished_unit_tile callback may meddle */ fc_assert_ret_val(pplayer == city_owner(pcity), FALSE); initialize_infrastructure_cache(pplayer); diff --git a/ai/default/aitools.c b/ai/default/aitools.c index c5ff7be390..6258007004 100644 --- a/ai/default/aitools.c +++ b/ai/default/aitools.c @@ -766,6 +766,7 @@ void dai_unit_new_task(struct ai_type *ait, struct unit *punit, /************************************************************************** Try to make pcity our new homecity. Fails if we can't upkeep it. Assumes success from server. + FIXME: what do we return here? Whatever, it is ignored. **************************************************************************/ bool dai_unit_make_homecity(struct unit *punit, struct city *pcity) { diff --git a/ai/default/aiunit.c b/ai/default/aiunit.c index 4153e66f93..ca165ba3da 100644 --- a/ai/default/aiunit.c +++ b/ai/default/aiunit.c @@ -2212,7 +2212,11 @@ static bool search_homecity_for_caravan(struct ai_type *ait, struct unit *punit) if (nearest != NULL) { alive = dai_unit_goto(ait, punit, nearest->tile); if (alive && same_pos(unit_tile(punit), nearest->tile)) { + int id = punit->id; + dai_unit_make_homecity(punit, nearest); + /* May die from a callback */ + alive = unit_is_alive(id); } } diff --git a/server/barbarian.c b/server/barbarian.c index 02d2ab6562..8a57ab18d0 100644 --- a/server/barbarian.c +++ b/server/barbarian.c @@ -251,6 +251,8 @@ static int random_unchecked_direction(int possibilities, const bool *checked) boats, otherwise (not much land and no sea) kill enemy unit and stay in a village. The return value indicates if the explorer survived entering the vilage. + + FIXME: errors may occur if not freeze callbacks per HRM#849859 **************************************************************************/ bool unleash_barbarians(struct tile *ptile) { diff --git a/server/citytools.c b/server/citytools.c index e2e57d3472..4a0e6786cd 100644 --- a/server/citytools.c +++ b/server/citytools.c @@ -689,13 +689,15 @@ static void transfer_unit(struct unit *punit, struct city *tocity, /********************************************************************* When a city is transferred (bought, incited, disbanded, civil war): - Units in a transferred city are transferred to the new owner; units + Units in a transferred city are transferred to the new owner; units supported by the city, but held in other cities are updated to - reflect those cities as their new homecity. Units supported + reflect those cities as their new homecity. Units supported by the transferred city that are not in a city tile may be deleted. - Kris Bubendorfer +FIXME: thaw signals or ensure they are already thawed per HRM#849859. + pplayer: The player receiving the units if they are not disbanded and are not in a city pvictim: The owner of the city the units are transferred from. @@ -1449,10 +1451,10 @@ void city_build_free_buildings(struct city *pcity) } /************************************************************************** - Creates real city. + Creates real city. size must be a valid size. **************************************************************************/ void create_city(struct player *pplayer, struct tile *ptile, - const char *name, struct player *nationality) + const char *name, struct player *nationality, citizens size) { struct player *saved_owner = tile_owner(ptile); struct tile *saved_claimer = tile_claimer(ptile); @@ -1484,6 +1486,7 @@ void create_city(struct player *pplayer, struct tile *ptile, tile_set_owner(ptile, pplayer, ptile); /* temporarily */ city_choose_build_default(pcity); pcity->id = identity_number(); + city_size_set(pcity, size); fc_allocate_mutex(&game.server.mutexes.city_list); idex_register_city(pcity); @@ -1607,6 +1610,8 @@ void create_city(struct player *pplayer, struct tile *ptile, /************************************************************************** Remove a city from the game. + FIXME: it's better to freeze the callbacks bouncing ships around + per HRM#849859. **************************************************************************/ void remove_city(struct city *pcity) { @@ -1662,14 +1667,18 @@ void remove_city(struct city *pcity) adjc_iterate(pcenter, tile1) { if (!moved && is_native_tile(punittype, tile1)) { if (adv_could_unit_move_to_tile(punit, tile1) == 1) { + struct player *owner = unit_owner(punit); + char ulink[MAX_LEN_LINK]; + + sz_strlcpy(ulink, unit_link(punit)); /* Move */ moved = unit_move_handling(punit, tile1, FALSE, TRUE, NULL); if (moved) { - notify_player(unit_owner(punit), tile1, + notify_player(owner, tile1, E_UNIT_RELOCATED, ftc_server, _("Moved %s out of disbanded city %s " "since it cannot stay on %s."), - unit_link(punit), ctl, + ulink, ctl, terrain_name_translation(tile_terrain(pcenter))); break; } @@ -1868,6 +1877,7 @@ bool unit_conquer_city(struct unit *punit, struct city *pcity) int coins; struct player *pplayer = unit_owner(punit); struct player *cplayer = city_owner(pcity); + int saved_id = pcity->id; /* If not at war, may peacefully enter city. */ fc_assert_ret_val_msg(pplayers_at_war(pplayer, cplayer), FALSE, @@ -1905,18 +1915,16 @@ bool unit_conquer_city(struct unit *punit, struct city *pcity) try_civil_war = TRUE; } - /* + /* * We later remove a citizen. Lets check if we can save this since * the city will be destroyed. */ if (city_size_get(pcity) <= 1) { - int saved_id = pcity->id; - notify_player(pplayer, city_tile(pcity), E_UNIT_WIN_ATT, ftc_server, _("You destroy %s completely."), city_tile_link(pcity)); notify_player(cplayer, city_tile(pcity), E_CITY_LOST, ftc_server, - _("%s has been destroyed by %s."), + _("%s has been destroyed by %s."), city_tile_link(pcity), player_name(pplayer)); script_server_signal_emit("city_destroyed", pcity, cplayer, pplayer); @@ -2002,13 +2010,14 @@ bool unit_conquer_city(struct unit *punit, struct city *pcity) * the size is reduced. */ /* FIXME: maybe it should be a ruleset option whether barbarians get * free buildings such as palaces? */ - city_remains = transfer_city(pplayer, pcity, 0, TRUE, TRUE, TRUE, - !is_barbarian(pplayer)); + city_remains = player_city_by_number(pplayer, saved_id) + && transfer_city(pplayer, pcity, 0, TRUE, TRUE, TRUE, + !is_barbarian(pplayer)); if (city_remains) { /* reduce size should not destroy this city */ fc_assert(city_size_get(pcity) > 1); - city_reduce_size(pcity, 1, pplayer, "conquest"); + city_remains = city_reduce_size(pcity, 1, pplayer, "conquest"); } if (try_civil_war) { @@ -2018,7 +2027,7 @@ bool unit_conquer_city(struct unit *punit, struct city *pcity) if (city_remains) { script_server_signal_emit("city_transferred", pcity, cplayer, pplayer, "conquest"); - script_server_signal_emit("city_lost", pcity, cplayer, pplayer); + script_server_signal_emit("city_lost", game_city_by_number(saved_id), cplayer, pplayer); } return TRUE; diff --git a/server/citytools.h b/server/citytools.h index 3abbf3e04b..543911e4cb 100644 --- a/server/citytools.h +++ b/server/citytools.h @@ -29,10 +29,10 @@ int build_points_left(struct city *pcity); -void transfer_city_units(struct player *pplayer, struct player *pvictim, - struct unit_list *units, struct city *pcity, - struct city *exclude_city, - int kill_outside, bool verbose); +void transfer_city_units(struct player *pplayer, struct player *pvictim, + struct unit_list *units, struct city *pcity, + struct city *exclude_city, + int kill_outside, bool verbose); bool transfer_city(struct player *ptaker, struct city *pcity, int kill_outside, bool transfer_unit_verbose, bool resolve_stack, bool raze, bool build_free); @@ -64,7 +64,7 @@ void remove_dumb_city(struct player *pplayer, struct tile *ptile); void city_build_free_buildings(struct city *pcity); void create_city(struct player *pplayer, struct tile *ptile, - const char *name, struct player *nationality); + const char *name, struct player *nationality, citizens size); void remove_city(struct city *pcity); struct trade_route *remove_trade_route(struct city *pc1, diff --git a/server/cityturn.c b/server/cityturn.c index 08cb306bbd..6d61c03c70 100644 --- a/server/cityturn.c +++ b/server/cityturn.c @@ -564,7 +564,10 @@ void update_city_activities(struct player *pplayer) pplayer->server.bulbs_last_turn = 0; if (n > 0) { - struct city *cities[n]; + struct { + struct city *pcity; + int id; + } cities[n]; int i = 0, r; city_list_iterate(pplayer->cities, pcity) { @@ -602,7 +605,8 @@ void update_city_activities(struct player *pplayer) } trade_routes_iterate_safe_end; /* Add cities to array for later random order handling */ - cities[i++] = pcity; + cities[i].pcity = pcity; + cities[i++].id = pcity->id; } city_list_iterate_end; /* How gold upkeep is handled depends on the setting @@ -619,9 +623,12 @@ void update_city_activities(struct player *pplayer) /* Iterate over cities in a random order. */ while (i > 0) { r = fc_rand(i); - /* update unit upkeep */ - city_units_upkeep(cities[r]); - update_city_activity(cities[r]); + /* update unit upkeep. City could be destroyed or transferred */ + /* Some cities that change hands may pass the turn in result. */ + if (NULL != player_city_by_number(pplayer, cities[r].id)) { + city_units_upkeep(cities[r].pcity); + update_city_activity(cities[r].pcity); + } cities[r] = cities[--i]; } @@ -754,6 +761,7 @@ bool city_reduce_size(struct city *pcity, citizens pop_loss, { citizens loss_remain; int old_radius_sq; + int saved_id = pcity->id; if (pop_loss == 0) { return TRUE; @@ -764,7 +772,9 @@ bool city_reduce_size(struct city *pcity, citizens pop_loss, script_server_signal_emit("city_destroyed", pcity, pcity->owner, destroyer); - remove_city(pcity); + if (city_exist(saved_id)) { + remove_city(pcity); + } return FALSE; } old_radius_sq = tile_border_source_radius_sq(pcity->tile); @@ -815,11 +825,9 @@ bool city_reduce_size(struct city *pcity, citizens pop_loss, sanity_check_city(pcity); if (reason != NULL) { - int id = pcity->id; - script_server_signal_emit("city_size_change", pcity, -pop_loss, reason); - return city_exist(id); + return city_exist(saved_id); } return TRUE; @@ -1026,12 +1034,13 @@ static void city_populate(struct city *pcity, struct player *nationality) { int saved_id = pcity->id; int granary_size = city_granary_size(city_size_get(pcity)); + struct player *owner = city_owner(pcity); + struct tile *ctile = city_tile(pcity); pcity->food_stock += pcity->surplus[O_FOOD]; if (pcity->food_stock >= granary_size || city_rapture_grow(pcity)) { if (city_had_recent_plague(pcity)) { - notify_player(city_owner(pcity), city_tile(pcity), - E_CITY_PLAGUE, ftc_server, + notify_player(owner, ctile, E_CITY_PLAGUE, ftc_server, _("A recent plague outbreak prevents growth in %s."), city_link(pcity)); /* Lose excess food */ @@ -1040,7 +1049,7 @@ static void city_populate(struct city *pcity, struct player *nationality) bool success; success = city_increase_size(pcity, nationality); - map_claim_border(pcity->tile, pcity->owner, -1); + map_claim_border(ctile, owner, -1); if (success) { script_server_signal_emit("city_size_change", pcity, 1, "growth"); @@ -1056,14 +1065,17 @@ static void city_populate(struct city *pcity, struct player *nationality) */ unit_list_iterate_safe(pcity->units_supported, punit) { if (punit->upkeep[O_FOOD] > 0) { - const char *punit_link = unit_tile_link(punit); + char utlink[MAX_LEN_LINK], clink[MAX_LEN_LINK]; + sz_strlcpy(utlink, unit_tile_link(punit)); + sz_strlcpy(clink, city_link(pcity)); if (upkeep_kill_unit(punit, O_FOOD, ULR_STARVED, game.info.muuk_food_wipe)) { - notify_player(city_owner(pcity), city_tile(pcity), - E_UNIT_LOST_MISC, ftc_server, + /* Script may destroy or transfer the city, + * but it is called to original owner who's thus notified */ + notify_player(owner, ctile, E_UNIT_LOST_MISC, ftc_server, _("Famine feared in %s, %s lost!"), - city_link(pcity), punit_link); + clink, utlink); } if (city_exist(saved_id)) { @@ -1808,8 +1820,8 @@ static bool worklist_item_postpone_req_vec(struct universal *target, } /************************************************************************** - Examine the worklist and change the build target. Return 0 if no - targets are available to change to. Otherwise return non-zero. Has + Examine the worklist and change the build target. Return FALSE if no + targets are available to change to. Otherwise return TRUE. Has the side-effect of removing from the worklist any no-longer-available targets as well as the target actually selected, if any. **************************************************************************/ @@ -1859,7 +1871,7 @@ static bool worklist_change_build_target(struct player *pplayer, /* Maybe we can just upgrade the target to what the city /can/ build. */ if (U_NOT_OBSOLETED == pupdate) { - /* Nope, we're stuck. Skip this item from the worklist. */ + /* Nope, we're stuck. Skip this item from the worklist. */ if (ptarget->need_government != NULL && ptarget->need_government != government_of_player(pplayer)) { notify_player(pplayer, city_tile(pcity), @@ -1902,20 +1914,20 @@ static bool worklist_change_build_target(struct player *pplayer, city_link(pcity), utype_name_translation(ptarget)); } city_checked = FALSE; - break; - } + break; /* switch (target.kind) */ + } /* if (U_NOT_OBSOLETED == pupdate) */ success = can_city_build_unit_later(pcity, pupdate); if (!success) { - /* If the city can never build this unit or its descendants, - * drop it. */ - notify_player(pplayer, city_tile(pcity), + /* If the city can never build this unit or its descendants, + * drop it. */ + notify_player(pplayer, city_tile(pcity), E_CITY_CANTBUILD, ftc_server, _("%s can't build %s from the worklist. Purging..."), city_link(pcity), - /* Yes, warn about the targets that's actually - in the worklist, not its obsolete-closure - pupdate. */ - utype_name_translation(ptarget)); + /* Yes, warn about the targets that's actually + in the worklist, not its obsolete-closure + pupdate. */ + utype_name_translation(ptarget)); script_server_signal_emit("unit_cant_be_built", ptarget, pcity, "never"); if (city_exist(saved_id)) { @@ -1924,19 +1936,19 @@ static bool worklist_change_build_target(struct player *pplayer, i--; worklist_remove(pwl, i); } else { - city_checked = FALSE; + return FALSE; } } else { - /* Yep, we can go after pupdate instead. Joy! */ + /* Yep, we can go after pupdate instead. Joy! */ notify_player(pplayer, city_tile(pcity), E_WORKLIST, ftc_server, _("Production of %s is upgraded to %s in %s."), utype_name_translation(ptarget), utype_name_translation(pupdate), city_link(pcity)); - target.value.utype = pupdate; - } + target.value.utype = pupdate; + } /*if (!success) ... else */ break; - } + } /* case VUT_UTYPE */ case VUT_IMPROVEMENT: { struct impr_type *ptarget = target.value.building; @@ -1965,17 +1977,17 @@ static bool worklist_change_build_target(struct player *pplayer, city_checked = TRUE; } } else if (success) { - /* Hey, we can upgrade the improvement! */ + /* Hey, we can upgrade the improvement! */ notify_player(pplayer, city_tile(pcity), E_WORKLIST, ftc_server, _("Production of %s is upgraded to %s in %s."), city_improvement_name_translation(pcity, ptarget), city_improvement_name_translation(pcity, pupdate), city_link(pcity)); - target.value.building = pupdate; - } + target.value.building = pupdate; + } /* if (purge) ... else if (success)*/ if (purge) { - /* Never in a million years. */ + /* Never in a million years. */ notify_player(pplayer, city_tile(pcity), E_CITY_CANTBUILD, ftc_server, _("%s can't build %s from the worklist. Purging..."), @@ -1989,17 +2001,17 @@ static bool worklist_change_build_target(struct player *pplayer, i--; worklist_remove(pwl, i); } else { - city_checked = FALSE; + return FALSE; } - } + } /* if (purge) */ break; - } + } /* case VUT_IMPROVEMENT */ default: /* skip useless target */ log_error("worklist_change_build_target() has unrecognized " "target kind (%d)", target.kind); break; - }; + }; /* switch (target.kind) */ } /* while */ if (success) { @@ -2011,7 +2023,8 @@ static bool worklist_change_build_target(struct player *pplayer, worklist_remove(pwl, i - 1); } - if (worklist_is_empty(pwl)) { + if ((city_checked || success || city_exist(saved_id)) + && worklist_is_empty(pwl)) { /* There *was* something in the worklist, but it's empty now. Bug the player about it. */ notify_player(pplayer, city_tile(pcity), E_WORKLIST, ftc_server, @@ -2161,23 +2174,31 @@ static void upgrade_unit_prod(struct city *pcity) static bool city_distribute_surplus_shields(struct player *pplayer, struct city *pcity) { + struct tile *ctile = city_tile(pcity); + int cid = pcity->id; + char clink[MAX_LEN_LINK]; + + sz_strlcpy(clink, city_link(pcity)); if (pcity->surplus[O_SHIELD] < 0) { unit_list_iterate_safe(pcity->units_supported, punit) { if (utype_upkeep_cost(unit_type_get(punit), pplayer, O_SHIELD) > 0 && pcity->surplus[O_SHIELD] < 0) { - const char *punit_link = unit_link(punit); + /* City and unit may be destroyed, links overwritten by Lua */ + char ulink[MAX_LEN_LINK]; /* TODO: Should the unit try to help cities on adjacent tiles? That * would be a rules change. (This action is performed by the game * it self) */ + sz_strlcpy(ulink, unit_link(punit)); if (upkeep_kill_unit(punit, O_SHIELD, ULR_DISBANDED, game.info.muuk_shield_wipe)) { - notify_player(pplayer, city_tile(pcity), - E_UNIT_LOST_MISC, ftc_server, + notify_player(pplayer, ctile, E_UNIT_LOST_MISC, ftc_server, _("%s can't upkeep %s, unit disbanded."), - city_link(pcity), punit_link); + clink, ulink); + } + if (!city_exist(cid)) { + return FALSE; } - /* pcity->surplus[O_SHIELD] is automatically updated. */ } } unit_list_iterate_safe_end; @@ -2191,18 +2212,22 @@ static bool city_distribute_surplus_shields(struct player *pplayer, unit_list_iterate_safe(pcity->units_supported, punit) { int upkeep = utype_upkeep_cost(unit_type_get(punit), pplayer, O_SHIELD); + if (punit->homecity != cid) { + /* Transferred by some script. Units transferred _in_ are ignored */ + continue; + } if (upkeep > 0 && pcity->surplus[O_SHIELD] < 0) { - notify_player(pplayer, city_tile(pcity), + notify_player(pplayer, ctile, E_UNIT_LOST_MISC, ftc_server, _("Citizens in %s perish for their failure to " "upkeep %s!"), - city_link(pcity), unit_link(punit)); - if (!city_reduce_size(pcity, 1, NULL, "upkeep_failure")) { - return FALSE; - } + clink, unit_link(punit)); + if (!city_reduce_size(pcity, 1, NULL, "upkeep_failure")) { + return FALSE; + } - /* No upkeep for the unit this turn. */ - pcity->surplus[O_SHIELD] += upkeep; + /* No upkeep for the unit this turn. */ + pcity->surplus[O_SHIELD] += upkeep; } } unit_list_iterate_safe_end; } @@ -2744,6 +2769,7 @@ static struct unit *sell_random_unit(struct player *pplayer, /************************************************************************** Balance the gold of a nation by selling some random units and buildings. + FIXME: callbacks must be frozen during the loop per HRM#849859 **************************************************************************/ static bool player_balance_treasury_units_and_buildings (struct player *pplayer) @@ -3127,6 +3153,7 @@ static void update_city_activity(struct city *pcity) int saved_id; int revolution_turns; + pplayer = city_owner(pcity); /* could change hands */ pcity->history += city_history_gain(pcity); /* History can decrease, but never go below zero */ @@ -3184,14 +3211,18 @@ static void update_city_activity(struct city *pcity) /* City population updated here, after the rapture stuff above. --Jing */ saved_id = pcity->id; city_populate(pcity, pplayer); - if (NULL == player_city_by_number(pplayer, saved_id)) { + if (!city_exist(saved_id)) { return; } + pplayer = city_owner(pcity); /* could change hands */ pcity->did_sell = FALSE; pcity->did_buy = FALSE; pcity->airlift = city_airlift_max(pcity); update_bulbs(pplayer, pcity->prod[O_SCIENCE], FALSE); + if (!city_exist(saved_id)) { + return; + } /* Update the treasury. */ pplayer->economic.gold += pcity->prod[O_GOLD]; @@ -3218,6 +3249,10 @@ static void update_city_activity(struct city *pcity) && game.info.gold_upkeep_style == GOLD_UPKEEP_CITY) { city_balance_treasury_units(pcity); } + if (!player_city_by_number(pplayer, saved_id)) { + /* City is destroyed or transferred by a callback */ + return; + } break; case GOLD_UPKEEP_NATION: break; @@ -3342,7 +3377,9 @@ static bool disband_city(struct city *pcity) script_server_signal_emit("city_destroyed", pcity, pcity->owner, NULL); - remove_city(pcity); + if (city_exist(saved_id)) { + remove_city(pcity); + } /* Since we've removed the city, we don't need to worry about * charging for production, etc. */ @@ -3636,6 +3673,8 @@ static bool do_city_migration(struct city *pcity_from, return FALSE; } } else { + int fro_id = pcity_from->id; + /* the migrants take half of the food box with them (this prevents * migration -> grow -> migration -> ... cycles) */ pcity_from->food_stock /= 2; @@ -3655,9 +3694,11 @@ static bool do_city_migration(struct city *pcity_from, citizens_nation_add(pcity_from, pplayer_citizen->slot, -1); } city_reduce_size(pcity_from, 1, pplayer_from, "migration_from"); - city_refresh_vision(pcity_from); - if (city_refresh(pcity_from)) { - auto_arrange_workers(pcity_from); + if (city_exist(fro_id)) { + city_refresh_vision(pcity_from); + if (city_refresh(pcity_from)) { + auto_arrange_workers(pcity_from); + } } } @@ -3784,6 +3825,7 @@ static void apply_disaster(struct city *pcity, struct disaster_type *pdis) struct player *pplayer = city_owner(pcity); struct tile *ptile = city_tile(pcity); bool had_internal_effect = FALSE; + int id = pcity->id; log_debug("%s at %s", disaster_rule_name(pdis), city_name_get(pcity)); @@ -3880,7 +3922,7 @@ static void apply_disaster(struct city *pcity, struct disaster_type *pdis) script_server_signal_emit("disaster_occurred", pdis, pcity, had_internal_effect); - script_server_signal_emit("disaster", pdis, pcity); + script_server_signal_emit("disaster", pdis, game_city_by_number(id)); } /************************************************************************** @@ -3893,6 +3935,8 @@ void check_disasters(void) return; } + /* FIXME: if a city changes hands during a callback, + * it may be processed again */ players_iterate(pplayer) { /* Safe city iterator needed as disaster may destroy city */ city_list_iterate_safe(pplayer->cities, pcity) { diff --git a/server/diplomats.c b/server/diplomats.c index 5e389120e6..8dd22eb1bc 100644 --- a/server/diplomats.c +++ b/server/diplomats.c @@ -97,19 +97,23 @@ bool spy_poison(struct player *pplayer, struct unit *pdiplomat, { struct player *cplayer; struct tile *ctile; - const char *clink; + char clink[MAX_LEN_LINK], ulink[MAX_LEN_LINK]; + int cid, uid; /* Fetch target city's player. Sanity checks. */ fc_assert_ret_val(pcity, FALSE); cplayer = city_owner(pcity); + cid = pcity->id; fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ fc_assert_ret_val(pplayer, FALSE); fc_assert_ret_val(pdiplomat, FALSE); + uid = pdiplomat->id; ctile = city_tile(pcity); - clink = city_link(pcity); + sz_strlcpy(clink, city_link(pcity)); + sz_strlcpy(ulink, unit_link(pdiplomat)); log_debug("poison: unit: %d", pdiplomat->id); @@ -127,25 +131,27 @@ bool spy_poison(struct player *pplayer, struct unit *pdiplomat, /* Notify everybody involved. */ notify_player(pplayer, ctile, E_MY_DIPLOMAT_POISON, ftc_server, _("Your %s poisoned the water supply of %s."), - unit_link(pdiplomat), clink); + ulink, clink); notify_player(cplayer, ctile, E_ENEMY_DIPLOMAT_POISON, ftc_server, _("%s is suspected of poisoning the water supply of %s."), player_name(pplayer), clink); - if (game.info.poison_empties_food_stock) { - /* The food was poisoned too. */ - city_empty_food_stock(pcity); - } + if (city_exist(cid)) { + if (game.info.poison_empties_food_stock) { + /* The food was poisoned too. */ + city_empty_food_stock(pcity); + } - /* Update clients. */ - city_refresh (pcity); - send_city_info(NULL, pcity); + /* Update clients. */ + city_refresh (pcity); + send_city_info(NULL, pcity); + } } else { /* Notify everybody involved. */ notify_player(pplayer, ctile, E_MY_DIPLOMAT_POISON, ftc_server, _("Your %s destroyed %s by poisoning its water supply."), - unit_link(pdiplomat), clink); + ulink, clink); notify_player(cplayer, ctile, E_ENEMY_DIPLOMAT_POISON, ftc_server, _("%s is suspected of destroying %s by poisoning its" @@ -157,7 +163,9 @@ bool spy_poison(struct player *pplayer, struct unit *pdiplomat, action_consequence_success(paction, pplayer, cplayer, ctile, clink); /* Now lets see if the spy survives. */ - diplomat_escape_full(pplayer, pdiplomat, TRUE, ctile, clink, paction); + if (player_unit_by_number(pplayer, uid)) { + diplomat_escape_full(pplayer, pdiplomat, TRUE, ctile, clink, paction); + } return TRUE; } @@ -448,7 +456,7 @@ bool spy_sabotage_unit(struct player *pplayer, struct unit *pdiplomat, /****************************************************************************** Bribe an enemy unit. - + - Can't bribe a unit if: - Player doesn't have enough gold. - Otherwise, the unit will be bribed. @@ -636,16 +644,23 @@ bool diplomat_get_tech(struct player *pplayer, struct unit *pdiplomat, int times; Tech_type_id tech_stolen; bool expected_kills; + int cid, uid; + struct tile *ctile; + char clink[MAX_LEN_LINK]; /* We have to check arguments. They are sent to us by a client, so we cannot trust them */ fc_assert_ret_val(pcity, FALSE); cplayer = city_owner(pcity); + sz_strlcpy(clink, city_link(pcity)); fc_assert_ret_val(cplayer, FALSE); + cid = pcity->id; + ctile = city_tile(pcity); /* Sanity check: The actor still exists. */ fc_assert_ret_val(pplayer, FALSE); fc_assert_ret_val(pdiplomat, FALSE); + uid = pdiplomat->id; /* Sanity check: The target is foreign. */ if (cplayer == pplayer) { @@ -700,7 +715,7 @@ bool diplomat_get_tech(struct player *pplayer, struct unit *pdiplomat, if (!diplomat_infiltrate_tile(pplayer, cplayer, paction, pdiplomat, NULL, - pcity->tile)) { + ctile)) { return FALSE; } @@ -738,41 +753,41 @@ bool diplomat_get_tech(struct player *pplayer, struct unit *pdiplomat, if (count > 0) { /* Failed to steal a tech. */ if (times > 0 && expected_kills) { - notify_player(pplayer, city_tile(pcity), + notify_player(pplayer, ctile, E_MY_DIPLOMAT_FAILED, ftc_server, /* TRANS: Paris was expecting ... Your Spy was caught */ _("%s was expecting your attempt to steal technology " "again. Your %s was caught and executed."), - city_link(pcity), + clink, unit_tile_link(pdiplomat)); - notify_player(cplayer, city_tile(pcity), + notify_player(cplayer, ctile, E_ENEMY_DIPLOMAT_FAILED, ftc_server, /* TRANS: The Belgian Spy ... from Paris */ _("The %s %s failed to steal technology again from %s. " "We were prepared for the attempt."), nation_adjective_for_player(pplayer), unit_tile_link(pdiplomat), - city_link(pcity)); + clink); } else { - notify_player(pplayer, city_tile(pcity), + notify_player(pplayer, ctile, E_MY_DIPLOMAT_FAILED, ftc_server, /* TRANS: Your Spy was caught ... from %s. */ _("Your %s was caught in the attempt of" " stealing technology from %s."), unit_tile_link(pdiplomat), - city_link(pcity)); - notify_player(cplayer, city_tile(pcity), + clink); + notify_player(cplayer, ctile, E_ENEMY_DIPLOMAT_FAILED, ftc_server, /* TRANS: The Belgian Spy ... from Paris */ _("The %s %s failed to steal technology from %s."), nation_adjective_for_player(pplayer), unit_tile_link(pdiplomat), - city_link(pcity)); + clink); } /* This may cause a diplomatic incident */ action_consequence_caught(paction, pplayer, cplayer, - city_tile(pcity), city_link(pcity)); + ctile, clink); wipe_unit(pdiplomat, ULR_CAUGHT, cplayer); return FALSE; } @@ -780,12 +795,14 @@ bool diplomat_get_tech(struct player *pplayer, struct unit *pdiplomat, tech_stolen = steal_a_tech(pplayer, cplayer, technology); if (tech_stolen == A_NONE) { - notify_player(pplayer, city_tile(pcity), + notify_player(pplayer, ctile, E_MY_DIPLOMAT_FAILED, ftc_server, _("No new technology found in %s."), - city_link(pcity)); - diplomat_charge_movement (pdiplomat, pcity->tile); - send_unit_info(NULL, pdiplomat); + clink); + if (unit_is_alive(uid)) { + diplomat_charge_movement(pdiplomat, ctile); + send_unit_info(NULL, pdiplomat); + } return FALSE; } @@ -793,14 +810,20 @@ bool diplomat_get_tech(struct player *pplayer, struct unit *pdiplomat, send_player_all_c(pplayer, NULL); /* Record the theft. */ - (pcity->server.steal)++; + if (city_exist(cid)) { + (pcity->server.steal)++; + } else { + pcity = NULL; + } /* this may cause a diplomatic incident */ action_consequence_success(paction, pplayer, cplayer, - city_tile(pcity), city_link(pcity)); + ctile, clink); /* Check if a spy survives her mission. */ - diplomat_escape(pplayer, pdiplomat, pcity, paction); + if (unit_is_alive(uid)) { + diplomat_escape(pplayer, pdiplomat, pcity, paction); + } return TRUE; } @@ -830,6 +853,7 @@ bool diplomat_incite(struct player *pplayer, struct unit *pdiplomat, struct tile *ctile; const char *clink; int revolt_cost; + int csz, cid, uid; /* Fetch target civilization's player. Sanity checks. */ fc_assert_ret_val(pcity, FALSE); @@ -847,7 +871,10 @@ bool diplomat_incite(struct player *pplayer, struct unit *pdiplomat, } ctile = city_tile(pcity); + csz = city_size_get(pcity); + cid = pcity->id; clink = city_link(pcity); + uid = pdiplomat->id; log_debug("incite: unit: %d", pdiplomat->id); @@ -899,7 +926,7 @@ bool diplomat_incite(struct player *pplayer, struct unit *pdiplomat, /* Subvert the city to your cause... */ /* City loses some population. */ - if (city_size_get(pcity) > 1) { + if (csz > 1) { city_reduce_size(pcity, 1, pplayer, "incited"); } @@ -915,8 +942,12 @@ bool diplomat_incite(struct player *pplayer, struct unit *pdiplomat, clink, nation_adjective_for_player(pplayer)); - pcity->shield_stock = 0; - nullify_prechange_production(pcity); + if (csz == 1 || city_exist(cid)) { + /* City survived size change callback */ + /* FIXME: Can we move it later and do a single check? */ + pcity->shield_stock = 0; + nullify_prechange_production(pcity); + } /* You get a technology advance, too! */ steal_a_tech(pplayer, cplayer, A_UNSET); @@ -926,17 +957,25 @@ bool diplomat_incite(struct player *pplayer, struct unit *pdiplomat, /* Transfer city and units supported by this city (that are within one square of the city) to the new owner. */ - if (transfer_city(pplayer, pcity, 1, TRUE, TRUE, FALSE, - !is_barbarian(pplayer))) { - script_server_signal_emit("city_transferred", pcity, cplayer, pplayer, - "incited"); + /* FIXME: The city may get to a third party during scripts, + * still need to transfer? And cplayer in the callback? */ + if (city_exist(cid)) { + if (city_owner(pcity) == pplayer + || transfer_city(pplayer, pcity, 1, TRUE, TRUE, FALSE, + !is_barbarian(pplayer))) { + script_server_signal_emit("city_transferred", pcity, cplayer, pplayer, + "incited"); + } } - /* Check if a spy survives her mission. + + /* Check if a spy survives her mission (if she has survived the scripts). * _After_ transferring the city, or the city area is first fogged * when the diplomat is removed, and then unfogged when the city * is transferred. */ - diplomat_escape_full(pplayer, pdiplomat, TRUE, ctile, clink, paction); + if (player_unit_by_number(pplayer, uid)) { + diplomat_escape_full(pplayer, pdiplomat, TRUE, ctile, clink, paction); + } /* Update the players gold in the client */ send_player_info_c(pplayer, pplayer->connections); @@ -970,6 +1009,8 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, struct player *cplayer; struct impr_type *ptarget; int count, which; + int actorid, cityid; + struct tile *ctile; /* Fetch target city's player. Sanity checks. */ fc_assert_ret_val(pcity, FALSE); @@ -980,13 +1021,18 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, fc_assert_ret_val(pplayer, FALSE); fc_assert_ret_val(pdiplomat, FALSE); + /* Save data for checks, and tile for linking */ + actorid = pdiplomat->id; + cityid = pcity->id; + ctile = city_tile(pcity); + log_debug("sabotage: unit: %d", pdiplomat->id); /* Check if the Diplomat/Spy succeeds against defending Diplomats/Spies. */ if (!diplomat_infiltrate_tile(pplayer, cplayer, paction, pdiplomat, NULL, - pcity->tile)) { + ctile)) { return FALSE; } @@ -1009,7 +1055,7 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, /* This may cause a diplomatic incident */ action_consequence_caught(paction, pplayer, cplayer, - city_tile(pcity), city_link(pcity)); + ctile, city_link(pcity)); wipe_unit(pdiplomat, ULR_CAUGHT, cplayer); return FALSE; @@ -1161,11 +1207,11 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, if (fc_rand(100) >= vulnerability) { /* Caught! */ - notify_player(pplayer, city_tile(pcity), + notify_player(pplayer, ctile, E_MY_DIPLOMAT_FAILED, ftc_server, _("Your %s was caught in the attempt of sabotage!"), unit_tile_link(pdiplomat)); - notify_player(cplayer, city_tile(pcity), + notify_player(cplayer, ctile, E_ENEMY_DIPLOMAT_FAILED, ftc_server, _("You caught %s %s attempting" " to sabotage the %s in %s!"), @@ -1176,7 +1222,7 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, /* This may cause a diplomatic incident */ action_consequence_caught(paction, pplayer, cplayer, - city_tile(pcity), city_link(pcity)); + ctile, city_link(pcity)); wipe_unit(pdiplomat, ULR_CAUGHT, cplayer); log_debug("sabotage: caught in capital or on city walls"); @@ -1184,7 +1230,7 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, } /* Report it. */ - notify_player(pplayer, city_tile(pcity), + notify_player(pplayer, ctile, E_MY_DIPLOMAT_SABOTAGE, ftc_server, _("Your %s destroyed the %s in %s."), unit_link(pdiplomat), @@ -1203,19 +1249,27 @@ bool diplomat_sabotage(struct player *pplayer, struct unit *pdiplomat, /* Do it. */ building_lost(pcity, ptarget, "sabotaged", pdiplomat); - /* FIXME: Lua script might have destroyed the diplomat, the city, or even the player. - * Avoid dangling pointers below in that case. */ + /* Lua script might have destroyed the diplomat, the city, or even the player. + * Avoid dangling pointers below in that case. */ + if (!city_exist(cityid)) { + pcity = NULL; + } } /* Update clients. */ - send_city_info(NULL, pcity); + + if (pcity) { + send_city_info(NULL, pcity); + } /* this may cause a diplomatic incident */ - action_consequence_success(paction, pplayer, cplayer, - city_tile(pcity), city_link(pcity)); + action_consequence_success(paction, pplayer, cplayer, ctile, + pcity ? city_link(pcity) : _("a destroyed city")); /* Check if a spy survives her mission. */ - diplomat_escape(pplayer, pdiplomat, pcity, paction); + if (unit_is_alive(actorid)) { + diplomat_escape(pplayer, pdiplomat, pcity, paction); + } return TRUE; } @@ -1904,7 +1958,7 @@ static void diplomat_escape(struct player *pplayer, struct unit *pdiplomat, /************************************************************************** This determines if a diplomat/spy survives and escapes. - Spies have a game.server.diplchance specified chance of survival (better + Spies have a game.server.diplchance specified chance of survival (better if veteran): - Diplomats always die. - Escapes to home city. diff --git a/server/edithand.c b/server/edithand.c index d85d675e4b..6fb5dadad6 100644 --- a/server/edithand.c +++ b/server/edithand.c @@ -698,17 +698,13 @@ void handle_edit_city_create(struct connection *pc, int owner, int tile, conn_list_do_buffer(game.est_connections); map_show_tile(pplayer, ptile); + fc_assert_action(size >= 1, size = 1); + fc_assert_action(size <= MAX_CITY_SIZE, size = MAX_CITY_SIZE); create_city(pplayer, ptile, city_name_suggestion(pplayer, ptile), - pplayer); + pplayer, size); pcity = tile_city(ptile); - - if (size > 1) { - /* FIXME: Slow and inefficient for large size changes. */ - city_change_size(pcity, CLIP(1, size, MAX_CITY_SIZE), pplayer, NULL); - send_city_info(NULL, pcity); - } - - if (tag > 0) { + /* Scripts may have done something here but sending this one is safe */ + if (NULL != pcity && tag > 0) { dsend_packet_edit_object_created(pc, tag, pcity->id); } diff --git a/server/gamehand.c b/server/gamehand.c index 96aa9835a7..94781c8cdc 100644 --- a/server/gamehand.c +++ b/server/gamehand.c @@ -791,7 +791,7 @@ void init_new_game(void) /* Place first city */ if (game.server.start_city) { create_city(pplayer, ptile, city_name_suggestion(pplayer, ptile), - NULL); + NULL, 1); /* Expose visible area. */ map_show_circle(pplayer, ptile, game.server.init_vis_radius_sq); diff --git a/server/maphand.c b/server/maphand.c index 08f860f345..131abeb1b1 100644 --- a/server/maphand.c +++ b/server/maphand.c @@ -1758,6 +1758,10 @@ static void check_units_single_tile(struct tile *ptile) Check ptile and nearby tiles to see if all units can remain at their current locations, and move or disband any that cannot. Call this after terrain or specials change on ptile. + FIXME: it is called when the map is in illegal state. While bugs are + unlikely to happen here (save for an infinite cycle by a specifically + ill-planned script), it's better to ensure that the callbacks + are frozen per HRM#849859. ****************************************************************************/ void bounce_units_on_terrain_change(struct tile *ptile) { diff --git a/server/plrhand.c b/server/plrhand.c index ae960bcf01..29632b25dc 100644 --- a/server/plrhand.c +++ b/server/plrhand.c @@ -221,18 +221,19 @@ void kill_player(struct player *pplayer) } city_list_iterate_safe_end; game.server.savepalace = palace; - + resolve_unit_stacks(pplayer, barbarians, FALSE); /* Barbarians don't get free buildings like Palaces, so we don't * call city_build_free_buildings(). * FIXME: maybe this should be a ruleset option? */ - } else { - /* Destroy any remaining cities */ - city_list_iterate(pplayer->cities, pcity) { - remove_city(pcity); - } city_list_iterate_end; } + /* FIXME: signals may make an endless cycle or other mess here. + * Probably better we freeze them per HRM#849859 */ + /* Destroy any remaining cities (incl. ones maybe given by scripts) */ + city_list_iterate(pplayer->cities, pcity) { + remove_city(pcity); + } city_list_iterate_end; /* Remove all units that are still ours */ unit_list_iterate_safe(pplayer->units, punit) { @@ -2842,8 +2843,11 @@ struct player *civil_war(struct player *pplayer) /* Number to try to flip; ensure that at least one eligible city is * flipped */ i = MAX(j/2, 1); - city_list_iterate(defector_candidates, pcity) { - fc_assert_action(!is_capital(pcity), continue); + city_list_iterate_safe(defector_candidates, pcity) { + if (city_owner(pcity) != pplayer || is_capital(pcity)) { + /* A script made a mess here */ + continue; + } if (i >= j || (i > 0 && fc_rand(2) == 1)) { /* Transfer city and units supported by this city to the new owner. * We do NOT resolve stack conflicts here, but rather later. @@ -2865,19 +2869,20 @@ struct player *civil_war(struct player *pplayer) i--; } j--; - } city_list_iterate_end; + } city_list_iterate_safe_end; city_list_destroy(defector_candidates); resolve_unit_stacks(pplayer, cplayer, FALSE); i = city_list_size(cplayer->cities); - fc_assert(i > 0); /* rebels should have got at least one city */ - - /* Choose a capital (random). */ - capital = city_list_get(cplayer->cities, fc_rand(i)); - city_build_free_buildings(capital); - give_midgame_initial_units(cplayer, city_tile(capital)); + /* rebels should have got at least one city, but script maybe destroyed it */ + if (i > 0) { + /* Choose a capital (random). */ + capital = city_list_get(cplayer->cities, fc_rand(i)); + city_build_free_buildings(capital); + give_midgame_initial_units(cplayer, city_tile(capital)); + } notify_player(NULL, NULL, E_CIVIL_WAR, ftc_server, /* TRANS: ... Danes ... Poles ... <7> cities. */ diff --git a/server/ruleset.c b/server/ruleset.c index 9b0419c4db..3354464579 100644 --- a/server/ruleset.c +++ b/server/ruleset.c @@ -2104,6 +2104,7 @@ static bool load_ruleset_units(struct section_file *file, "%s.bombard_rate", sec_name); u->city_slots = secfile_lookup_int_default(file, 0, "%s.city_slots", sec_name); + /* FIXME: no check for validity */ u->city_size = secfile_lookup_int_default(file, 1, "%s.city_size", sec_name); } unit_type_iterate_end; diff --git a/server/scripting/api_server_edit.c b/server/scripting/api_server_edit.c index 385eb82474..4293093e03 100644 --- a/server/scripting/api_server_edit.c +++ b/server/scripting/api_server_edit.c @@ -158,6 +158,7 @@ bool api_edit_unit_teleport(lua_State *L, Unit *punit, Tile *dest) { bool alive; struct city *pcity; + struct player *owner = unit_owner(punit); LUASCRIPT_CHECK_STATE(L, FALSE); LUASCRIPT_CHECK_ARG_NIL(L, punit, 2, Unit, FALSE); @@ -173,10 +174,11 @@ bool api_edit_unit_teleport(lua_State *L, Unit *punit, Tile *dest) && uclass_has_flag(unit_class_get(punit), UCF_CAN_OCCUPY_CITY) && !unit_has_type_flag(punit, UTYF_CIVILIAN) - && pplayers_at_war(unit_owner(punit), - city_owner(pcity)))); + && pplayers_at_war(owner, city_owner(pcity)))); if (alive) { - struct player *owner = unit_owner(punit); + /* cascade callbacks may have changed things */ + owner = unit_owner(punit); + pcity = tile_city(dest); if (!can_unit_exist_at_tile(punit, dest)) { wipe_unit(punit, ULR_NONNATIVE_TERR, NULL); @@ -274,7 +276,7 @@ void api_edit_create_city(lua_State *L, Player *pplayer, Tile *ptile, } /* TODO: Allow initial citizen to be of nationality other than owner */ - create_city(pplayer, ptile, name, pplayer); + create_city(pplayer, ptile, name, pplayer, 1); } /***************************************************************************** diff --git a/server/techtools.c b/server/techtools.c index 8991044032..20eace13ee 100644 --- a/server/techtools.c +++ b/server/techtools.c @@ -841,9 +841,11 @@ static void research_tech_lost(struct research *presearch, Tech_type_id tech) unit_activities_cancel_all_illegal_plr(pplayer); /* Check city production */ - city_list_iterate(pplayer->cities, pcity) { + city_list_iterate_safe(pplayer->cities, pcity) { bool update = FALSE; + int cid = pcity->id; + /* A script may transfer the city, but we may just ignore it */ if (pcity->production.kind == VUT_UTYPE && !can_city_build_unit_now(pcity, pcity->production.value.utype)) { notify_player(pplayer, city_tile(pcity), @@ -854,11 +856,9 @@ static void research_tech_lost(struct research *presearch, Tech_type_id tech) utype_name_translation(pcity->production.value.utype)); choose_build_target(pplayer, pcity); update = TRUE; - } - - if (pcity->production.kind == VUT_IMPROVEMENT - && !can_city_build_improvement_now(pcity, - pcity->production.value.building)) { + } else if (pcity->production.kind == VUT_IMPROVEMENT + && !can_city_build_improvement_now + (pcity, pcity->production.value.building)) { notify_player(pplayer, city_tile(pcity), E_CITY_CANTBUILD, ftc_server, _("%s can't build %s. The required technology was " @@ -870,11 +870,11 @@ static void research_tech_lost(struct research *presearch, Tech_type_id tech) update = TRUE; } - if (update) { + if (update && city_exist(cid)) { city_refresh(pcity); send_city_info(pplayer, pcity); } - } city_list_iterate_end; + } city_list_iterate_safe_end; } research_players_iterate_end; } diff --git a/server/unithand.c b/server/unithand.c index 34ae97e10a..744f198bea 100644 --- a/server/unithand.c +++ b/server/unithand.c @@ -164,11 +164,8 @@ void handle_unit_type_upgrade(struct player *pplayer, Unit_type_id uti) int number_of_upgraded_units = 0; struct action *paction = action_by_number(ACTION_UPGRADE_UNIT); - if (NULL == from_unittype) { - /* Probably died or bribed. */ - log_verbose("handle_unit_type_upgrade() invalid unit type %d", uti); - return; - } + fc_assert_ret_msg(NULL != from_unittype, + "handle_unit_type_upgrade() invalid unit type %d", uti); to_unittype = can_upgrade_unittype(pplayer, from_unittype); if (!to_unittype) { @@ -178,12 +175,16 @@ void handle_unit_type_upgrade(struct player *pplayer, Unit_type_id uti) return; } - /* + /* * Try to upgrade units. The order we upgrade in is arbitrary (if - * the player really cared they should have done it manually). + * the player really cared they should have done it manually). */ conn_list_do_buffer(pplayer->connections); - unit_list_iterate(pplayer->units, punit) { + unit_list_iterate_safe(pplayer->units, punit) { + if (unit_owner(punit) != pplayer) { + /* A script has transferred the unit */ + continue; + } if (unit_type_get(punit) == from_unittype) { struct city *pcity = tile_city(unit_tile(punit)); @@ -195,7 +196,7 @@ void handle_unit_type_upgrade(struct player *pplayer, Unit_type_id uti) break; } } - } unit_list_iterate_end; + } unit_list_iterate_safe_end; conn_list_do_unbuffer(pplayer->connections); /* Alert the player about what happened. */ @@ -274,6 +275,8 @@ static bool do_capture_units(struct player *pplayer, /* Sanity check: make sure that the capture won't result in the actor * ending up with more than one unit of each unique unit type. */ BV_CLR_ALL(unique_on_tile); + /* FIXME: the callbacks within the cycle may mess everything! + * To be fixed in HRM#849859 */ unit_list_iterate(pdesttile->units, to_capture) { bool unique_conflict = FALSE; @@ -3321,16 +3324,9 @@ static bool city_build(struct player *pplayer, struct unit *punit, } nationality = unit_nationality(punit); - - create_city(pplayer, ptile, name, nationality); size = unit_type_get(punit)->city_size; - if (size > 1) { - struct city *pcity = tile_city(ptile); - fc_assert_ret_val(pcity != NULL, FALSE); - - city_change_size(pcity, size, nationality, NULL); - } + create_city(pplayer, ptile, name, nationality, size); /* May cause an incident even if the target tile is unclaimed. A ruleset * could give everyone a casus belli against the city founder. A rule @@ -3670,14 +3666,23 @@ static bool unit_bombard(struct unit *punit, struct tile *ptile, unit_did_action(punit); unit_forget_last_activity(punit); - + if (pcity && city_size_get(pcity) > 1 && get_city_bonus(pcity, EFT_UNIT_NO_LOSE_POP) <= 0 && kills_citizen_after_attack(punit)) { + int cid = pcity->id; + int uid = punit->id; + city_reduce_size(pcity, 1, pplayer, "bombard"); - city_refresh(pcity); - send_city_info(NULL, pcity); + /* A script may act here */ + if (city_exist(cid)) { + city_refresh(pcity); + send_city_info(NULL, pcity); + } + if (!unit_is_alive(uid)) { + return TRUE; + } } send_unit_info(NULL, punit); @@ -3857,7 +3862,7 @@ static bool do_attack(struct unit *punit, struct tile *def_tile, char attacker_tired[MAX_LEN_LINK]; struct unit *ploser, *pwinner; struct city *pcity; - int moves_used, def_moves_used; + int moves_used, def_moves_used; int old_unit_vet, old_defender_vet, vet; int winner_id; struct player *pplayer = unit_owner(punit); @@ -3867,12 +3872,14 @@ static bool do_attack(struct unit *punit, struct tile *def_tile, int att_hp_start, def_hp_start; int def_power, att_power; struct unit *pdefender; + int att_id = punit->id, def_id; if (!(pdefender = get_defender(punit, def_tile))) { /* Can't fight air... */ return FALSE; } - + + def_id = pdefender->id; att_hp_start = punit->hp; def_hp_start = pdefender->hp; def_power = get_total_defense_power(punit, pdefender); @@ -3965,9 +3972,18 @@ static bool do_attack(struct unit *punit, struct tile *def_tile, && city_size_get(pcity) > 1 && get_city_bonus(pcity, EFT_UNIT_NO_LOSE_POP) <= 0 && kills_citizen_after_attack(punit)) { + int cid = pcity->id; + city_reduce_size(pcity, 1, pplayer, "attack"); - city_refresh(pcity); - send_city_info(NULL, pcity); + /* A script may bring some mess here */ + if (city_exist(cid)) { + city_refresh(pcity); + send_city_info(NULL, pcity); + } + if (!unit_is_alive(att_id) || !unit_is_alive(def_id)) { + /* The combat happened but an undead citizen interrupted it */ + return TRUE; + } } if (unit_has_type_flag(punit, UTYF_ONEATTACK)) { punit->moves_left = 0; @@ -4143,13 +4159,17 @@ static bool do_attack(struct unit *punit, struct tile *def_tile, && unit_perform_action(unit_owner(punit), punit->id, pcity->id, 0, "", ACTION_CONQUER_CITY, ACT_REQ_RULES)) || (unit_move_handling(punit, def_tile, FALSE, TRUE, NULL))) { - int mcost = MAX(0, full_moves - punit->moves_left - SINGLE_MOVE); - - /* Move cost is bigger of attack (SINGLE_MOVE) and occupying move costs. - * Attack SINGLE_COST is already calculated in to old_moves. */ - punit->moves_left = old_moves - mcost; - if (punit->moves_left < 0) { - punit->moves_left = 0; + int mcost; + + /* punit may have been destroyed in action/move callbacks */ + if (unit_is_alive(winner_id)) { + mcost = MAX(0, full_moves - punit->moves_left - SINGLE_MOVE); + /* Move cost is bigger of attack (SINGLE_MOVE) and occupying move costs. + * Attack SINGLE_COST is already calculated in to old_moves. */ + punit->moves_left = old_moves - mcost; + if (punit->moves_left < 0) { + punit->moves_left = 0; + } } } else { punit->moves_left = old_moves; @@ -4157,7 +4177,7 @@ static bool do_attack(struct unit *punit, struct tile *def_tile, } /* The attacker may have died for many reasons */ - if (game_unit_by_number(winner_id) != NULL) { + if (unit_is_alive(winner_id)) { send_unit_info(NULL, pwinner); } diff --git a/server/unittools.c b/server/unittools.c index 26e729a264..9adc034a6b 100644 --- a/server/unittools.c +++ b/server/unittools.c @@ -1190,6 +1190,8 @@ bool teleport_unit_to_city(struct unit *punit, struct city *pcity, tile and move the unit there. If no tiles are found, the unit is disbanded. If 'verbose' is TRUE, a message is sent to the unit owner regarding what happened. + + FIXME: freeze signals here per HRM#849859 to avoid potential messy mess **************************************************************************/ void bounce_unit(struct unit *punit, bool verbose) { @@ -1262,6 +1264,8 @@ void bounce_unit(struct unit *punit, bool verbose) Throw pplayer's units from non allied cities If verbose is true, pplayer gets messages about where each units goes. + + FIXME: thaw signals or ensure they are already thawed per HRM#849859. **************************************************************************/ static void throw_units_from_illegal_cities(struct player *pplayer, bool verbose) @@ -1327,6 +1331,8 @@ static void throw_units_from_illegal_cities(struct player *pplayer, If verbose is true, the unit owner gets messages about where each units goes. + + FIXME: it's better to freeze signals here per HRM#849859. **************************************************************************/ static void resolve_stack_conflicts(struct player *pplayer, struct player *aplayer, bool verbose) @@ -2271,6 +2277,7 @@ void kill_unit(struct unit *pkiller, struct unit *punit, bool vet) } /* count killed units */ + /* FIXME: freeze callbacks per HRM#849859 to avoid errors */ unit_list_iterate_safe(deftile->units, vunit) { struct player *vplayer = unit_owner(vunit); @@ -2750,6 +2757,8 @@ bool do_airline(struct unit *punit, struct city *pdest_city, const struct action *paction) { struct city *psrc_city = tile_city(unit_tile(punit)); + int srcid = psrc_city->id; + int dstid = pdest_city->id; notify_player(unit_owner(punit), city_tile(pdest_city), E_UNIT_RELOCATED, ftc_server, @@ -2761,11 +2770,13 @@ bool do_airline(struct unit *punit, struct city *pdest_city, FALSE); /* Update airlift fields. */ - if (!(game.info.airlifting_style & AIRLIFTING_UNLIMITED_SRC)) { + if (!(game.info.airlifting_style & AIRLIFTING_UNLIMITED_SRC) + && city_exist(srcid)) { psrc_city->airlift--; send_city_info(city_owner(psrc_city), psrc_city); } - if (!(game.info.airlifting_style & AIRLIFTING_UNLIMITED_DEST)) { + if (!(game.info.airlifting_style & AIRLIFTING_UNLIMITED_DEST) + && city_exist(dstid)) { pdest_city->airlift--; send_city_info(city_owner(pdest_city), pdest_city); } -- 2.34.1