From ab0e8fd1b77e1b9a88424a9e204278e3291e0079 Mon Sep 17 00:00:00 2001 From: Ihnatus Date: Fri, 10 Jun 2022 00:28:04 +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 | 74 ++++++++++++++++-------- server/unithand.c | 138 +++++++++++++++++++++++++++++++++++++-------- server/unittools.c | 18 ++++-- 3 files changed, 177 insertions(+), 53 deletions(-) diff --git a/server/diplomats.c b/server/diplomats.c index 5e389120e6..67d629c60f 100644 --- a/server/diplomats.c +++ b/server/diplomats.c @@ -464,6 +464,7 @@ 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; struct city *pcity; @@ -472,11 +473,13 @@ bool diplomat_bribe(struct player *pplayer, struct unit *pdiplomat, /* 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); + diplo_type = unit_type_get(pdiplomat); /* Sanity check: The target is foreign. */ if (uplayer == pplayer) { @@ -524,17 +527,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 +557,46 @@ 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 */ + 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..c2579f3403 100644 --- a/server/unithand.c +++ b/server/unithand.c @@ -251,6 +251,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 +291,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 +341,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; + + 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); + } - 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: ... */ @@ -346,12 +408,44 @@ 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, n, 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