From 7705265477cb6c5ced76d5faee3423f8555c6f1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gilles=20Roudi=C3=A8re?= Date: Thu, 9 May 2024 14:19:16 +0200 Subject: [PATCH] Fix PropertyListHelper::_get_property returning a valid value even if an index is outside the array valid indices Co-authored-by: Tomasz Chabora --- editor/gui/editor_file_dialog.cpp | 1 + editor/scene_tree_dock.cpp | 29 ++++++++++++++++------------- scene/2d/tile_map.cpp | 1 + scene/gui/file_dialog.cpp | 1 + scene/gui/item_list.cpp | 1 + scene/gui/menu_button.cpp | 1 + scene/gui/option_button.cpp | 1 + scene/gui/popup_menu.cpp | 1 + scene/gui/tab_bar.cpp | 1 + scene/property_list_helper.cpp | 26 ++++++++++++++++++++------ scene/property_list_helper.h | 7 +++++++ servers/audio/audio_stream.cpp | 1 + 12 files changed, 52 insertions(+), 19 deletions(-) diff --git a/editor/gui/editor_file_dialog.cpp b/editor/gui/editor_file_dialog.cpp index 7d26cb21fbc..08e75ee8120 100644 --- a/editor/gui/editor_file_dialog.cpp +++ b/editor/gui/editor_file_dialog.cpp @@ -1964,6 +1964,7 @@ void EditorFileDialog::_bind_methods() { Option defaults; base_property_helper.set_prefix("option_"); + base_property_helper.set_array_length_getter(&EditorFileDialog::get_option_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "name"), defaults.name, &EditorFileDialog::set_option_name, &EditorFileDialog::get_option_name); base_property_helper.register_property(PropertyInfo(Variant::PACKED_STRING_ARRAY, "values"), defaults.values, &EditorFileDialog::set_option_values, &EditorFileDialog::get_option_values); base_property_helper.register_property(PropertyInfo(Variant::INT, "default"), defaults.default_idx, &EditorFileDialog::set_option_default, &EditorFileDialog::get_option_default); diff --git a/editor/scene_tree_dock.cpp b/editor/scene_tree_dock.cpp index 3a1de937e26..6f0a8bc909d 100644 --- a/editor/scene_tree_dock.cpp +++ b/editor/scene_tree_dock.cpp @@ -2865,21 +2865,24 @@ void SceneTreeDock::replace_node(Node *p_node, Node *p_by_node) { void SceneTreeDock::_replace_node(Node *p_node, Node *p_by_node, bool p_keep_properties, bool p_remove_old) { ERR_FAIL_COND_MSG(!p_node->is_inside_tree(), "_replace_node() can't be called on a node outside of tree. You might have called it twice."); - Node *n = p_node; + Node *oldnode = p_node; Node *newnode = p_by_node; if (p_keep_properties) { - Node *default_oldnode = Object::cast_to(ClassDB::instantiate(n->get_class())); + Node *default_oldnode = Object::cast_to(ClassDB::instantiate(oldnode->get_class())); + List pinfo; - n->get_property_list(&pinfo); + oldnode->get_property_list(&pinfo); for (const PropertyInfo &E : pinfo) { if (!(E.usage & PROPERTY_USAGE_STORAGE)) { continue; } - if (default_oldnode->get(E.name) != n->get(E.name)) { - newnode->set(E.name, n->get(E.name)); + bool valid; + const Variant &default_val = default_oldnode->get(E.name, &valid); + if (!valid || default_val != oldnode->get(E.name)) { + newnode->set(E.name, oldnode->get(E.name)); } } @@ -2891,10 +2894,10 @@ void SceneTreeDock::_replace_node(Node *p_node, Node *p_by_node, bool p_keep_pro //reconnect signals List sl; - n->get_signal_list(&sl); + oldnode->get_signal_list(&sl); for (const MethodInfo &E : sl) { List cl; - n->get_signal_connection_list(E.name, &cl); + oldnode->get_signal_connection_list(E.name, &cl); for (const Object::Connection &c : cl) { if (!(c.flags & Object::CONNECT_PERSIST)) { @@ -2904,15 +2907,15 @@ void SceneTreeDock::_replace_node(Node *p_node, Node *p_by_node, bool p_keep_pro } } - String newname = n->get_name(); + String newname = oldnode->get_name(); List to_erase; - for (int i = 0; i < n->get_child_count(); i++) { - if (n->get_child(i)->get_owner() == nullptr && n->is_owned_by_parent()) { - to_erase.push_back(n->get_child(i)); + for (int i = 0; i < oldnode->get_child_count(); i++) { + if (oldnode->get_child(i)->get_owner() == nullptr && oldnode->is_owned_by_parent()) { + to_erase.push_back(oldnode->get_child(i)); } } - n->replace_by(newnode, true); + oldnode->replace_by(newnode, true); //small hack to make collisionshapes and other kind of nodes to work for (int i = 0; i < newnode->get_child_count(); i++) { @@ -2928,7 +2931,7 @@ void SceneTreeDock::_replace_node(Node *p_node, Node *p_by_node, bool p_keep_pro _push_item(newnode); if (p_remove_old) { - memdelete(n); + memdelete(oldnode); while (to_erase.front()) { memdelete(to_erase.front()->get()); diff --git a/scene/2d/tile_map.cpp b/scene/2d/tile_map.cpp index 165d4d5a675..fbe7563e6b8 100644 --- a/scene/2d/tile_map.cpp +++ b/scene/2d/tile_map.cpp @@ -1073,6 +1073,7 @@ TileMap::TileMap() { TileMapLayer *defaults = memnew(TileMapLayer); base_property_helper.set_prefix("layer_"); + base_property_helper.set_array_length_getter(&TileMap::get_layers_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "name"), defaults->get_name(), &TileMap::set_layer_name, &TileMap::get_layer_name); base_property_helper.register_property(PropertyInfo(Variant::BOOL, "enabled"), defaults->is_enabled(), &TileMap::set_layer_enabled, &TileMap::is_layer_enabled); base_property_helper.register_property(PropertyInfo(Variant::COLOR, "modulate"), defaults->get_modulate(), &TileMap::set_layer_modulate, &TileMap::get_layer_modulate); diff --git a/scene/gui/file_dialog.cpp b/scene/gui/file_dialog.cpp index 97a2917dc1f..91424c0ffda 100644 --- a/scene/gui/file_dialog.cpp +++ b/scene/gui/file_dialog.cpp @@ -1344,6 +1344,7 @@ void FileDialog::_bind_methods() { Option defaults; base_property_helper.set_prefix("option_"); + base_property_helper.set_array_length_getter(&FileDialog::get_option_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "name"), defaults.name, &FileDialog::set_option_name, &FileDialog::get_option_name); base_property_helper.register_property(PropertyInfo(Variant::PACKED_STRING_ARRAY, "values"), defaults.values, &FileDialog::set_option_values, &FileDialog::get_option_values); base_property_helper.register_property(PropertyInfo(Variant::INT, "default"), defaults.default_idx, &FileDialog::set_option_default, &FileDialog::get_option_default); diff --git a/scene/gui/item_list.cpp b/scene/gui/item_list.cpp index 8376ef48b67..1dd13f2ebf2 100644 --- a/scene/gui/item_list.cpp +++ b/scene/gui/item_list.cpp @@ -1882,6 +1882,7 @@ void ItemList::_bind_methods() { Item defaults(true); base_property_helper.set_prefix("item_"); + base_property_helper.set_array_length_getter(&ItemList::get_item_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "text"), defaults.text, &ItemList::set_item_text, &ItemList::get_item_text); base_property_helper.register_property(PropertyInfo(Variant::OBJECT, "icon", PROPERTY_HINT_RESOURCE_TYPE, "Texture2D"), defaults.icon, &ItemList::set_item_icon, &ItemList::get_item_icon); base_property_helper.register_property(PropertyInfo(Variant::BOOL, "selectable"), defaults.selectable, &ItemList::set_item_selectable, &ItemList::is_item_selectable); diff --git a/scene/gui/menu_button.cpp b/scene/gui/menu_button.cpp index e83d9c7c1bd..998f99b2f90 100644 --- a/scene/gui/menu_button.cpp +++ b/scene/gui/menu_button.cpp @@ -190,6 +190,7 @@ void MenuButton::_bind_methods() { PopupMenu::Item defaults(true); base_property_helper.set_prefix("popup/item_"); + base_property_helper.set_array_length_getter(&MenuButton::get_item_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "text"), defaults.text); base_property_helper.register_property(PropertyInfo(Variant::OBJECT, "icon", PROPERTY_HINT_RESOURCE_TYPE, "Texture2D"), defaults.icon); base_property_helper.register_property(PropertyInfo(Variant::INT, "checkable", PROPERTY_HINT_ENUM, "No,As Checkbox,As Radio Button"), defaults.checkable_type); diff --git a/scene/gui/option_button.cpp b/scene/gui/option_button.cpp index 509c6aca99a..0f161a014ae 100644 --- a/scene/gui/option_button.cpp +++ b/scene/gui/option_button.cpp @@ -571,6 +571,7 @@ void OptionButton::_bind_methods() { PopupMenu::Item defaults(true); base_property_helper.set_prefix("popup/item_"); + base_property_helper.set_array_length_getter(&OptionButton::get_item_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "text"), defaults.text, &OptionButton::_dummy_setter, &OptionButton::get_item_text); base_property_helper.register_property(PropertyInfo(Variant::OBJECT, "icon", PROPERTY_HINT_RESOURCE_TYPE, "Texture2D"), defaults.icon, &OptionButton::_dummy_setter, &OptionButton::get_item_icon); base_property_helper.register_property(PropertyInfo(Variant::INT, "id", PROPERTY_HINT_RANGE, "0,10,1,or_greater"), defaults.id, &OptionButton::_dummy_setter, &OptionButton::get_item_id); diff --git a/scene/gui/popup_menu.cpp b/scene/gui/popup_menu.cpp index 9b991972be8..5865fe141bb 100644 --- a/scene/gui/popup_menu.cpp +++ b/scene/gui/popup_menu.cpp @@ -2808,6 +2808,7 @@ void PopupMenu::_bind_methods() { Item defaults(true); base_property_helper.set_prefix("item_"); + base_property_helper.set_array_length_getter(&PopupMenu::get_item_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "text"), defaults.text, &PopupMenu::set_item_text, &PopupMenu::get_item_text); base_property_helper.register_property(PropertyInfo(Variant::OBJECT, "icon", PROPERTY_HINT_RESOURCE_TYPE, "Texture2D"), defaults.icon, &PopupMenu::set_item_icon, &PopupMenu::get_item_icon); base_property_helper.register_property(PropertyInfo(Variant::INT, "checkable", PROPERTY_HINT_ENUM, "No,As checkbox,As radio button"), defaults.checkable_type, &PopupMenu::_set_item_checkable_type, &PopupMenu::_get_item_checkable_type); diff --git a/scene/gui/tab_bar.cpp b/scene/gui/tab_bar.cpp index 0e130d60af7..8a04974479d 100644 --- a/scene/gui/tab_bar.cpp +++ b/scene/gui/tab_bar.cpp @@ -1868,6 +1868,7 @@ void TabBar::_bind_methods() { Tab defaults(true); base_property_helper.set_prefix("tab_"); + base_property_helper.set_array_length_getter(&TabBar::get_tab_count); base_property_helper.register_property(PropertyInfo(Variant::STRING, "title"), defaults.text, &TabBar::set_tab_title, &TabBar::get_tab_title); base_property_helper.register_property(PropertyInfo(Variant::STRING, "tooltip"), defaults.tooltip, &TabBar::set_tab_tooltip, &TabBar::get_tab_tooltip); base_property_helper.register_property(PropertyInfo(Variant::OBJECT, "icon", PROPERTY_HINT_RESOURCE_TYPE, "Texture2D"), defaults.icon, &TabBar::set_tab_icon, &TabBar::get_tab_icon); diff --git a/scene/property_list_helper.cpp b/scene/property_list_helper.cpp index b666e4c52d4..152ecaf89db 100644 --- a/scene/property_list_helper.cpp +++ b/scene/property_list_helper.cpp @@ -36,14 +36,17 @@ const PropertyListHelper::Property *PropertyListHelper::_get_property(const Stri return nullptr; } - { - const String index_string = components[0].trim_prefix(prefix); - if (!index_string.is_valid_int()) { - return nullptr; - } - *r_index = index_string.to_int(); + const String index_string = components[0].trim_prefix(prefix); + if (!index_string.is_valid_int()) { + return nullptr; } + int index = index_string.to_int(); + if (index < 0 || index >= _call_array_length_getter()) { + return nullptr; + } + + *r_index = index; return property_list.getptr(components[1]); } @@ -66,6 +69,11 @@ Variant PropertyListHelper::_call_getter(const Property *p_property, int p_index return p_property->getter->call(object, argptrs, 1, ce); } +int PropertyListHelper::_call_array_length_getter() const { + Callable::CallError ce; + return array_length_getter->call(object, nullptr, 0, ce); +} + void PropertyListHelper::set_prefix(const String &p_prefix) { prefix = p_prefix; } @@ -83,7 +91,13 @@ bool PropertyListHelper::is_initialized() const { } void PropertyListHelper::setup_for_instance(const PropertyListHelper &p_base, Object *p_object) { + DEV_ASSERT(!p_base.prefix.is_empty()); + DEV_ASSERT(p_base.array_length_getter != nullptr); + DEV_ASSERT(!p_base.property_list.is_empty()); + DEV_ASSERT(p_object != nullptr); + prefix = p_base.prefix; + array_length_getter = p_base.array_length_getter; property_list = p_base.property_list; object = p_object; } diff --git a/scene/property_list_helper.h b/scene/property_list_helper.h index eac6b03d474..e19e7cd22e1 100644 --- a/scene/property_list_helper.h +++ b/scene/property_list_helper.h @@ -43,15 +43,22 @@ class PropertyListHelper { }; String prefix; + MethodBind *array_length_getter = nullptr; HashMap property_list; Object *object = nullptr; const Property *_get_property(const String &p_property, int *r_index) const; void _call_setter(const MethodBind *p_setter, int p_index, const Variant &p_value) const; Variant _call_getter(const Property *p_property, int p_index) const; + int _call_array_length_getter() const; public: void set_prefix(const String &p_prefix); + template + void set_array_length_getter(G p_array_length_getter) { + array_length_getter = create_method_bind(p_array_length_getter); + } + // Register property without setter/getter. Only use when you don't need PropertyListHelper for _set/_get logic. void register_property(const PropertyInfo &p_info, const Variant &p_default); diff --git a/servers/audio/audio_stream.cpp b/servers/audio/audio_stream.cpp index 051ed596329..6966c243b51 100644 --- a/servers/audio/audio_stream.cpp +++ b/servers/audio/audio_stream.cpp @@ -711,6 +711,7 @@ void AudioStreamRandomizer::_bind_methods() { PoolEntry defaults; base_property_helper.set_prefix("stream_"); + base_property_helper.set_array_length_getter(&AudioStreamRandomizer::get_streams_count); base_property_helper.register_property(PropertyInfo(Variant::OBJECT, "stream", PROPERTY_HINT_RESOURCE_TYPE, "AudioStream"), defaults.stream, &AudioStreamRandomizer::set_stream, &AudioStreamRandomizer::get_stream); base_property_helper.register_property(PropertyInfo(Variant::FLOAT, "weight", PROPERTY_HINT_RANGE, "0,100,0.001,or_greater"), defaults.weight, &AudioStreamRandomizer::set_stream_probability_weight, &AudioStreamRandomizer::get_stream_probability_weight); }