From 9a3db25e6c932476fe907e834d9c9e64e6cb4cc4 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Thu, 25 Apr 2024 08:20:46 +0100 Subject: [PATCH] Codechange: Make sort list function lists safer. GUIList has a pointer only to the start of each sort/filter func list, which has the potential for UB as it is unable to validate that the selected sort or filter type is in range. Use a std::span instead and check if the selected type is in range before using it. --- src/bridge_gui.cpp | 4 ++-- src/industry_gui.cpp | 4 ++-- src/network/network_content_gui.cpp | 8 ++++---- src/network/network_gui.cpp | 12 +++++------ src/newgrf_gui.cpp | 8 ++++---- src/object_gui.cpp | 8 ++++---- src/rail_gui.cpp | 8 ++++---- src/road_gui.cpp | 8 ++++---- src/sortlist_type.h | 32 ++++++++++++++++------------- src/station_gui.cpp | 4 ++-- src/story_gui.cpp | 8 ++++---- src/town_gui.cpp | 4 ++-- src/vehicle_gui.cpp | 4 ++-- src/vehicle_gui_base.h | 6 +++--- 14 files changed, 61 insertions(+), 57 deletions(-) diff --git a/src/bridge_gui.cpp b/src/bridge_gui.cpp index 1eba99ddce..6510355f8c 100644 --- a/src/bridge_gui.cpp +++ b/src/bridge_gui.cpp @@ -76,7 +76,7 @@ private: /* Constants for sorting the bridges */ static const StringID sorter_names[]; - static GUIBridgeList::SortFunction * const sorter_funcs[]; + static const std::initializer_list sorter_funcs; /* Internal variables */ TileIndex start_tile; @@ -298,7 +298,7 @@ public: Listing BuildBridgeWindow::last_sorting = {true, 2}; /** Available bridge sorting functions. */ -GUIBridgeList::SortFunction * const BuildBridgeWindow::sorter_funcs[] = { +const std::initializer_list BuildBridgeWindow::sorter_funcs = { &BridgeIndexSorter, &BridgePriceSorter, &BridgeSpeedSorter diff --git a/src/industry_gui.cpp b/src/industry_gui.cpp index ade687df5f..7d85c5eded 100644 --- a/src/industry_gui.cpp +++ b/src/industry_gui.cpp @@ -1326,7 +1326,7 @@ protected: /* Constants for sorting industries */ static const StringID sorter_names[]; - static GUIIndustryList::SortFunction * const sorter_funcs[]; + static const std::initializer_list sorter_funcs; GUIIndustryList industries{IndustryDirectoryWindow::produced_cargo_filter}; Scrollbar *vscroll; @@ -1902,7 +1902,7 @@ public: Listing IndustryDirectoryWindow::last_sorting = {false, 0}; /* Available station sorting functions. */ -GUIIndustryList::SortFunction * const IndustryDirectoryWindow::sorter_funcs[] = { +const std::initializer_list IndustryDirectoryWindow::sorter_funcs = { &IndustryNameSorter, &IndustryTypeSorter, &IndustryProductionSorter, diff --git a/src/network/network_content_gui.cpp b/src/network/network_content_gui.cpp index d6a15f44fa..8310d7afed 100644 --- a/src/network/network_content_gui.cpp +++ b/src/network/network_content_gui.cpp @@ -334,8 +334,8 @@ class NetworkContentListWindow : public Window, ContentCallback { static Listing last_sorting; ///< The last sorting setting. static Filtering last_filtering; ///< The last filtering setting. - static GUIContentList::SortFunction * const sorter_funcs[]; ///< Sorter functions - static GUIContentList::FilterFunction * const filter_funcs[]; ///< Filter functions. + static const std::initializer_list sorter_funcs; ///< Sorter functions + static const std::initializer_list filter_funcs; ///< Filter functions. GUIContentList content; ///< List with content bool auto_select; ///< Automatically select all content when the meta-data becomes available ContentListFilterData filter_data; ///< Filter for content list @@ -1010,13 +1010,13 @@ public: Listing NetworkContentListWindow::last_sorting = {false, 1}; Filtering NetworkContentListWindow::last_filtering = {false, 0}; -NetworkContentListWindow::GUIContentList::SortFunction * const NetworkContentListWindow::sorter_funcs[] = { +const std::initializer_list NetworkContentListWindow::sorter_funcs = { &StateSorter, &TypeSorter, &NameSorter, }; -NetworkContentListWindow::GUIContentList::FilterFunction * const NetworkContentListWindow::filter_funcs[] = { +const std::initializer_list NetworkContentListWindow::filter_funcs = { &TagNameFilter, &TypeOrSelectedFilter, }; diff --git a/src/network/network_gui.cpp b/src/network/network_gui.cpp index 3421b9b56d..3822242b70 100644 --- a/src/network/network_gui.cpp +++ b/src/network/network_gui.cpp @@ -181,8 +181,8 @@ protected: static Listing last_sorting; /* Constants for sorting servers */ - static GUIGameServerList::SortFunction * const sorter_funcs[]; - static GUIGameServerList::FilterFunction * const filter_funcs[]; + static const std::initializer_list sorter_funcs; + static const std::initializer_list filter_funcs; NetworkGameList *server; ///< Selected server. NetworkGameList *last_joined; ///< The last joined server. @@ -459,8 +459,8 @@ public: this->server = this->last_joined; this->servers.SetListing(this->last_sorting); - this->servers.SetSortFuncs(this->sorter_funcs); - this->servers.SetFilterFuncs(this->filter_funcs); + this->servers.SetSortFuncs(NetworkGameWindow::sorter_funcs); + this->servers.SetFilterFuncs(NetworkGameWindow::filter_funcs); this->servers.ForceRebuild(); } @@ -860,7 +860,7 @@ public: }; Listing NetworkGameWindow::last_sorting = {false, 5}; -GUIGameServerList::SortFunction * const NetworkGameWindow::sorter_funcs[] = { +const std::initializer_list NetworkGameWindow::sorter_funcs = { &NGameNameSorter, &NGameClientSorter, &NGameMapSizeSorter, @@ -869,7 +869,7 @@ GUIGameServerList::SortFunction * const NetworkGameWindow::sorter_funcs[] = { &NGameAllowedSorter }; -GUIGameServerList::FilterFunction * const NetworkGameWindow::filter_funcs[] = { +const std::initializer_list NetworkGameWindow::filter_funcs = { &NGameSearchFilter }; diff --git a/src/newgrf_gui.cpp b/src/newgrf_gui.cpp index 146f890189..234ec24020 100644 --- a/src/newgrf_gui.cpp +++ b/src/newgrf_gui.cpp @@ -609,8 +609,8 @@ struct NewGRFWindow : public Window, NewGRFScanCallback { static Listing last_sorting; ///< Default sorting of #GUIGRFConfigList. static Filtering last_filtering; ///< Default filtering of #GUIGRFConfigList. - static GUIGRFConfigList::SortFunction * const sorter_funcs[]; ///< Sort functions of the #GUIGRFConfigList. - static GUIGRFConfigList::FilterFunction * const filter_funcs[]; ///< Filter functions of the #GUIGRFConfigList. + static const std::initializer_list sorter_funcs; ///< Sort functions of the #GUIGRFConfigList. + static const std::initializer_list filter_funcs; ///< Filter functions of the #GUIGRFConfigList. GUIGRFConfigList avails; ///< Available (non-active) grfs. const GRFConfig *avail_sel; ///< Currently selected available grf. \c nullptr is none is selected. @@ -1586,11 +1586,11 @@ void ShowMissingContentWindow(const GRFConfig *list) Listing NewGRFWindow::last_sorting = {false, 0}; Filtering NewGRFWindow::last_filtering = {false, 0}; -NewGRFWindow::GUIGRFConfigList::SortFunction * const NewGRFWindow::sorter_funcs[] = { +const std::initializer_list NewGRFWindow::sorter_funcs = { &NameSorter, }; -NewGRFWindow::GUIGRFConfigList::FilterFunction * const NewGRFWindow::filter_funcs[] = { +const std::initializer_list NewGRFWindow::filter_funcs = { &TagNameFilter, }; diff --git a/src/object_gui.cpp b/src/object_gui.cpp index 9199c55538..91ed77860f 100644 --- a/src/object_gui.cpp +++ b/src/object_gui.cpp @@ -56,8 +56,8 @@ class BuildObjectWindow : public Window { static Listing last_sorting; ///< Default sorting of #GUIObjectClassList. static Filtering last_filtering; ///< Default filtering of #GUIObjectClassList. - static GUIObjectClassList::SortFunction * const sorter_funcs[]; ///< Sort functions of the #GUIObjectClassList. - static GUIObjectClassList::FilterFunction * const filter_funcs[]; ///< Filter functions of the #GUIObjectClassList. + static const std::initializer_list sorter_funcs; ///< Sort functions of the #GUIObjectClassList. + static const std::initializer_list filter_funcs; ///< Filter functions of the #GUIObjectClassList. GUIObjectClassList object_classes; ///< Available object classes. StringFilter string_filter; ///< Filter for available objects. QueryString filter_editbox; ///< Filter editbox. @@ -655,11 +655,11 @@ public: Listing BuildObjectWindow::last_sorting = { false, 0 }; Filtering BuildObjectWindow::last_filtering = { false, 0 }; -BuildObjectWindow::GUIObjectClassList::SortFunction * const BuildObjectWindow::sorter_funcs[] = { +const std::initializer_list BuildObjectWindow::sorter_funcs = { &ObjectClassIDSorter, }; -BuildObjectWindow::GUIObjectClassList::FilterFunction * const BuildObjectWindow::filter_funcs[] = { +const std::initializer_list BuildObjectWindow::filter_funcs = { &TagNameFilter, }; diff --git a/src/rail_gui.cpp b/src/rail_gui.cpp index 7cf1dfeb3c..5cdfb1fb01 100644 --- a/src/rail_gui.cpp +++ b/src/rail_gui.cpp @@ -964,8 +964,8 @@ private: static Listing last_sorting; ///< Default sorting of #GUIStationClassList. static Filtering last_filtering; ///< Default filtering of #GUIStationClassList. - static GUIStationClassList::SortFunction * const sorter_funcs[]; ///< Sort functions of the #GUIStationClassList. - static GUIStationClassList::FilterFunction * const filter_funcs[]; ///< Filter functions of the #GUIStationClassList. + static const std::initializer_list sorter_funcs; ///< Sort functions of the #GUIStationClassList. + static const std::initializer_list filter_funcs; ///< Filter functions of the #GUIStationClassList. GUIStationClassList station_classes; ///< Available station classes. StringFilter string_filter; ///< Filter for available station classes. QueryString filter_editbox; ///< Filter editbox. @@ -1571,11 +1571,11 @@ public: Listing BuildRailStationWindow::last_sorting = { false, 0 }; Filtering BuildRailStationWindow::last_filtering = { false, 0 }; -BuildRailStationWindow::GUIStationClassList::SortFunction * const BuildRailStationWindow::sorter_funcs[] = { +const std::initializer_list BuildRailStationWindow::sorter_funcs = { &StationClassIDSorter, }; -BuildRailStationWindow::GUIStationClassList::FilterFunction * const BuildRailStationWindow::filter_funcs[] = { +const std::initializer_list BuildRailStationWindow::filter_funcs = { &TagNameFilter, }; diff --git a/src/road_gui.cpp b/src/road_gui.cpp index 8637c910f1..c2280a559f 100644 --- a/src/road_gui.cpp +++ b/src/road_gui.cpp @@ -1112,8 +1112,8 @@ private: static Listing last_sorting; ///< Default sorting of #GUIRoadStopClassList. static Filtering last_filtering; ///< Default filtering of #GUIRoadStopClassList. - static GUIRoadStopClassList::SortFunction * const sorter_funcs[]; ///< Sort functions of the #GUIRoadStopClassList. - static GUIRoadStopClassList::FilterFunction * const filter_funcs[]; ///< Filter functions of the #GUIRoadStopClassList. + static const std::initializer_list sorter_funcs; ///< Sort functions of the #GUIRoadStopClassList. + static const std::initializer_list filter_funcs; ///< Filter functions of the #GUIRoadStopClassList. GUIRoadStopClassList roadstop_classes; ///< Available road stop classes. StringFilter string_filter; ///< Filter for available road stop classes. QueryString filter_editbox; ///< Filter editbox. @@ -1599,11 +1599,11 @@ public: Listing BuildRoadStationWindow::last_sorting = { false, 0 }; Filtering BuildRoadStationWindow::last_filtering = { false, 0 }; -BuildRoadStationWindow::GUIRoadStopClassList::SortFunction * const BuildRoadStationWindow::sorter_funcs[] = { +const std::initializer_list BuildRoadStationWindow::sorter_funcs = { &RoadStopClassIDSorter, }; -BuildRoadStationWindow::GUIRoadStopClassList::FilterFunction * const BuildRoadStationWindow::filter_funcs[] = { +const std::initializer_list BuildRoadStationWindow::filter_funcs = { &TagNameFilter, }; diff --git a/src/sortlist_type.h b/src/sortlist_type.h index c0fc465525..8edaa91435 100644 --- a/src/sortlist_type.h +++ b/src/sortlist_type.h @@ -50,8 +50,8 @@ public: typedef bool CDECL FilterFunction(const T*, F); ///< Signature of filter function. protected: - SortFunction * const *sort_func_list; ///< the sort criteria functions - FilterFunction * const *filter_func_list; ///< the filter criteria functions + std::span sort_func_list; ///< the sort criteria functions + std::span filter_func_list; ///< the filter criteria functions SortListFlags flags; ///< used to control sorting/resorting/etc. uint8_t sort_type; ///< what criteria to sort on uint8_t filter_type; ///< what criteria to filter on @@ -85,8 +85,8 @@ public: /* If sort parameters are not used then we don't require a reference to the params. */ template >* = nullptr> GUIList() : - sort_func_list(nullptr), - filter_func_list(nullptr), + sort_func_list({}), + filter_func_list({}), flags(VL_NONE), sort_type(0), filter_type(0), @@ -97,8 +97,8 @@ public: /* If sort parameters are used then we require a reference to the params. */ template >* = nullptr> GUIList(const P ¶ms) : - sort_func_list(nullptr), - filter_func_list(nullptr), + sort_func_list({}), + filter_func_list({}), flags(VL_NONE), sort_type(0), filter_type(0), @@ -123,6 +123,7 @@ public: */ void SetSortType(uint8_t n_type) { + assert(n_type < std::size(this->sort_func_list)); if (this->sort_type != n_type) { SETBITS(this->flags, VL_RESORT); this->sort_type = n_type; @@ -175,6 +176,7 @@ public: */ void SetFilterType(uint8_t n_type) { + assert(n_type < std::size(this->filter_func_list)); if (this->filter_type != n_type) { this->filter_type = n_type; } @@ -288,11 +290,11 @@ public: } /** - * Hand the array of sort function pointers to the sort list + * Hand the sort function pointers to the GUIList. * - * @param n_funcs The pointer to the first sort func + * @param n_funcs Span covering the sort function pointers. */ - void SetSortFuncs(SortFunction * const *n_funcs) + void SetSortFuncs(std::span n_funcs) { this->sort_func_list = n_funcs; } @@ -305,7 +307,8 @@ public: */ bool Sort() { - assert(this->sort_func_list != nullptr); + if (this->sort_func_list.empty()) return false; + assert(this->sort_type < this->sort_func_list.size()); return this->Sort(this->sort_func_list[this->sort_type]); } @@ -359,11 +362,11 @@ public: } /** - * Hand the array of filter function pointers to the sort list + * Hand the filter function pointers to the GUIList. * - * @param n_funcs The pointer to the first filter func + * @param n_funcs Span covering the filter function pointers. */ - void SetFilterFuncs(FilterFunction * const *n_funcs) + void SetFilterFuncs(std::span n_funcs) { this->filter_func_list = n_funcs; } @@ -376,7 +379,8 @@ public: */ bool Filter(F filter_data) { - if (this->filter_func_list == nullptr) return false; + if (this->filter_func_list.empty()) return false; + assert(this->filter_type < this->filter_func_list.size()); return this->Filter(this->filter_func_list[this->filter_type], filter_data); } diff --git a/src/station_gui.cpp b/src/station_gui.cpp index 526c5c16a2..6db2d27cb5 100644 --- a/src/station_gui.cpp +++ b/src/station_gui.cpp @@ -245,7 +245,7 @@ protected: /* Constants for sorting stations */ static const StringID sorter_names[]; - static GUIStationList::SortFunction * const sorter_funcs[]; + static const std::initializer_list sorter_funcs; FilterState filter; GUIStationList stations{filter.cargoes}; @@ -720,7 +720,7 @@ public: }; /* Available station sorting functions */ -GUIStationList::SortFunction * const CompanyStationsWindow::sorter_funcs[] = { +const std::initializer_list CompanyStationsWindow::sorter_funcs = { &StationNameSorter, &StationTypeSorter, &StationWaitingTotalSorter, diff --git a/src/story_gui.cpp b/src/story_gui.cpp index 9a91eb7128..807a4198a7 100644 --- a/src/story_gui.cpp +++ b/src/story_gui.cpp @@ -62,8 +62,8 @@ protected: StoryPageElementID active_button_id; ///< Which button element the player is currently using - static GUIStoryPageList::SortFunction * const page_sorter_funcs[]; - static GUIStoryPageElementList::SortFunction * const page_element_sorter_funcs[]; + static const std::initializer_list page_sorter_funcs; + static const std::initializer_list page_element_sorter_funcs; /** (Re)Build story page list. */ void BuildStoryPageList() @@ -939,11 +939,11 @@ public: } }; -GUIStoryPageList::SortFunction * const StoryBookWindow::page_sorter_funcs[] = { +const std::initializer_list StoryBookWindow::page_sorter_funcs = { &PageOrderSorter, }; -GUIStoryPageElementList::SortFunction * const StoryBookWindow::page_element_sorter_funcs[] = { +const std::initializer_list StoryBookWindow::page_element_sorter_funcs = { &PageElementOrderSorter, }; diff --git a/src/town_gui.cpp b/src/town_gui.cpp index 5ee4ce28e2..69af8c411b 100644 --- a/src/town_gui.cpp +++ b/src/town_gui.cpp @@ -721,7 +721,7 @@ private: /* Constants for sorting towns */ static const StringID sorter_names[]; - static GUITownList::SortFunction * const sorter_funcs[]; + static const std::initializer_list sorter_funcs; StringFilter string_filter; ///< Filter for towns QueryString townname_editbox; ///< Filter editbox @@ -1054,7 +1054,7 @@ const StringID TownDirectoryWindow::sorter_names[] = { }; /** Available town directory sorting functions. */ -GUITownList::SortFunction * const TownDirectoryWindow::sorter_funcs[] = { +const std::initializer_list TownDirectoryWindow::sorter_funcs = { &TownNameSorter, &TownPopulationSorter, &TownRatingSorter, diff --git a/src/vehicle_gui.cpp b/src/vehicle_gui.cpp index 93efd79c4e..00060351eb 100644 --- a/src/vehicle_gui.cpp +++ b/src/vehicle_gui.cpp @@ -78,7 +78,7 @@ static bool VehicleIndividualToGroupSorterWrapper(GUIVehicleGroup const &a, GUIV return func(*(a.vehicles_begin), *(b.vehicles_begin)); } -BaseVehicleListWindow::VehicleGroupSortFunction * const BaseVehicleListWindow::vehicle_group_none_sorter_funcs[] = { +const std::initializer_list BaseVehicleListWindow::vehicle_group_none_sorter_funcs = { &VehicleIndividualToGroupSorterWrapper, &VehicleIndividualToGroupSorterWrapper, &VehicleIndividualToGroupSorterWrapper, @@ -128,7 +128,7 @@ const StringID BaseVehicleListWindow::vehicle_group_none_sorter_names_wallclock[ INVALID_STRING_ID }; -BaseVehicleListWindow::VehicleGroupSortFunction * const BaseVehicleListWindow::vehicle_group_shared_orders_sorter_funcs[] = { +const std::initializer_list BaseVehicleListWindow::vehicle_group_shared_orders_sorter_funcs = { &VehicleGroupLengthSorter, &VehicleGroupTotalProfitThisYearSorter, &VehicleGroupTotalProfitLastYearSorter, diff --git a/src/vehicle_gui_base.h b/src/vehicle_gui_base.h index c1a9c7a246..4b4519b225 100644 --- a/src/vehicle_gui_base.h +++ b/src/vehicle_gui_base.h @@ -103,8 +103,8 @@ struct BaseVehicleListWindow : public Window { static const StringID vehicle_group_none_sorter_names_wallclock[]; static const StringID vehicle_group_shared_orders_sorter_names_calendar[]; static const StringID vehicle_group_shared_orders_sorter_names_wallclock[]; - static VehicleGroupSortFunction * const vehicle_group_none_sorter_funcs[]; - static VehicleGroupSortFunction * const vehicle_group_shared_orders_sorter_funcs[]; + static const std::initializer_list vehicle_group_none_sorter_funcs; + static const std::initializer_list vehicle_group_shared_orders_sorter_funcs; BaseVehicleListWindow(WindowDesc *desc, WindowNumber wno); @@ -126,7 +126,7 @@ struct BaseVehicleListWindow : public Window { const StringID *GetVehicleSorterNames(); - VehicleGroupSortFunction * const *GetVehicleSorterFuncs() + std::span GetVehicleSorterFuncs() { switch (this->grouping) { case GB_NONE: