Skip to content

[3.x] Sanitize input to ImageTexture::set_data() to prevent bad memory access#54935

Open
nobuyukinyuu wants to merge 1 commit intogodotengine:3.xfrom
nobuyukinyuu:imagetexture.set-data-bounds-check
Open

[3.x] Sanitize input to ImageTexture::set_data() to prevent bad memory access#54935
nobuyukinyuu wants to merge 1 commit intogodotengine:3.xfrom
nobuyukinyuu:imagetexture.set-data-bounds-check

Conversation

@nobuyukinyuu
Copy link
Copy Markdown
Contributor

@nobuyukinyuu nobuyukinyuu commented Nov 13, 2021

Fixes #54930. As part of the refactoring of texture.cpp, the equivalent checks should already be in the corresponding method in master.

@nobuyukinyuu nobuyukinyuu force-pushed the imagetexture.set-data-bounds-check branch from 4b9d427 to 9ede3c2 Compare November 13, 2021 03:18
@YeldhamDev YeldhamDev added this to the 3.5 milestone Nov 13, 2021
@YeldhamDev YeldhamDev requested a review from a team November 13, 2021 03:50
@nobuyukinyuu
Copy link
Copy Markdown
Contributor Author

Flags have changed to separate fields in 4.0; the state of the mipmaps field seems to be different to flags & FLAG_MIPMAPS at the time get_data() is called by the engine in many cases which I'm not clear on. To check mipmaps I pulled from get_data() but the sanitizer build doesn't like this because of the potential null pointer access. I can add a check for image_stored before doing a mipmap check, but I don't know if having the branch statement in this method is undesirable. In any case, I'll try and see if this satisfies the sanitizer build check.

@nobuyukinyuu nobuyukinyuu force-pushed the imagetexture.set-data-bounds-check branch from 9ede3c2 to 9aea089 Compare November 13, 2021 05:34
@Macksaur
Copy link
Copy Markdown
Contributor

Does this change work with VisualServer.texture_set_shrink_all_x2_on_set_data? and also with GLES2 having to resize_to_po2 to support older hardware?

@nobuyukinyuu
Copy link
Copy Markdown
Contributor Author

nobuyukinyuu commented Nov 14, 2021

@Macksaur After testing, I notice that setting VisualServer.texture_set_shrink_all_x2_on_set_data(true) will cause image data of the original to report the original dimensions, but duplicating the original will pull from the shrunk texture once bound to the texture. Chaining duplicates from the bound texture will do the same thing and half the resolution. Considering the nature of the set_data() method, it's probably sill up to you to maintain awareness of the state of the VisualServer texture shrink setting when writing your bind method. After performing resize() and create_from_image() with the shrink setting enabled, the actual raw image size and what set_data() expects will no longer match. This PR should, however, still catch the discrepancy and prevent OOB memory access if you were to naively attempt , like so:
image

I don't know if this discrepancy would be considered another bug, but I feel like it is outside the scope of this PR, as it's not an issue introduced by these changes. It may require a separate bug report, testing on the 4.0 branch, etc. Hope that helps.

@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 3, 2022
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.

4 participants