From a21cecc5f0403cf1c7fae802ce5f0b0ab682a3a9 Mon Sep 17 00:00:00 2001 From: Dino Date: Sun, 11 Oct 2026 20:00:15 -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 3e78fc8937..897f89715f 100644 --- a/ai/default/daiunit.c +++ b/ai/default/daiunit.c @@ -2382,13 +2382,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 == 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..81715dedf1 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 70e2149ca9..93919fd3a8 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 f7e12acd38..dc3d6e2be2 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 ae95369ef7..7f04935cfd 100644 --- a/common/actres.c +++ b/common/actres.c @@ -924,9 +924,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 0a40b640bf..6a6c689a6c 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), NULL, 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() : 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 */ @@ -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 c81a0d743b..accfa8ce36 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 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) @@ -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 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 ee0de992ee..7ab97f7b10 100644 --- a/common/unit.c +++ b/common/unit.c @@ -2976,3 +2976,11 @@ enum gen_action activity_default_action(enum unit_activity act) return act_act[act]; } + +/**********************************************************************//** + 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 f9603c4632..fca091744f 100644 --- a/common/unit.h +++ b/common/unit.h @@ -594,6 +594,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