From 499bdb79e56b4be104147e9afe18a2623f888867 Mon Sep 17 00:00:00 2001 From: Dino Date: Fri, 16 Oct 2026 20:52:50 -0400 Subject: [PATCH] prevent dereferencing NULL struct goods_type *pgood pointer RM #2179 --- ai/default/daiunit.c | 4 ++-- client/gui-sdl2/menu.c | 4 ++-- client/gui-sdl3/menu.c | 4 ++-- client/text.c | 17 ++++++++-------- common/actres.c | 6 +++--- common/aicore/caravan.c | 44 ++++++++++++++++++++++++----------------- common/traderoutes.c | 10 ++++++---- common/unit.c | 11 +++++++++++ common/unit.h | 3 +++ 9 files changed, 64 insertions(+), 39 deletions(-) diff --git a/ai/default/daiunit.c b/ai/default/daiunit.c index 3e78fc8937..fa043fdc7e 100644 --- a/ai/default/daiunit.c +++ b/ai/default/daiunit.c @@ -2381,14 +2381,14 @@ static void dai_manage_caravan(struct ai_type *ait, struct player *pplayer, /* We are moving to our destination */ /* We check to see if our current goal is feasible */ struct city *city_dest = tile_city(punit->goto_tile); - struct goods_type *pgood = unit_current_goods(punit, homecity); + int priority = unit_current_good_replace_priority(punit, homecity); if ((city_dest == NULL) || !pplayers_allied(unit_owner(punit), city_dest->owner) || (unit_data->task == AIUNIT_TRADE && !(can_cities_trade(homecity, city_dest) && can_establish_trade_route(homecity, city_dest, - pgood->replace_priority))) + priority))) || (unit_data->task == AIUNIT_WONDER /* Helping the (new) production is illegal. */ && !city_production_gets_caravan_shields(&city_dest->production)) diff --git a/client/gui-sdl2/menu.c b/client/gui-sdl2/menu.c index f1957b3376..112a06fcd1 100644 --- a/client/gui-sdl2/menu.c +++ b/client/gui-sdl2/menu.c @@ -1138,9 +1138,9 @@ void real_menus_update(void) unit_type_get(punit), punit->carrying, TRUE); - struct goods_type *pgood = unit_current_goods(punit, homecity); + int priority = unit_current_good_replace_priority(punit, homecity); - if (can_establish_trade_route(homecity, pcity, pgood->replace_priority)) { + if (can_establish_trade_route(homecity, pcity, priority)) { fc_snprintf(cbuf, sizeof(cbuf), _("%s With %s ( %d one time bonus + %d trade ) (R)"), action_id_name_translation(ACTION_TRADE_ROUTE), diff --git a/client/gui-sdl3/menu.c b/client/gui-sdl3/menu.c index 70e2149ca9..d65bcc681e 100644 --- a/client/gui-sdl3/menu.c +++ b/client/gui-sdl3/menu.c @@ -1134,9 +1134,9 @@ void real_menus_update(void) unit_type_get(punit), punit->carrying, TRUE); - struct goods_type *pgood = unit_current_goods(punit, homecity); + int priority = unit_current_good_replace_priority(punit, homecity); - if (can_establish_trade_route(homecity, pcity, pgood->replace_priority)) { + if (can_establish_trade_route(homecity, pcity, priority)) { fc_snprintf(cbuf, sizeof(cbuf), _("%s With %s ( %d one time bonus + %d trade ) (R)"), action_id_name_translation(ACTION_TRADE_ROUTE), diff --git a/client/text.c b/client/text.c index f7e12acd38..b03a743d59 100644 --- a/client/text.c +++ b/client/text.c @@ -365,14 +365,15 @@ const char *popup_info_text(struct tile *ptile) struct city *hcity = game_city_by_number(pfocus_unit->homecity); if (utype_can_do_action(unit_type_get(pfocus_unit), ACTION_TRADE_ROUTE) - && can_cities_trade(hcity, pcity) - && can_establish_trade_route(hcity, pcity, - unit_current_goods(pfocus_unit, - hcity)->replace_priority)) { - /* TRANS: "Trade from Warsaw: 5" */ - astr_add_line(&str, _("Trade from %s: %d"), - city_name_get(hcity), - trade_base_between_cities(hcity, pcity)); + && can_cities_trade(hcity, pcity)) { + int priority = unit_current_good_replace_priority(pfocus_unit, hcity); + + if (can_establish_trade_route(hcity, pcity, priority)) { + /* TRANS: "Trade from Warsaw: 5" */ + astr_add_line(&str, _("Trade from %s: %d"), + city_name_get(hcity), + trade_base_between_cities(hcity, pcity)); + } } } unit_list_iterate_end; } diff --git a/common/actres.c b/common/actres.c index ae95369ef7..ea35eb9dd5 100644 --- a/common/actres.c +++ b/common/actres.c @@ -923,10 +923,10 @@ enum fc_tristate actres_possible(const struct civ_map *nmap, /* There are more restrictions on establishing a trade route than on * entering the market place. */ if (result == ACTRES_TRADE_ROUTE) { - struct goods_type *pgood = unit_current_goods(actor->unit, homecity); + int priority = + unit_current_good_replace_priority(actor->unit, homecity); - if (!can_establish_trade_route(homecity, target->city, - pgood->replace_priority)) { + if (!can_establish_trade_route(homecity, target->city, priority)) { return TRI_NO; } } diff --git a/common/aicore/caravan.c b/common/aicore/caravan.c index 0a40b640bf..29e36643a1 100644 --- a/common/aicore/caravan.c +++ b/common/aicore/caravan.c @@ -251,6 +251,20 @@ static void caravan_search_from(const struct civ_map *nmap, pf_map_destroy(pfm); } +/**********************************************************************//** + Return the replace_priority of the goods a city would provide. + May return -1 if city can't provide any goods. + + @param src City to start traderoute from + @return the goods's replace_priority, maybe -1 +**************************************************************************/ +static int city_current_good_replace_priority(const struct city *pcity) +{ + struct goods_type *pgoods = goods_from_city_to_unit(pcity, nullptr); + + return pgoods != nullptr ? pgoods->replace_priority : -1; +} + /************************************************************************//** When the caravan arrives, compute the benefit from the immediate windfall, taking into account the parameter's objective. @@ -258,15 +272,15 @@ static void caravan_search_from(const struct civ_map *nmap, static double windfall_benefit(const struct unit *caravan, const struct city *src, const struct city *dest, - const struct goods_type *pgood, const struct caravan_parameter *param) { if (!param->consider_windfall || !can_cities_trade(src, dest)) { return 0; } else { - bool can_establish = (unit_can_do_action(caravan, ACTION_TRADE_ROUTE) - && can_establish_trade_route(src, dest, - pgood->replace_priority)); + int priority = + city_current_good_replace_priority(src); + bool can_establish = unit_can_do_action(caravan, ACTION_TRADE_ROUTE) + && can_establish_trade_route(src, dest, priority); int bonus = get_caravan_enter_city_trade_bonus(src, dest, unit_type_get(caravan), NULL, can_establish); @@ -291,7 +305,6 @@ static double windfall_benefit(const struct unit *caravan, ****************************************************************************/ static int one_city_trade_benefit(const struct city *pcity, const struct player *pplayer, - const struct goods_type *pgood, bool countloser, int newtrade) { int losttrade = 0; @@ -306,10 +319,10 @@ static int one_city_trade_benefit(const struct city *pcity, /* If the city can handle this route, we don't break any old routes */ losttrade = 0; } else { + int priority = city_current_good_replace_priority(pcity); struct trade_route_list *would_remove = (countloser ? trade_route_list_new() : NULL); - int oldtrade = city_trade_removable(pcity, pgood->replace_priority, - would_remove); + int oldtrade = city_trade_removable(pcity, priority, would_remove); /* If we own the city, the trade benefit is only by how much better we are than the old trade route */ @@ -347,7 +360,6 @@ static int one_city_trade_benefit(const struct city *pcity, static double trade_benefit(const struct player *caravan_owner, const struct city *src, const struct city *dest, - const struct goods_type *pgood, const struct caravan_parameter *param) { /* Do we care about trade at all? */ @@ -357,7 +369,8 @@ static double trade_benefit(const struct player *caravan_owner, /* First, see if a new route is made. */ if (!can_cities_trade(src, dest) - || !can_establish_trade_route(src, dest, pgood->replace_priority)) { + || !can_establish_trade_route(src, dest, + city_current_good_replace_priority(src))) { return 0; } if (max_trade_routes(src) <= 0 || max_trade_routes(dest) <= 0) { @@ -370,10 +383,8 @@ static double trade_benefit(const struct player *caravan_owner, bool countloser = param->account_for_broken_routes; int newtrade = trade_base_between_cities(src, dest); - return one_city_trade_benefit(src, caravan_owner, pgood, - countloser, newtrade) - + one_city_trade_benefit(dest, caravan_owner, pgood, - countloser, newtrade); + return one_city_trade_benefit(src, caravan_owner, countloser, newtrade) + + one_city_trade_benefit(dest, caravan_owner, countloser, newtrade); } else { /* Always fails. */ fc_assert_msg(!param->convert_trade, @@ -495,7 +506,6 @@ static bool get_discounted_reward(const struct unit *caravan, bool consider_wonder; bool consider_trade; bool consider_windfall; - struct goods_type *pgood; const struct civ_map *nmap = &(wld.map); /* If no foreign trade is allowed, just quit. */ @@ -537,10 +547,8 @@ static bool get_discounted_reward(const struct unit *caravan, wonder = -1.0; } - pgood = goods_from_city_to_unit(src, NULL); - if (consider_trade) { - trade = trade_benefit(pplayer_src, src, dest, pgood, parameter); + trade = trade_benefit(pplayer_src, src, dest, parameter); if (parameter->horizon == FC_INFINITY) { trade = perpetuity(trade, discount); } else { @@ -552,7 +560,7 @@ static bool get_discounted_reward(const struct unit *caravan, } if (consider_windfall) { - windfall = windfall_benefit(caravan, src, dest, pgood, parameter); + windfall = windfall_benefit(caravan, src, dest, parameter); windfall = presentvalue(windfall, arrival_time, discount); } else { windfall = 0.0; diff --git a/common/traderoutes.c b/common/traderoutes.c index c81a0d743b..5feb8333cc 100644 --- a/common/traderoutes.c +++ b/common/traderoutes.c @@ -735,11 +735,12 @@ bool city_receives_goods(const struct city *pcity, } /*********************************************************************//** - Fond out goods type for the new trade route + Return goods type for the new trade route. + May return nullptr if city can't provide any goods. @param src City to start traderoute from @param punit Unit to carry the goods - @return Goods to carry + @return Goods to carry, may be nullptr *************************************************************************/ struct goods_type *goods_from_city_to_unit(const struct city *src, const struct unit *punit) @@ -763,7 +764,7 @@ struct goods_type *goods_from_city_to_unit(const struct city *src, } goods_type_iterate_end; if (i == 0) { - return NULL; + return nullptr; } return potential[fc_rand(i)]; @@ -771,10 +772,11 @@ struct goods_type *goods_from_city_to_unit(const struct city *src, /*********************************************************************//** What goods unit would provide if it established trade route now? + May return nullptr if city can't provide any goods. @param punit Unit to establish the trade route @param homecity Homecity of the unit. Can be used speculatively. - @return What good unit would provide + @return What good unit would provide, may be nullptr *************************************************************************/ struct goods_type *unit_current_goods(const struct unit *punit, const struct city *homecity) diff --git a/common/unit.c b/common/unit.c index ee0de992ee..9b70e3ceb0 100644 --- a/common/unit.c +++ b/common/unit.c @@ -2976,3 +2976,14 @@ enum gen_action activity_default_action(enum unit_activity act) return act_act[act]; } + +/**********************************************************************//** + Returns the unit's *pgood's replace_priority, or -1 if pgood is nullptr +**************************************************************************/ +int unit_current_good_replace_priority(const struct unit *punit, + const struct city *homecity) +{ + struct goods_type *pgoods = unit_current_goods(punit, homecity); + + return pgoods != nullptr ? pgoods->replace_priority : -1; +} diff --git a/common/unit.h b/common/unit.h index f9603c4632..772d808dac 100644 --- a/common/unit.h +++ b/common/unit.h @@ -594,6 +594,9 @@ struct unit_order *create_unit_orders(const struct civ_map *nmap, enum gen_action activity_default_action(enum unit_activity act); +int unit_current_good_replace_priority(const struct unit *punit, + const struct city *homecity); + #ifdef __cplusplus } #endif /* __cplusplus */ -- 2.31.0