From b9e981e07e1525e78e64530c08dba1e24e29c467 Mon Sep 17 00:00:00 2001 From: Ihnatus Date: Mon, 13 Jun 2022 00:22:50 +0300 Subject: [PATCH] Rework functions using unit_change_owner() to avoid data corruption by scripts. This function have returned bad pointer, make it return good NULL. See OSDN#44778. Signed-off-by: Ihnatus --- server/diplomats.c | 58 ++++++++++++------ server/unithand.c | 147 ++++++++++++++++++++++++++++++++++++++------- server/unittools.c | 19 ++++-- 3 files changed, 178 insertions(+), 46 deletions(-) diff --git a/server/diplomats.c b/server/diplomats.c index d1ce80fb1c..b8e3641bb0 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,33 @@ 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 */ + 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 +759,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..b34b0b80f8 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; - 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); + 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); + } + + /* 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