From 1d0ee3abf1180741389e3dcc77726c0ede892c96 Mon Sep 17 00:00:00 2001 From: Dino Date: Sun, 11 Oct 2026 20:42:47 -0400 Subject: [PATCH] prevent dereferencing NULL struct goods_type *pgood pointer RM #2179 --- ai/default/daiunit.c | 3 ++- client/gui-sdl2/menu.c | 3 ++- client/gui-sdl3/menu.c | 3 ++- client/text.c | 19 +++++++++++-------- common/actres.c | 4 ++-- common/aicore/caravan.c | 14 ++++++++------ common/traderoutes.c | 8 +++++--- common/unit.c | 8 ++++++++ common/unit.h | 2 ++ 9 files changed, 42 insertions(+), 22 deletions(-) diff --git a/ai/default/daiunit.c b/ai/default/daiunit.c index 9c4c555eeb..2a37d96a94 100644 --- a/ai/default/daiunit.c +++ b/ai/default/daiunit.c @@ -2391,13 +2391,14 @@ static void dai_manage_caravan(struct ai_type *ait, struct player *pplayer, /* 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(pgood); if ((city_dest == nullptr) || !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 ecb187ed16..bbbd3cf619 100644 --- a/client/gui-sdl2/menu.c +++ b/client/gui-sdl2/menu.c @@ -1139,8 +1139,9 @@ void real_menus_update(void) punit->carrying, TRUE); struct goods_type *pgood = unit_current_goods(punit, homecity); + int priority = unit_current_good_replace_priority(pgood); - 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 4c6670922b..83436a4875 100644 --- a/client/gui-sdl3/menu.c +++ b/client/gui-sdl3/menu.c @@ -1135,8 +1135,9 @@ void real_menus_update(void) punit->carrying, TRUE); struct goods_type *pgood = unit_current_goods(punit, homecity); + int priority = unit_current_good_replace_priority(pgood); - 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 d2e215154c..5e5d4e1625 100644 --- a/client/text.c +++ b/client/text.c @@ -365,14 +365,17 @@ 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)) { + struct goods_type *pgood = + unit_current_goods(pfocus_unit, hcity); + int priority = unit_current_good_replace_priority(pgood); + + 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 b7b4f7fcce..dce7015777 100644 --- a/common/actres.c +++ b/common/actres.c @@ -932,9 +932,9 @@ enum fc_tristate actres_possible(const struct civ_map *nmap, * 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(pgood); - 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 5b1640970a..4f58bc26cb 100644 --- a/common/aicore/caravan.c +++ b/common/aicore/caravan.c @@ -264,9 +264,10 @@ static double windfall_benefit(const struct unit *caravan, 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 = + unit_current_good_replace_priority(pgood); + 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), nullptr, can_establish); @@ -306,10 +307,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 = unit_current_good_replace_priority(pgood); struct trade_route_list *would_remove = (countloser ? trade_route_list_new() : nullptr); - 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 */ @@ -357,7 +358,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, + unit_current_good_replace_priority(pgood))) { return 0; } if (max_trade_routes(src) <= 0 || max_trade_routes(dest) <= 0) { diff --git a/common/traderoutes.c b/common/traderoutes.c index e2c9963d5d..0177d2b5e1 100644 --- a/common/traderoutes.c +++ b/common/traderoutes.c @@ -736,11 +736,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 NULL 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 NULL *************************************************************************/ struct goods_type *goods_from_city_to_unit(const struct city *src, const struct unit *punit) @@ -772,10 +773,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 NULL 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 NULL *************************************************************************/ 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 1b484d1613..d70c2a5be0 100644 --- a/common/unit.c +++ b/common/unit.c @@ -2975,3 +2975,11 @@ int unit_upkeep_cost(const struct unit *punit, Output_type_id otype) return utype_upkeep_cost(unit_type_get(punit), punit, unit_owner(punit), otype); } + +/**********************************************************************//** + Returns the *pgood's replace_priority, or -1 if pgood is nullptr +**************************************************************************/ +int unit_current_good_replace_priority(const struct goods_type *pgood) +{ + return pgood != nullptr ? pgood->replace_priority : -1; +} diff --git a/common/unit.h b/common/unit.h index 4c82e2b6c9..f838facdc9 100644 --- a/common/unit.h +++ b/common/unit.h @@ -609,6 +609,8 @@ 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 goods_type *pgood); + #ifdef __cplusplus } #endif /* __cplusplus */ -- 2.31.0