Division by zero in rasterizer_canvas_gles3.cpp

[qarmin]

Godot version: 3.2 beta 1 OS/device including version: Ubuntu 19.10 Issue description:

drivers/gles3/rasterizer_canvas_gles3.cpp:998:52: runtime error: division by zero
drivers/gles3/rasterizer_canvas_gles3.cpp:998:30: runtime error: division by zero
drivers/gles3/rasterizer_canvas_gles3.cpp:824:52: runtime error: division by zero
drivers/gles3/rasterizer_canvas_gles3.cpp:824:30: runtime error: division by zero

One of possible solutions

diff --git a/drivers/gles3/rasterizer_canvas_gles3.cpp b/drivers/gles3/rasterizer_canvas_gles3.cpp
index edffe852a2..f9acb184d3 100644
--- a/drivers/gles3/rasterizer_canvas_gles3.cpp
+++ b/drivers/gles3/rasterizer_canvas_gles3.cpp
@@ -820,7 +820,7 @@ void RasterizerCanvasGLES3::_canvas_item_render_commands(Item *p_item, Item *cur

                                RasterizerStorageGLES3::Texture *texture = _bind_canvas_texture(mesh->texture, mesh->normal_map);

-                               if (texture) {
+                               if (texture && texture->width != 0 && texture->height != 0) {
                                        Size2 texpixel_size(1.0 / texture->width, 1.0 / texture->height);
                                        state.canvas_shader.set_uniform(CanvasShaderGLES3::COLOR_TEXPIXEL_SIZE, texpixel_size);
                                }
@@ -871,7 +871,7 @@ void RasterizerCanvasGLES3::_canvas_item_render_commands(Item *p_item, Item *cur
                                state.using_texture_rect = true;
                                _set_texture_rect_mode(false);

-                               if (texture) {
+                               if (texture && texture->width != 0 && texture->height != 0) {
                                        Size2 texpixel_size(1.0 / texture->width, 1.0 / texture->height);
                                        state.canvas_shader.set_uniform(CanvasShaderGLES3::COLOR_TEXPIXEL_SIZE, texpixel_size);
                                }
@@ -994,7 +994,7 @@ void RasterizerCanvasGLES3::_canvas_item_render_commands(Item *p_item, Item *cur

                                RasterizerStorageGLES3::Texture *texture = _bind_canvas_texture(particles_cmd->texture, particles_cmd->normal_map);

-                               if (texture) {
+                               if (texture && texture->width != 0 && texture->height != 0) {
                                        Size2 texpixel_size(1.0 / texture->width, 1.0 / texture->height);
                                        state.canvas_shader.set_uniform(CanvasShaderGLES3::COLOR_TEXPIXEL_SIZE, texpixel_size);
                                } else {

Steps to reproduce: To get this error try to compile Godot with ubsan sanitizer(GCC or LLVM) or compile Godot from this branch(find more errors but tgis needs LLVM 9 or GCC 9) - https://github.com/qarmin/godot/tree/bb

1. Read Project README - https://github.com/qarmin/The-worst-Godot-test-project#the-worst-godot-test-project
2. Run Project

Minimal reproduction project: https://github.com/qarmin/The-worst-Godot-test-project/archive/60944c7930279db20747b8d1835e1687188a10ab.zip

[KoBeWi]

This is very likely fixed in master. Not sure if it's worth keeping it open, as the issue doesn't look critical.

[akien-mga]

In master it's fixed by removing the GLES3 backend for now :)

But I think it would still be relevant to fix in 3.2, though I'm not sure that adding those checks here wouldn't impact performance. CC @clayjohn @lawnjelly

[lawnjelly]

These divisions are from the legacy renderer, and from a quick look may not be used in the batching renderer. I had noticed them, I'm presuming because they don't occur in practice there must be earlier checks that preclude the possibility of textures with zero size.

For instance, the form is as follows:

if (texture) {
    Size2 texpixel_size(1.0 / texture->width, 1.0 / texture->height);
    state.canvas_shader.set_uniform(CanvasShaderGLES2::COLOR_TEXPIXEL_SIZE, texpixel_size);
}

So perhaps texture is never set with zero size textures.

Having never had a report of this happen in practice (this has been in the renderer for years), personally I'd be inclined to leave as is and keep an eye, unless these warnings need to be disabled, in which case pragmas might be more suited. The checks will have some small slow down for the legacy renderer, and these changes do look as though they would literally stop a warning and nothing more.