From 90659e5808b8f9037a3a4aa6c38aee115623f1c2 Mon Sep 17 00:00:00 2001 From: Marko Lindqvist <cazfi74@gmail.com> Date: Wed, 23 Nov 2022 21:37:07 +0200 Subject: [PATCH 21/21] Move PACKET_CITY_RALLY_POINT unpacking to common/ See osdn #46101 Signed-off-by: Marko Lindqvist <cazfi74@gmail.com> --- common/city.c | 46 ++++++++ common/city.h | 6 +- common/networking/packets.def | 2 +- common/unit.c | 211 ++++++++++++++++++++++++++++++++++ common/unit.h | 6 +- server/cityhand.c | 47 +------- server/unittools.c | 210 --------------------------------- server/unittools.h | 6 +- 8 files changed, 274 insertions(+), 260 deletions(-) diff --git a/common/city.c b/common/city.c index 8b13bc9b44..deccceb421 100644 --- a/common/city.c +++ b/common/city.c @@ -3478,3 +3478,49 @@ void city_rally_point_clear(struct city *pcity) pcity->rally_point.orders = NULL; } } + +/**********************************************************************//** + Fill city rally point from the packet. +**************************************************************************/ +void city_rally_point_receive(const struct packet_city_rally_point *packet, + struct city *pcity) +{ + struct unit_order *checked_orders; + + if (NULL != pcity) { + /* Probably lost. */ + log_verbose("handle_city_rally_point() bad city number %d.", + packet->city_id); + return; + } + + if (0 > packet->length || MAX_LEN_ROUTE < packet->length) { + /* Shouldn't happen */ + log_error("city_rally_point_receive() invalid packet length %d (max %d)", + packet->length, MAX_LEN_ROUTE); + return; + } + + pcity->rally_point.length = packet->length; + + if (packet->length == 0) { + pcity->rally_point.vigilant = FALSE; + pcity->rally_point.persistent = FALSE; + if (pcity->rally_point.orders) { + free(pcity->rally_point.orders); + pcity->rally_point.orders = NULL; + } + } else { + checked_orders = create_unit_orders(packet->length, packet->orders); + if (!checked_orders) { + pcity->rally_point.length = 0; + log_error("invalid rally point orders for city number %d.", + packet->city_id); + return; + } + + pcity->rally_point.persistent = packet->persistent; + pcity->rally_point.vigilant = packet->vigilant; + pcity->rally_point.orders = checked_orders; + } +} diff --git a/common/city.h b/common/city.h index 7ed515bcac..d6f6ba9e84 100644 --- a/common/city.h +++ b/common/city.h @@ -33,6 +33,8 @@ extern "C" { #include "workertask.h" #include "worklist.h" +struct packet_city_rally_point; + enum production_class_type { PCT_UNIT, PCT_NORMAL_IMPROVEMENT, @@ -802,9 +804,11 @@ void city_set_ai_data(struct city *pcity, const struct ai_type *ai, void *data); void city_rally_point_clear(struct city *pcity); +void city_rally_point_receive(const struct packet_city_rally_point *packet, + struct city *pcity); #ifdef __cplusplus } #endif /* __cplusplus */ -#endif /* FC__CITY_H */ +#endif /* FC__CITY_H */ diff --git a/common/networking/packets.def b/common/networking/packets.def index 2cfc9bfd21..6450d1c421 100644 --- a/common/networking/packets.def +++ b/common/networking/packets.def @@ -873,7 +873,7 @@ PACKET_CITY_SABOTAGE_LIST = 45; sc, lsend UINT8 request_kind; end -PACKET_CITY_RALLY_POINT = 138; cs +PACKET_CITY_RALLY_POINT = 138; cs, handle-via-packet CITY city_id; UINT16 length; BOOL persistent; diff --git a/common/unit.c b/common/unit.c index a561fd23d5..71d8929910 100644 --- a/common/unit.c +++ b/common/unit.c @@ -2522,3 +2522,214 @@ bool unit_is_cityfounder(const struct unit *punit) { return utype_is_cityfounder(unit_type_get(punit)); } + + +/**********************************************************************//** + Returns TRUE iff the unit order array is sane. +**************************************************************************/ +bool unit_order_list_is_sane(int length, const struct unit_order *orders) +{ + int i; + + for (i = 0; i < length; i++) { + struct action *paction; + struct extra_type *pextra; + + if (orders[i].order > ORDER_LAST) { + log_error("invalid order %d at index %d", orders[i].order, i); + return FALSE; + } + switch (orders[i].order) { + case ORDER_MOVE: + case ORDER_ACTION_MOVE: + if (!map_untrusted_dir_is_valid(orders[i].dir)) { + log_error("in order %d, invalid move direction %d.", i, orders[i].dir); + return FALSE; + } + break; + case ORDER_ACTIVITY: + switch (orders[i].activity) { + case ACTIVITY_SENTRY: + if (i != length - 1) { + /* Only allowed as the last order. */ + log_error("activity %d is not allowed at index %d.", orders[i].activity, + i); + return FALSE; + } + break; + /* Replaced by action orders */ + case ACTIVITY_BASE: + case ACTIVITY_GEN_ROAD: + case ACTIVITY_FALLOUT: + case ACTIVITY_POLLUTION: + case ACTIVITY_PILLAGE: + case ACTIVITY_MINE: + case ACTIVITY_IRRIGATE: + case ACTIVITY_PLANT: + case ACTIVITY_CULTIVATE: + case ACTIVITY_TRANSFORM: + case ACTIVITY_CONVERT: + case ACTIVITY_FORTIFYING: + log_error("at index %d, use action rather than activity %d.", + i, orders[i].activity); + return FALSE; + /* Not supported. */ + case ACTIVITY_EXPLORE: + case ACTIVITY_IDLE: + /* Not set from the client. */ + case ACTIVITY_GOTO: + case ACTIVITY_FORTIFIED: + /* Compatiblity, used in savegames. */ + case ACTIVITY_OLD_ROAD: + case ACTIVITY_OLD_RAILROAD: + case ACTIVITY_FORTRESS: + case ACTIVITY_AIRBASE: + /* Unused. */ + case ACTIVITY_PATROL_UNUSED: + case ACTIVITY_LAST: + case ACTIVITY_UNKNOWN: + log_error("at index %d, unsupported activity %d.", i, orders[i].activity); + return FALSE; + } + + break; + case ORDER_PERFORM_ACTION: + if (!action_id_exists(orders[i].action)) { + /* Non-existent action. */ + log_error("at index %d, the action %d doesn't exist.", i, orders[i].action); + return FALSE; + } + + paction = action_by_number(orders[i].action); + + /* Validate main target. */ + if (index_to_tile(&(wld.map), orders[i].target) == NULL) { + log_error("at index %d, invalid tile target %d for the action %d.", + i, orders[i].target, orders[i].action); + return FALSE; + } + + if (orders[i].dir != DIR8_ORIGIN) { + log_error("at index %d, the action %d sets the outdated target" + " specification dir.", + i, orders[i].action); + } + + /* Validate sub target. */ + switch (action_id_get_sub_target_kind(orders[i].action)) { + case ASTK_BUILDING: + /* Sub target is a building. */ + if (!improvement_by_number(orders[i].sub_target)) { + /* Sub target is invalid. */ + log_error("at index %d, cannot do %s without a target.", i, + action_id_rule_name(orders[i].action)); + return FALSE; + } + break; + case ASTK_TECH: + /* Sub target is a technology. */ + if (orders[i].sub_target == A_NONE + || (!valid_advance_by_number(orders[i].sub_target) + && orders[i].sub_target != A_FUTURE)) { + /* Target tech is invalid. */ + log_error("at index %d, cannot do %s without a target.", i, + action_id_rule_name(orders[i].action)); + return FALSE; + } + break; + case ASTK_EXTRA: + case ASTK_EXTRA_NOT_THERE: + /* Sub target is an extra. */ + pextra = (!(orders[i].sub_target == NO_TARGET + || (orders[i].sub_target < 0 + || (orders[i].sub_target + >= game.control.num_extra_types))) + ? extra_by_number(orders[i].sub_target) : NULL); + fc_assert(pextra == NULL || !(pextra->ruledit_disabled)); + if (pextra == NULL) { + if (paction->target_complexity != ACT_TGT_COMPL_FLEXIBLE) { + /* Target extra is invalid. */ + log_error("at index %d, cannot do %s without a target.", i, + action_id_rule_name(orders[i].action)); + return FALSE; + } + } else { + if (!(action_removes_extra(paction, pextra) + || action_creates_extra(paction, pextra))) { + /* Target extra is irrelevant for the action. */ + log_error("at index %d, cannot do %s to %s.", i, + action_id_rule_name(orders[i].action), + extra_rule_name(pextra)); + return FALSE; + } + } + break; + case ASTK_NONE: + /* No validation required. */ + break; + /* Invalid action? */ + case ASTK_COUNT: + fc_assert_ret_val_msg( + action_id_get_sub_target_kind(orders[i].action) != ASTK_COUNT, + FALSE, + "Bad action %d in order number %d.", orders[i].action, i); + } + + /* Some action orders are sane only in the last order. */ + if (i != length - 1) { + /* If the unit is dead, */ + if (utype_is_consumed_by_action(paction, NULL) + /* or if Freeciv has no idea where the unit will end up after it + * has performed this action, */ + || !(utype_is_unmoved_by_action(paction, NULL) + || utype_is_moved_to_tgt_by_action(paction, NULL)) + /* or if the unit will end up standing still, */ + || action_has_result(paction, ACTRES_FORTIFY)) { + /* than having this action in the middle of a unit's orders is + * probably wrong. */ + log_error("action %d is not allowed at index %d.", + orders[i].action, i); + return FALSE; + } + } + + /* Don't validate that the target tile really contains a target or + * that the actor player's map think the target tile has one. + * The player may target something from their player map that isn't + * there any more, a target they think is there even if their player + * map doesn't have it, or even a target they assume will be there + * when the unit reaches the target tile. + * + * With that said: The client should probably at least have an + * option to only aim city targeted actions at cities. */ + + break; + case ORDER_FULL_MP: + break; + case ORDER_LAST: + /* An invalid order. This is handled above. */ + break; + } + } + + return TRUE; +} + +/**********************************************************************//** + Sanity-check unit order arrays from a packet and create a unit_order array + from their contents if valid. +**************************************************************************/ +struct unit_order *create_unit_orders(int length, + const struct unit_order *orders) +{ + struct unit_order *unit_orders; + + if (!unit_order_list_is_sane(length, orders)) { + return NULL; + } + + unit_orders = fc_malloc(length * sizeof(*(unit_orders))); + memcpy(unit_orders, orders, length * sizeof(*(unit_orders))); + + return unit_orders; +} diff --git a/common/unit.h b/common/unit.h index ce0df6747d..2ac7d8cc26 100644 --- a/common/unit.h +++ b/common/unit.h @@ -474,8 +474,12 @@ struct iterator *cargo_iter_init(struct cargo_iter *iter, cargo_iter_sizeof, cargo_iter_init, _ptrans) #define unit_cargo_iterate_end generic_iterate_end +bool unit_order_list_is_sane(int length, const struct unit_order *orders); +struct unit_order *create_unit_orders(int length, + const struct unit_order *orders); + #ifdef __cplusplus } #endif /* __cplusplus */ -#endif /* FC__UNIT_H */ +#endif /* FC__UNIT_H */ diff --git a/server/cityhand.c b/server/cityhand.c index 137534972d..d7f61b1127 100644 --- a/server/cityhand.c +++ b/server/cityhand.c @@ -514,49 +514,12 @@ void handle_city_options_req(struct player *pplayer, int city_id, Handles a request to set city rally point for new units. **************************************************************************/ void handle_city_rally_point(struct player *pplayer, - int city_id, int length, - bool persistent, bool vigilant, - const struct unit_order *orders) + const struct packet_city_rally_point *packet) { - struct city *pcity = player_city_by_number(pplayer, city_id); - struct unit_order *checked_orders; - - if (NULL == pcity) { - /* Probably lost. */ - log_verbose("handle_city_rally_point() bad city number %d.", - city_id); - return; - } - - if (0 > length || MAX_LEN_ROUTE < length) { - /* Shouldn't happen */ - log_error("handle_city_rally_point() invalid packet length %d (max %d)", - length, MAX_LEN_ROUTE); - return; - } + struct city *pcity = player_city_by_number(pplayer, packet->city_id); - pcity->rally_point.length = length; - - if (length == 0) { - pcity->rally_point.vigilant = FALSE; - pcity->rally_point.persistent = FALSE; - if (pcity->rally_point.orders) { - free(pcity->rally_point.orders); - pcity->rally_point.orders = NULL; - } - } else { - checked_orders = create_unit_orders(length, orders); - if (!checked_orders) { - pcity->rally_point.length = 0; - log_error("invalid rally point orders for city number %d.", - city_id); - return; - } - - pcity->rally_point.persistent = persistent; - pcity->rally_point.vigilant = vigilant; - pcity->rally_point.orders = checked_orders; + if (NULL != pcity) { + city_rally_point_receive(packet, pcity); + send_city_info(pplayer, pcity); } - - send_city_info(pplayer, pcity); } diff --git a/server/unittools.c b/server/unittools.c index 6f841a888b..1207671876 100644 --- a/server/unittools.c +++ b/server/unittools.c @@ -4807,213 +4807,3 @@ bool unit_can_be_retired(struct unit *punit) return TRUE; } - -/**********************************************************************//** - Returns TRUE iff the unit order array is sane. -**************************************************************************/ -bool unit_order_list_is_sane(int length, const struct unit_order *orders) -{ - int i; - - for (i = 0; i < length; i++) { - struct action *paction; - struct extra_type *pextra; - - if (orders[i].order > ORDER_LAST) { - log_error("invalid order %d at index %d", orders[i].order, i); - return FALSE; - } - switch (orders[i].order) { - case ORDER_MOVE: - case ORDER_ACTION_MOVE: - if (!map_untrusted_dir_is_valid(orders[i].dir)) { - log_error("in order %d, invalid move direction %d.", i, orders[i].dir); - return FALSE; - } - break; - case ORDER_ACTIVITY: - switch (orders[i].activity) { - case ACTIVITY_SENTRY: - if (i != length - 1) { - /* Only allowed as the last order. */ - log_error("activity %d is not allowed at index %d.", orders[i].activity, - i); - return FALSE; - } - break; - /* Replaced by action orders */ - case ACTIVITY_BASE: - case ACTIVITY_GEN_ROAD: - case ACTIVITY_FALLOUT: - case ACTIVITY_POLLUTION: - case ACTIVITY_PILLAGE: - case ACTIVITY_MINE: - case ACTIVITY_IRRIGATE: - case ACTIVITY_PLANT: - case ACTIVITY_CULTIVATE: - case ACTIVITY_TRANSFORM: - case ACTIVITY_CONVERT: - case ACTIVITY_FORTIFYING: - log_error("at index %d, use action rather than activity %d.", - i, orders[i].activity); - return FALSE; - /* Not supported. */ - case ACTIVITY_EXPLORE: - case ACTIVITY_IDLE: - /* Not set from the client. */ - case ACTIVITY_GOTO: - case ACTIVITY_FORTIFIED: - /* Compatiblity, used in savegames. */ - case ACTIVITY_OLD_ROAD: - case ACTIVITY_OLD_RAILROAD: - case ACTIVITY_FORTRESS: - case ACTIVITY_AIRBASE: - /* Unused. */ - case ACTIVITY_PATROL_UNUSED: - case ACTIVITY_LAST: - case ACTIVITY_UNKNOWN: - log_error("at index %d, unsupported activity %d.", i, orders[i].activity); - return FALSE; - } - - break; - case ORDER_PERFORM_ACTION: - if (!action_id_exists(orders[i].action)) { - /* Non-existent action. */ - log_error("at index %d, the action %d doesn't exist.", i, orders[i].action); - return FALSE; - } - - paction = action_by_number(orders[i].action); - - /* Validate main target. */ - if (index_to_tile(&(wld.map), orders[i].target) == NULL) { - log_error("at index %d, invalid tile target %d for the action %d.", - i, orders[i].target, orders[i].action); - return FALSE; - } - - if (orders[i].dir != DIR8_ORIGIN) { - log_error("at index %d, the action %d sets the outdated target" - " specification dir.", - i, orders[i].action); - } - - /* Validate sub target. */ - switch (action_id_get_sub_target_kind(orders[i].action)) { - case ASTK_BUILDING: - /* Sub target is a building. */ - if (!improvement_by_number(orders[i].sub_target)) { - /* Sub target is invalid. */ - log_error("at index %d, cannot do %s without a target.", i, - action_id_rule_name(orders[i].action)); - return FALSE; - } - break; - case ASTK_TECH: - /* Sub target is a technology. */ - if (orders[i].sub_target == A_NONE - || (!valid_advance_by_number(orders[i].sub_target) - && orders[i].sub_target != A_FUTURE)) { - /* Target tech is invalid. */ - log_error("at index %d, cannot do %s without a target.", i, - action_id_rule_name(orders[i].action)); - return FALSE; - } - break; - case ASTK_EXTRA: - case ASTK_EXTRA_NOT_THERE: - /* Sub target is an extra. */ - pextra = (!(orders[i].sub_target == NO_TARGET - || (orders[i].sub_target < 0 - || (orders[i].sub_target - >= game.control.num_extra_types))) - ? extra_by_number(orders[i].sub_target) : NULL); - fc_assert(pextra == NULL || !(pextra->ruledit_disabled)); - if (pextra == NULL) { - if (paction->target_complexity != ACT_TGT_COMPL_FLEXIBLE) { - /* Target extra is invalid. */ - log_error("at index %d, cannot do %s without a target.", i, - action_id_rule_name(orders[i].action)); - return FALSE; - } - } else { - if (!(action_removes_extra(paction, pextra) - || action_creates_extra(paction, pextra))) { - /* Target extra is irrelevant for the action. */ - log_error("at index %d, cannot do %s to %s.", i, - action_id_rule_name(orders[i].action), - extra_rule_name(pextra)); - return FALSE; - } - } - break; - case ASTK_NONE: - /* No validation required. */ - break; - /* Invalid action? */ - case ASTK_COUNT: - fc_assert_ret_val_msg( - action_id_get_sub_target_kind(orders[i].action) != ASTK_COUNT, - FALSE, - "Bad action %d in order number %d.", orders[i].action, i); - } - - /* Some action orders are sane only in the last order. */ - if (i != length - 1) { - /* If the unit is dead, */ - if (utype_is_consumed_by_action(paction, NULL) - /* or if Freeciv has no idea where the unit will end up after it - * has performed this action, */ - || !(utype_is_unmoved_by_action(paction, NULL) - || utype_is_moved_to_tgt_by_action(paction, NULL)) - /* or if the unit will end up standing still, */ - || action_has_result(paction, ACTRES_FORTIFY)) { - /* than having this action in the middle of a unit's orders is - * probably wrong. */ - log_error("action %d is not allowed at index %d.", - orders[i].action, i); - return FALSE; - } - } - - /* Don't validate that the target tile really contains a target or - * that the actor player's map think the target tile has one. - * The player may target something from their player map that isn't - * there any more, a target they think is there even if their player - * map doesn't have it, or even a target they assume will be there - * when the unit reaches the target tile. - * - * With that said: The client should probably at least have an - * option to only aim city targeted actions at cities. */ - - break; - case ORDER_FULL_MP: - break; - case ORDER_LAST: - /* An invalid order. This is handled above. */ - break; - } - } - - return TRUE; -} - -/**********************************************************************//** - Sanity-check unit order arrays from a packet and create a unit_order array - from their contents if valid. -**************************************************************************/ -struct unit_order *create_unit_orders(int length, - const struct unit_order *orders) -{ - struct unit_order *unit_orders; - - if (!unit_order_list_is_sane(length, orders)) { - return NULL; - } - - unit_orders = fc_malloc(length * sizeof(*(unit_orders))); - memcpy(unit_orders, orders, length * sizeof(*(unit_orders))); - - return unit_orders; -} diff --git a/server/unittools.h b/server/unittools.h index 3dcc5dd0b2..da776124cc 100644 --- a/server/unittools.h +++ b/server/unittools.h @@ -185,8 +185,4 @@ void unit_activities_cancel_all_illegal_area(const struct tile *ptile); void unit_get_goods(struct unit *punit); -bool unit_order_list_is_sane(int length, const struct unit_order *orders); -struct unit_order *create_unit_orders(int length, - const struct unit_order *orders); - -#endif /* FC__UNITTOOLS_H */ +#endif /* FC__UNITTOOLS_H */ -- 2.35.1