Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add protection in RichTextLabel.update_image to prevent crash #84833

Conversation

jsjtxietian
Copy link
Contributor

Fixes #84739

@jsjtxietian jsjtxietian requested a review from a team as a code owner November 13, 2023 08:20
@akien-mga akien-mga added this to the 4.2 milestone Nov 13, 2023
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While widht and height should not be negative, and this change is fine, I'm not sure why it would cause a crash.

Relevant line: ret.width = p_image->get_width() * p_height / p_image->get_height();.

I would suspect crash reason is missing ERR_FAIL_COND(p_image.is_null()); check in the update_image. It's done only if UPDATE_TEXTURE flag is set, but _get_image_size which is reading the image is also called if UPDATE_SIZE or UPDATE_REGION is set.

diff --git a/scene/gui/rich_text_label.cpp b/scene/gui/rich_text_label.cpp
index 30a468dfc5..e8adce897a 100644
--- a/scene/gui/rich_text_label.cpp
+++ b/scene/gui/rich_text_label.cpp
@@ -3291,6 +3291,9 @@ void RichTextLabel::update_image(const Variant &p_key, BitField<ImageUpdateMask>
 					}
 				}
 				if ((p_mask & UPDATE_SIZE) || (p_mask & UPDATE_REGION) || (p_mask & UPDATE_TEXTURE)) {
+					ERR_FAIL_COND(item->image.is_null());
+					ERR_FAIL_COND(item->image->get_width() == 0);
+					ERR_FAIL_COND(item->image->get_height() == 0);
 					Size2 new_size = _get_image_size(item->image, item->rq_size.width, item->rq_size.height, item->region);
 					if (item->size != new_size) {
 						reshape = true;

@jsjtxietian jsjtxietian force-pushed the add-protection-in-rich-text-label-add-image branch from 9255863 to 306a8ee Compare November 13, 2023 10:02
@akien-mga akien-mga changed the title Add protection in RichTextLabel.update_image to prevent crash Add protection in RichTextLabel.update_image to prevent crash Nov 13, 2023
@akien-mga akien-mga merged commit 5945768 into godotengine:master Nov 13, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the add-protection-in-rich-text-label-add-image branch November 13, 2023 11:25
sandygk added a commit to sandygk/godot that referenced this pull request Nov 17, 2023
…in-rich-text-label-add-image

Add protection in `RichTextLabel.update_image` to prevent crash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executing RichTextLabel.update_image function crashes Godot
3 participants