Fix PropertyListHelper::_get_property returning a valid value even if…

… an index is outside the array valid indices Co-authored-by: Tomasz Chabora <[email protected]>
godotengine · May 10, 2024 · 7705265 · 7705265
1 parent c4279fe
commit 7705265
Show file tree

Hide file tree

Showing 12 changed files with 52 additions and 19 deletions.
diff --git 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
@@ -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<Node>(ClassDB::instantiate(n->get_class()));
+		Node *default_oldnode = Object::cast_to<Node>(ClassDB::instantiate(oldnode->get_class()));
+
 		List<PropertyInfo> 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<MethodInfo> sl;
 
-	n->get_signal_list(&sl);
+	oldnode->get_signal_list(&sl);
 	for (const MethodInfo &E : sl) {
 		List<Object::Connection> 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<Node *> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -43,15 +43,22 @@ class PropertyListHelper {
 	};
 
 	String prefix;
+	MethodBind *array_length_getter = nullptr;
 	HashMap<String, Property> 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 <typename G>
+	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
@@ -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);
 }