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

Prevent crash on conversion of invalid data in Image #84782

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

rsburke4
Copy link
Contributor

@rsburke4 rsburke4 commented Nov 12, 2023

Added error to address #84750

Bugsquad edit:

@rsburke4 rsburke4 requested a review from a team as a code owner November 12, 2023 03:22
@Chaosus Chaosus added this to the 4.2 milestone Nov 12, 2023
@AThousandShips
Copy link
Member

AThousandShips commented Nov 12, 2023

Thank you for your contribution

This wouldn't be sufficient, it won't always catch the negative case as the check above is checking for positive values

I'd suggest (as I did in the issue report) to check the type, look in _initialize_data, add this at the top of the convert function instead: ERR_FAIL_INDEX_MSG(p_new_format, FORMAT_MAX, "The Image format specified (" + itos(p_new_format) + ") is out of range. See Image's Format enum.");

After the check for p_new_format == format (line 519)

Edit: note also that the check added is in reverse, it requires that the data be empty, the error macros checks if the condition is true and if it is the error is thrown

@AThousandShips
Copy link
Member

I'd also suggest adding a test for this invalid use to the unit tests, in file tests/core/io /test_image.h

@akien-mga akien-mga modified the milestones: 4.2, 4.x Nov 12, 2023
@rsburke4 rsburke4 requested a review from a team as a code owner November 12, 2023 19:45
@rsburke4 rsburke4 force-pushed the image-fix branch 2 times, most recently from 6643b4f to 413d29c Compare November 12, 2023 19:48
@rsburke4
Copy link
Contributor Author

Thank you for your help and clarification. I think I understand a little better now. Would this suffice for a test case? It seems to catch problems and print a helpful errors when it fails.

@AThousandShips
Copy link
Member

These tests are good but doesn't cover that it safely handles invalid ones

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

I don't have an ASAN build to confirm this but the tests pass and the check works otherwise, preventing some spurious errors

Can someone with an ASAN build confirm in addition to the unit tests?

Just some minor style fixes

tests/core/io/test_image.h Outdated Show resolved Hide resolved
tests/core/io/test_image.h Outdated Show resolved Hide resolved
@AThousandShips AThousandShips modified the milestones: 4.x, 4.3 Nov 13, 2023
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 13, 2023
@YuriSizov
Copy link
Contributor

@rsburke4 Are you around to address the latest style suggestions? 🙃

@rsburke4
Copy link
Contributor Author

rsburke4 commented Dec 4, 2023

Yes, I will make the adjustment today. Thank you.

@rsburke4 rsburke4 force-pushed the image-fix branch 2 times, most recently from 2506597 to ddc8443 Compare December 4, 2023 16:36
@YuriSizov YuriSizov changed the title Added error to catch conversion on invalid data in Image Prevent crash on conversion of invalid data in Image Dec 8, 2023
core/io/image.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit b952b00 into godotengine:master Dec 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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 Image.convert function crashes Godot
5 participants