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

Check if the line pointer goes away from the image buffer's EOF in the BMP importer #46555

Merged
merged 1 commit into from
Oct 3, 2021

Conversation

gongpha
Copy link
Contributor

@gongpha gongpha commented Mar 1, 2021

Fixes #46542.

I wish there is a better way to handle this.

@gongpha gongpha requested a review from a team as a code owner March 1, 2021 13:49
@Calinou Calinou added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release crash topic:import labels Mar 1, 2021
@Calinou Calinou added this to the 4.0 milestone Mar 1, 2021
@gongpha gongpha force-pushed the line-ptr-more-than-size-bmp branch from b68c10a to 47d56dc Compare March 1, 2021 18:01
@gongpha gongpha requested a review from fire March 1, 2021 18:05
@@ -91,11 +91,13 @@ Error ImageLoaderBMP::convert_to_image(Ref<Image> p_image,
// the data width in case of 8/4/1 bit images
const uint32_t w = bits_per_pixel >= 24 ? width : width_bytes;
const uint8_t *line = p_buffer + (line_width * (height - 1));
const uint8_t *end_buffer = p_buffer + p_header.bmp_info_header.bmp_size_image;
Copy link
Member

Choose a reason for hiding this comment

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

bmp_size_image do we trust this number? How is this calculated? I would expect the buffer size to be calculable from the size of the bmp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that buffer size is calculated from bmp_file_size - bmp_file_offset. However, if two variables are corrupt but the buffer had allocated as calculated. I think it may be safe to trust this calculation.

@gongpha gongpha force-pushed the line-ptr-more-than-size-bmp branch from 47d56dc to ac5d7ab Compare March 2, 2021 02:51
@gongpha gongpha requested a review from fire March 9, 2021 08:06
Copy link
Contributor

@theraot theraot left a comment

Choose a reason for hiding this comment

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

  • I tested that the bug still works on master, and also that this fixes it.
  • Yep, that would be how you compute the buffer size, as evidenced here:
    uint32_t bmp_buffer_size = (bmp_header.bmp_file_header.bmp_file_size -
    bmp_header.bmp_file_header.bmp_file_offset);

Would this change allow Godot to catch every possible case of a corrupted image? No, probably not. But it will prevent crashes!

I would argue the class would benefit from a refactor, but that is not here nor there.

@akien-mga akien-mga merged commit 66ab3ce into godotengine:master Oct 3, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 4, 2021
@gongpha gongpha deleted the line-ptr-more-than-size-bmp branch October 6, 2021 16:33
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.

Crash when importing broken bmp file
5 participants