From 33975d114a00c4bcbdc63238f153df09652f0765 Mon Sep 17 00:00:00 2001 From: Ihnatus Date: Mon, 13 Jun 2022 21:17:54 +0300 Subject: [PATCH] Rework functions using unit_change_owner() for script effects This function may sometimes return NULL or a foreign unit, respect it. During bribing, the gold may be taken away by scripts before paid, don't go negative. During capturing, the city to home units may be destroyed, the captured units are going to be lost with it. Some other fixes to avoid using dead pointers or forming impossible stacks. See OSDN#44778. Signed-off-by: Ihnatus --- server/diplomats.c | 62 +++++++++++++------ server/unithand.c | 147 ++++++++++++++++++++++++++++++++++++++------- server/unittools.c | 19 ++++-- 3 files changed, 182 insertions(+), 46 deletions(-) diff --git a/server/diplomats.c b/server/diplomats.c index d1ce80fb1c..ec077ad44a 100644 --- a/server/diplomats.c +++ b/server/diplomats.c @@ -630,19 +630,21 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, struct player *uplayer; struct tile *victim_tile; int bribe_cost; - int diplomat_id = pdiplomat->id; - const struct unit_type *act_utype; + int diplomat_id; + const struct unit_type *act_utype, *victim_type; struct city *pcity; bool bounce; /* Fetch target unit's player. Sanity checks. */ fc_assert_ret_val(pvictim, FALSE); uplayer = unit_owner(pvictim); + victim_type = unit_type_get(pvictim); fc_assert_ret_val(uplayer, FALSE); /* Sanity check: The actor still exists. */ fc_assert_ret_val(pplayer, FALSE); fc_assert_ret_val(pdiplomat, FALSE); + diplomat_id = pdiplomat->id; act_utype = unit_type_get(pdiplomat); @@ -693,17 +695,27 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, pvictim = unit_change_owner(pvictim, pplayer, pdiplomat->homecity, ULR_BRIBED); - /* N.B.: unit_link always returns the same pointer. As unit_change_owner() - * currently remove the old unit and replace by a new one (with a new id), - * we want to make link to the new unit. */ - sz_strlcpy(victim_link, unit_link(pvictim)); + if (pvictim) { + /* N.B.: unit_link always returns the same pointer. As unit_change_owner() + * currently remove the old unit and replace by a new one (with a new id), + * we want to make link to the new unit. */ + sz_strlcpy(victim_link, unit_link(pvictim)); + } else { + sz_strlcpy(victim_link, utype_name_translation(victim_type)); + } + + if (!unit_is_alive(diplomat_id)) { + /* Destroyed by a script */ + pdiplomat = NULL; + } /* Notify everybody involved. */ notify_player(pplayer, victim_tile, E_MY_DIPLOMAT_BRIBE, ftc_server, /* TRANS: ... */ _("Your %s succeeded in bribing the %s."), - unit_link(pdiplomat), victim_link); - if (maybe_make_veteran(pdiplomat, 100)) { + pdiplomat ? unit_link(pdiplomat) + : utype_name_translation(act_utype), victim_link); + if (pdiplomat && maybe_make_veteran(pdiplomat, 100)) { notify_unit_experience(pdiplomat); } notify_player(uplayer, victim_tile, E_ENEMY_DIPLOMAT_BRIBE, ftc_server, @@ -711,23 +723,37 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, _("Your %s was bribed by the %s."), victim_link, nation_plural_for_player(pplayer)); - /* The unit may have been on a tile shared with a city or a unit - * it no longer can share a tile with. */ - pcity = tile_city(unit_tile(pvictim)); - bounce = ((NULL != pcity && !pplayers_allied(city_owner(pcity), pplayer)) - || 1 < unit_list_size(unit_tile(pvictim)->units)); - if (bounce) { - bounce_unit(pvictim, TRUE); + if (pvictim) { + /* The unit may have been on a tile shared with a city or a unit + * it no longer can share a tile with. */ + pcity = tile_city(unit_tile(pvictim)); + bounce = ((NULL != pcity + && !pplayers_allied(city_owner(pcity), unit_owner(pvictim))) + /* Keep the old behavior (insto check is_non_allied_unit_tile) */ + || 1 < unit_list_size(unit_tile(pvictim)->units)); + if (bounce) { + bounce_unit(pvictim, TRUE); + } + } else { + bounce = FALSE; } /* This costs! */ pplayer->economic.gold -= bribe_cost; + if (pplayer->economic.gold < 0) { + /* Scripts have deprived us of too much gold before we paid */ + log_normal("%s has bribed %s but has not %d gold at payment time, " + "%d is the discount", player_name(pplayer), + utype_rule_name(victim_type), bribe_cost, + -pplayer->economic.gold); + pplayer->economic.gold = 0; + } /* This may cause a diplomatic incident */ action_consequence_success(paction, pplayer, act_utype, uplayer, victim_tile, victim_link); - if (!unit_is_alive(diplomat_id)) { + if (!pdiplomat || !unit_is_alive(diplomat_id)) { return TRUE; } @@ -737,8 +763,8 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, /* Try to perform post move forced actions. */ && (NULL == action_auto_perf_unit_do(AAPC_POST_ACTION, pdiplomat, uplayer, NULL, paction, - victim_tile, pcity, pvictim, - NULL)) + victim_tile, tile_city(victim_tile), + pvictim, NULL)) /* May have died while trying to do forced actions. */ && unit_is_alive(diplomat_id)) { pdiplomat->moves_left = 0; diff --git a/server/unithand.c b/server/unithand.c index 8ba3330634..ab02155649 100644 --- a/server/unithand.c +++ b/server/unithand.c @@ -172,6 +172,9 @@ static bool do_action_activity(struct unit *punit, static bool do_action_activity_targeted(struct unit *punit, const struct action *paction, struct extra_type **new_target); +static inline bool +non_allied_not_listed_at(const struct player *pplayer, + const int *list, int n, const struct tile *ptile); /**********************************************************************//** Upgrade all units of a given type. @@ -270,6 +273,33 @@ static bool do_unit_upgrade(struct player *pplayer, return TRUE; } +/**********************************************************************//** + Helper function for do_capture_units(). Tells if ptile contains + a unit not allied to pplayer whose id is not on the list. +**************************************************************************/ +static inline bool +non_allied_not_listed_at(const struct player *pplayer, + const int *list, int n, const struct tile *ptile) +{ + unit_list_iterate(ptile->units, punit) { + if (!pplayers_allied(pplayer, unit_owner(punit))) { + bool listed = FALSE; + int id = punit->id; + + for (int i = 0; i < n; i++) { + if (id == list[i]) { + listed = TRUE; + break; + } + } + if (!listed) { + return TRUE; + } + } + } unit_list_iterate_end; + return FALSE; +} + /**********************************************************************//** Capture all the units at pdesttile using punit. @@ -283,13 +313,18 @@ static bool do_capture_units(struct player *pplayer, { struct city *pcity; char capturer_link[MAX_LEN_LINK]; + char hcity_name[MAX_LEN_NAME] = {'\0'}; const char *capturer_nation = nation_plural_for_player(pplayer); bv_unit_types unique_on_tile; const struct unit_type *act_utype; + int id, hcity; + int n = 0, capt[unit_list_size(pdesttile->units)]; + bool lost_with_city = FALSE; /* Sanity check: The actor still exists. */ fc_assert_ret_val(pplayer, FALSE); fc_assert_ret_val(punit, FALSE); + id = punit->id; act_utype = unit_type_get(punit); @@ -331,33 +366,64 @@ static bool do_capture_units(struct player *pplayer, return FALSE; } + /* Remember the units here + * for the mess callbacks may do in the process of transferring */ + capt[n++] = to_capture->id; } unit_list_iterate_end; /* N.B: unit_link() always returns the same pointer. */ sz_strlcpy(capturer_link, unit_link(punit)); pcity = tile_city(pdesttile); - unit_list_iterate(pdesttile->units, to_capture) { - struct player *uplayer = unit_owner(to_capture); + hcity = game.server.homecaughtunits + ? punit->homecity : IDENTITY_NUMBER_ZERO; + if (hcity) { + /* Rarely, we'll need it... */ + sz_strlcpy(hcity_name, city_name_get(game_city_by_number(hcity))); + } + for (int i = 0; i < n; i++) { + struct unit *to_capture = game_unit_by_number(capt[i]); + struct player *uplayer; const char *victim_link; + const struct unit_type *utype; + struct tile *ptile = NULL; + bool really_lost = FALSE; + + if (!to_capture) { + continue; + } + uplayer = unit_owner(to_capture); + if (uplayer == pplayer) { + /* Somehow transferred by scripts (e.g. diplomat incited a city) */ + continue; + } + utype = unit_type_get(to_capture); + really_lost = lost_with_city && !utype_has_flag(utype, UTYF_NOHOME); + uplayer->score.units_lost++; + if (!really_lost) { + /* A hack: if the captured unit is lost with a capturer's city, + * we link the old unit, otherwise the new one */ + to_capture = unit_change_owner(to_capture, pplayer, + hcity, ULR_CAPTURED); + } + if (!to_capture) { + /* Lost during capturing */ + victim_link = utype_name_translation(utype); + } else { + /* As unit_change_owner() currently remove the old unit and + * replace by a new one (with a new id), we want to make link to + * the new unit. */ + victim_link = unit_link(to_capture); + ptile = unit_tile(to_capture); + /* Notify capturer only if there is a gain */ + notify_player(pplayer, pdesttile, E_MY_DIPLOMAT_BRIBE, ftc_server, + /* TRANS: ... */ + _("Your %s succeeded in capturing the %s %s."), + capturer_link, nation_adjective_for_player(uplayer), + victim_link); + } - unit_owner(to_capture)->score.units_lost++; - to_capture = unit_change_owner(to_capture, pplayer, - (game.server.homecaughtunits - ? punit->homecity - : IDENTITY_NUMBER_ZERO), - ULR_CAPTURED); - /* As unit_change_owner() currently remove the old unit and - * replace by a new one (with a new id), we want to make link to - * the new unit. */ - victim_link = unit_link(to_capture); - - /* Notify players */ - notify_player(pplayer, pdesttile, E_MY_DIPLOMAT_BRIBE, ftc_server, - /* TRANS: ... */ - _("Your %s succeeded in capturing the %s %s."), - capturer_link, nation_adjective_for_player(uplayer), - victim_link); + /* Notify loser */ notify_player(uplayer, pdesttile, E_ENEMY_DIPLOMAT_BRIBE, ftc_server, /* TRANS: ... */ @@ -368,12 +434,49 @@ static bool do_capture_units(struct player *pplayer, action_consequence_success(paction, pplayer, act_utype, uplayer, pdesttile, victim_link); - if (NULL != pcity) { - /* The captured unit is in a city. Bounce it. */ + if (really_lost) { + /* The city for which the unit was captured has perished! */ + /* Nobody actually gets the unit. */ + pplayer->score.units_lost++; + notify_player(pplayer, pdesttile, + E_UNIT_LOST_MISC, ftc_server, + _("%s lost along with control of %s."), + victim_link, hcity_name); + /* As in unit_change_owner(), don't say pplayer is killer */ + wipe_unit(to_capture, ULR_CAPTURED, NULL); + continue; + } + + if (to_capture + && (NULL != pcity /* Keep old behavior */ + || is_non_allied_city_tile(ptile, unit_owner(to_capture)) + || (unit_owner(to_capture) == pplayer + ? non_allied_not_listed_at(pplayer, capt + (i + 1), + n - (i + 1), ptile) + : (bool) + is_non_allied_unit_tile(ptile, unit_owner(to_capture))))) { + /* The captured unit is in a city or with a foreign unit + * that its owner is not capturing. Bounce it. */ bounce_unit(to_capture, TRUE); } - } unit_list_iterate_end; + /* Check if the city we are going to home units in stays. */ + if (hcity && i + 1 < n && !player_city_by_number(pplayer, hcity)) { + /* Oops, it's lost. Maybe the capturer is rehomed? */ + if (player_unit_by_number(pplayer, id)) { + /* Well, it's natural to home them here now */ + hcity = punit->homecity; + } else { + /* Removing the rest of the stack (except "NoHome" units) */ + lost_with_city = TRUE; + } + } + } + + if (!unit_is_alive(id)) { + /* Callbacks took the capturer, nothing more to do */ + return TRUE; + } unit_did_action(punit); unit_forget_last_activity(punit); diff --git a/server/unittools.c b/server/unittools.c index ea96e3f205..a8542d70a7 100644 --- a/server/unittools.c +++ b/server/unittools.c @@ -2124,20 +2124,27 @@ static bool try_to_save_unit(struct unit *punit, const struct unit_type *pttype, /**********************************************************************//** We don't really change owner of the unit, but create completely new unit as its copy. The new pointer to 'punit' is returned. + Always wipes the source unit but sometimes returns NULL or a unit + of a player other than pplayer. **************************************************************************/ struct unit *unit_change_owner(struct unit *punit, struct player *pplayer, int homecity, enum unit_loss_reason reason) { struct unit *gained_unit; + int id = 0; fc_assert(!utype_player_already_has_this_unique(pplayer, unit_type_get(punit))); /* Convert the unit to your cause. Fog is lifted in the create algorithm. */ + /* This call supposes that the original unit is on a valid tile + * and is not transported. */ gained_unit = create_unit_full(pplayer, unit_tile(punit), unit_type_get(punit), punit->veteran, homecity, punit->moves_left, punit->hp, NULL); + fc_assert_action(gained_unit, goto uco_wipe); /* Tile must be valid */ + id = gained_unit->id; /* Owner changes, nationality not. */ gained_unit->nationality = punit->nationality; @@ -2149,19 +2156,19 @@ struct unit *unit_change_owner(struct unit *punit, struct player *pplayer, send_unit_info(NULL, gained_unit); - /* update unit upkeep in the homecity of the victim */ - if (punit->homecity > 0) { - /* update unit upkeep */ - city_units_upkeep(game_city_by_number(punit->homecity)); - } /* update unit upkeep in the new homecity */ if (homecity > 0) { city_units_upkeep(game_city_by_number(homecity)); } /* Be sure to wipe the converted unit! */ - wipe_unit(punit, reason, NULL); + /* Old homecity upkeep is updated in process */ + uco_wipe: wipe_unit(punit, reason, NULL); + if (!unit_is_alive(id)) { + /* Destroyed by a script */ + return NULL; + } return gained_unit; /* Returns the replacement. */ } -- 2.34.1