From e5b12f48da6651aa9605ee1f7d678aa1c7339f2f Mon Sep 17 00:00:00 2001 From: Ihnatus Date: Mon, 13 Jun 2022 20:35:07 +0300 Subject: [PATCH] Rework functions using unit_change_owner() for script effects This function may sometimes return NULL, 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 | 81 +++++++++++++++++--------- server/unithand.c | 142 ++++++++++++++++++++++++++++++++++++++------- server/unittools.c | 18 ++++-- 3 files changed, 187 insertions(+), 54 deletions(-) diff --git a/server/diplomats.c b/server/diplomats.c index 5e389120e6..2dd705f274 100644 --- a/server/diplomats.c +++ b/server/diplomats.c @@ -464,19 +464,23 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, char victim_link[MAX_LEN_LINK]; struct player *uplayer; struct tile *victim_tile; + struct unit_type *victim_type, *diplo_type; int bribe_cost; - int diplomat_id = pdiplomat->id; + int diplomat_id; 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; + diplo_type = unit_type_get(pdiplomat); /* Sanity check: The target is foreign. */ if (uplayer == pplayer) { @@ -524,17 +528,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)) { + pdiplomat ? unit_link(pdiplomat) + : utype_name_translation(diplo_type), victim_link); + if (pdiplomat && maybe_make_veteran(pdiplomat)) { notify_unit_experience(pdiplomat); } notify_player(uplayer, victim_tile, E_ENEMY_DIPLOMAT_BRIBE, ftc_server, @@ -544,35 +558,50 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, /* 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) { + 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); + } + } 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, uplayer, victim_tile, victim_link); - if (!unit_is_alive(diplomat_id)) { - return TRUE; - } + if (pdiplomat) { + if (!unit_is_alive(diplomat_id)) { + /* Destroyed during bouncing */ + return TRUE; + } - /* Try to move the briber onto the victim's square unless the victim has - * been bounced because it couldn't share tile with a unit or city. */ - if (!bounce - /* Post bribe move. */ - && !unit_move_handling(pdiplomat, victim_tile, FALSE, TRUE, NULL) - /* May have died while trying to move. */ - && unit_is_alive(diplomat_id)) { - pdiplomat->moves_left = 0; - } - if (NULL != player_unit_by_number(pplayer, diplomat_id)) { - send_unit_info(NULL, pdiplomat); + /* Try to move the briber onto the victim's square unless the victim has + * been bounced because it couldn't share tile with a unit or city. */ + if (!bounce + /* Post bribe move. */ + && !unit_move_handling(pdiplomat, victim_tile, FALSE, TRUE, NULL) + /* May have died while trying to move. */ + && unit_is_alive(diplomat_id)) { + pdiplomat->moves_left = 0; + } + if (NULL != player_unit_by_number(pplayer, diplomat_id)) { + send_unit_info(NULL, pdiplomat); + } } /* Update clients. */ diff --git a/server/unithand.c b/server/unithand.c index 34ae97e10a..129e2ccf0f 100644 --- a/server/unithand.c +++ b/server/unithand.c @@ -153,6 +153,9 @@ static bool do_unit_conquer_city(struct player *act_player, struct unit *act_unit, struct city *tgt_city, struct action *paction); +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. @@ -251,6 +254,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. @@ -264,12 +294,17 @@ 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; + 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; /* 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. */ @@ -309,33 +344,63 @@ 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); + utype = unit_type_get(to_capture); + really_lost = lost_with_city && !utype_has_flag(utype, UTYF_NOHOME); + /* Likely, currently there's no probability that + * a unit with given id has been transferred during this loop + * (check kill_dying_players() calls to be sure) */ + 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: ... */ @@ -346,12 +411,45 @@ static bool do_capture_units(struct player *pplayer, action_consequence_success(paction, pplayer, 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, pplayer) + || non_allied_not_listed_at(pplayer, capt + (i + 1), + n - (i + 1), ptile))) { + /* The captured unit is in a city or with a foreign unit + * that we are 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. */ + lost_with_city = TRUE; + } + } + } + + if (!unit_is_alive(id)) { + /* Callbacks took the capturer, nothing more to do */ + return TRUE; + } /* Subtract movement point from capturer */ punit->moves_left -= SINGLE_MOVE; if (punit->moves_left < 0) { diff --git a/server/unittools.c b/server/unittools.c index 8d431f23fc..52bb2a24de 100644 --- a/server/unittools.c +++ b/server/unittools.c @@ -2081,20 +2081,26 @@ static bool try_to_save_unit(struct unit *punit, 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. ****************************************************************************/ 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; @@ -2106,19 +2112,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