Use-after-free in canvas batching

Tested versions

Reproducible in v4.4.dev.custom_build [74de05a01], v4.4.dev.custom_build [a657ea42f]
Not reproducible in v4.4.dev.custom_build [14a7e0abb]

System information

Godot v4.4.dev (74de05a01) - KISS Linux Community #17 SMP PREEMPT_DYNAMIC Sat Aug 24 18:13:54 CEST 2024 - Wayland - Vulkan (Forward+) - integrated AMD Radeon Vega 8 Graphics (RADV RAVEN) - AMD Ryzen 5 3500U with Radeon Vega Mobile Gfx (8 Threads)

Issue description

Recently I pulled master and got a consistent crash I couldn't explain. Further bisecting and debugging brought me to #92797, commit a657ea42f1e656695f502a0e5f50ea9e0e041c3e

Valgrind uncovered that the issue was an use-after-free in _render_batch_items. Why nobody is replicating it except me is a bit weird, but that might be because I'm running a musl distro.

I'm getting an invalid read here, in the condition, when dereferencing current_batch:

https://github.com/godotengine/godot/blob/74de05a01c8716a42d4e3427f607d7bea76b35e5/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp#L2115-L2119

The problematic call trace looks something like this: _render_batch_items -> _record_item_commands -> _new_batch

current_batch points into a LocalVector, which can be updated by _new_batch, triggering a reallocation.

As you can see in the code above (and in the rest of the codebase AFAICT), current_batch gets correctly updated in case of an update, but not when calling _record_item_commands:

https://github.com/godotengine/godot/blob/74de05a01c8716a42d4e3427f607d7bea76b35e5/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp#L2150-L2167

I suppose that a potential fix might be to return the new current_batch just like in add_to_batch.

This is the current signature of add_to_batch for reference:

void RendererCanvasRenderRD::_add_to_batch(uint32_t &r_index, bool &r_batch_broken, Batch *&r_current_batch) {

Notice how it returns the new current_batch pointer.

I don't have the confidence to file a PR as I don't have any experience with this part of the codebase so I have no idea if this might have some unforseen consequences.

Steps to reproduce

Create a new project
Craete a new 2D scene
Try dragging icon.svg into it from the 2D view
In my case it crashes, but running with valgrind should cause the same use-after-free to be reported:

==17362== Invalid read of size 8
==17362==    at 0xAD66272: RendererCanvasRenderRD::_render_batch_items(RendererCanvasRenderRD::RenderTarget, int, Transform2D const&, RendererCanvasRender::Light*, bool&, bool, RenderingMethod::RenderInfo*) (renderer_canvas_render_rd.cpp:2115)
==17362==    by 0xAD5DD8F: RendererCanvasRenderRD::canvas_render_items(RID, RendererCanvasRender::Item*, Color const&, RendererCanvasRender::Light*, RendererCanvasRender::Light*, Transform2D const&, RenderingServer::CanvasItemTextureFilter, RenderingServer::CanvasItemTextureRepeat, bool, bool&, RenderingMethod::RenderInfo*) (renderer_canvas_render_rd.cpp:849)
==17362==    by 0xAB093E0: RendererCanvasCull::_render_canvas_item_tree(RID, RendererCanvasCull::Canvas::ChildItem*, int, Transform2D const&, Rect2 const&, Color const&, RendererCanvasRender::Light*, RendererCanvasRender::Light*, RenderingServer::CanvasItemTextureFilter, RenderingServer::CanvasItemTextureRepeat, bool, unsigned int, RenderingMethod::RenderInfo*) (renderer_canvas_cull.cpp:76)
==17362==    by 0xAB0AD93: RendererCanvasCull::render_canvas(RID, RendererCanvasCull::Canvas*, Transform2D const&, RendererCanvasRender::Light*, RendererCanvasRender::Light*, Rect2 const&, RenderingServer::CanvasItemTextureFilter, RenderingServer::CanvasItemTextureRepeat, bool, bool, unsigned int, RenderingMethod::RenderInfo*) (renderer_canvas_cull.cpp:430)
==17362==    by 0xAB3407E: RendererViewport::_draw_viewport(RendererViewport::Viewport*) (renderer_viewport.cpp:616)
==17362==    by 0xAB34F32: RendererViewport::draw_viewports(bool) (renderer_viewport.cpp:808)
==17362==    by 0xAB9E9FF: RenderingServerDefault::_draw(bool, double) (rendering_server_default.cpp:87)
==17362==    by 0xABA04D9: RenderingServerDefault::draw(bool, double) (rendering_server_default.cpp:405)
==17362==    by 0x639F4AF: Main::iteration() (main.cpp:4361)
==17362==    by 0x62CF049: OS_LinuxBSD::run() (os_linuxbsd.cpp:962)
==17362==    by 0x62C94CB: main (godot_linuxbsd.cpp:85)
==17362==  Address 0xf60c958 is 14,552 bytes inside a block of size 32,784 free'd
==17362==    at 0xCD8EEAF: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==17362==    by 0xB4A2A9F: Memory::realloc_static(void*, unsigned long, bool) (memory.cpp:162)
==17362==    by 0xAD891C8: LocalVector<RendererCanvasRenderRD::Batch, unsigned int, false, false>::push_back(RendererCanvasRenderRD::Batch) (local_vector.h:63)
==17362==    by 0xAD6B311: RendererCanvasRenderRD::_new_batch(bool&) (renderer_canvas_render_rd.cpp:3008)
==17362==    by 0xAD675A1: RendererCanvasRenderRD::_record_item_commands(RendererCanvasRender::Item const*, RendererCanvasRenderRD::RenderTarget, Transform2D const&, RendererCanvasRender::Item*&, RendererCanvasRender::Light*, unsigned int&, bool&, bool&) (renderer_canvas_render_rd.cpp:2378)
==17362==    by 0xAD664F9: RendererCanvasRenderRD::_render_batch_items(RendererCanvasRenderRD::RenderTarget, int, Transform2D const&, RendererCanvasRender::Light*, bool&, bool, RenderingMethod::RenderInfo*) (renderer_canvas_render_rd.cpp:2151)
==17362==    by 0xAD5DD8F: RendererCanvasRenderRD::canvas_render_items(RID, RendererCanvasRender::Item*, Color const&, RendererCanvasRender::Light*, RendererCanvasRender::Light*, Transform2D const&, RenderingServer::CanvasItemTextureFilter, RenderingServer::CanvasItemTextureRepeat, bool, bool&, RenderingMethod::RenderInfo*) (renderer_canvas_render_rd.cpp:849)
==17362==    by 0xAB093E0: RendererCanvasCull::_render_canvas_item_tree(RID, RendererCanvasCull::Canvas::ChildItem*, int, Transform2D const&, Rect2 const&, Color const&, RendererCanvasRender::Light*, RendererCanvasRender::Light*, RenderingServer::CanvasItemTextureFilter, RenderingServer::CanvasItemTextureRepeat, bool, unsigned int, RenderingMethod::RenderInfo*) (renderer_canvas_cull.cpp:76)
==17362==    by 0xAB0AD93: RendererCanvasCull::render_canvas(RID, RendererCanvasCull::Canvas*, Transform2D const&, RendererCanvasRender::Light*, RendererCanvasRender::Light*, Rect2 const&, RenderingServer::CanvasItemTextureFilter, RenderingServer::CanvasItemTextureRepeat, bool, bool, unsigned int, RenderingMethod::RenderInfo*) (renderer_canvas_cull.cpp:430)
==17362==    by 0xAB3407E: RendererViewport::_draw_viewport(RendererViewport::Viewport*) (renderer_viewport.cpp:616)
==17362==    by 0xAB34F32: RendererViewport::draw_viewports(bool) (renderer_viewport.cpp:808)
==17362==    by 0xAB9E9FF: RenderingServerDefault::_draw(bool, double) (rendering_server_default.cpp:87)
==17362==  Block was alloc'd at
==17362==    at 0xCD89874: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==17362==    by 0xB4A259C: Memory::alloc_static(unsigned long, bool) (memory.cpp:108)
==17362==    by 0xB4A283F: Memory::realloc_static(void*, unsigned long, bool) (memory.cpp:132)
==17362==    by 0xAD88C8B: LocalVector<RendererCanvasRenderRD::Batch, unsigned int, false, false>::reserve(unsigned int) (local_vector.h:144)
==17362==    by 0xAD65C65: RendererCanvasRenderRD::RendererCanvasRenderRD() (renderer_canvas_render_rd.cpp:2040)
==17362==    by 0xAD6DEA2: RendererCompositorRD::RendererCompositorRD() (renderer_compositor_rd.cpp:312)
==17362==    by 0x6308FF4: RendererCompositorRD::_create_current() (renderer_compositor_rd.h:138)
==17362==    by 0xAB17494: RendererCompositor::create() (renderer_compositor.cpp:42)
==17362==    by 0xAB9F90E: RenderingServerDefault::_init() (rendering_server_default.cpp:218)
==17362==    by 0xAB9FD6A: RenderingServerDefault::init() (rendering_server_default.cpp:257)
==17362==    by 0x63954EA: Main::setup2(bool) (main.cpp:3061)
==17362==    by 0x6392A1A: Main::setup(char const*, int, char**, bool) (main.cpp:2597)

Minimal reproduction project (MRP)

An empty project :P

I suppose that a potential fix might be to return the new current_batch just like in add_to_batch.

Update, the following patch seems indeed to fix the crash on my end but as above I have no idea on whether this is a proper fix.

diff --git a/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp b/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp
index b0851efe5f..6022e48aaf 100644
--- a/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp
+++ b/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp
@@ -2148,7 +2148,7 @@ void RendererCanvasRenderRD::_render_batch_items(RenderTarget p_to_render_target

            Transform2D base_transform = p_canvas_transform_inverse * ci->final_transform;
            if (!ci->repeat_size.x && !ci->repeat_size.y) {
-               _record_item_commands(ci, p_to_render_target, base_transform, current_clip, p_lights, instance_index, batch_broken, r_sdf_used);
+               _record_item_commands(ci, p_to_render_target, base_transform, current_clip, p_lights, instance_index, batch_broken, r_sdf_used, current_batch);
            } else {
                Point2 start_pos = ci->repeat_size * -(ci->repeat_times / 2);
                Point2 end_pos = ci->repeat_size * ci->repeat_times + ci->repeat_size + start_pos;
@@ -2156,7 +2156,7 @@ void RendererCanvasRenderRD::_render_batch_items(RenderTarget p_to_render_target
                do {
                    do {
                        Transform2D transform = base_transform * Transform2D(0, pos / ci->xform_curr.get_scale());
-                       _record_item_commands(ci, p_to_render_target, transform, current_clip, p_lights, instance_index, batch_broken, r_sdf_used);
+                       _record_item_commands(ci, p_to_render_target, transform, current_clip, p_lights, instance_index, batch_broken, r_sdf_used, current_batch);
                        pos.y += ci->repeat_size.y;
                    } while (pos.y < end_pos.y);

@@ -2262,7 +2262,7 @@ void RendererCanvasRenderRD::_render_batch_items(RenderTarget p_to_render_target
    state.last_instance_index += instance_index;
 }

-void RendererCanvasRenderRD::_record_item_commands(const Item *p_item, RenderTarget p_render_target, const Transform2D &p_base_transform, Item *&r_current_clip, Light *p_lights, uint32_t &r_index, bool &r_batch_broken, bool &r_sdf_used) {
+void RendererCanvasRenderRD::_record_item_commands(const Item *p_item, RenderTarget p_render_target, const Transform2D &p_base_transform, Item *&r_current_clip, Light *p_lights, uint32_t &r_index, bool &r_batch_broken, bool &r_sdf_used, Batch *&r_current_batch) {
    Batch *current_batch = &state.canvas_instance_batches[state.current_batch_index];

    RenderingServer::CanvasItemTextureFilter texture_filter = p_item->texture_filter == RS::CANVAS_ITEM_TEXTURE_FILTER_DEFAULT ? default_filter : p_item->texture_filter;
@@ -2784,6 +2784,8 @@ void RendererCanvasRenderRD::_record_item_commands(const Item *p_item, RenderTar
        // will make it re-enable clipping if needed afterwards
        r_current_clip = nullptr;
    }
+
+   r_current_batch = current_batch;
 }

 void RendererCanvasRenderRD::_render_batch(RD::DrawListID p_draw_list, PipelineVariants *p_pipeline_variants, RenderingDevice::FramebufferFormatID p_framebuffer_format, Light *p_lights, Batch const *p_batch, RenderingMethod::RenderInfo *r_render_info) {
diff --git a/servers/rendering/renderer_rd/renderer_canvas_render_rd.h b/servers/rendering/renderer_rd/renderer_canvas_render_rd.h
index 87de07464e..8d90cd23ce 100644
--- a/servers/rendering/renderer_rd/renderer_canvas_render_rd.h
+++ b/servers/rendering/renderer_rd/renderer_canvas_render_rd.h
@@ -559,7 +559,7 @@ class RendererCanvasRenderRD : public RendererCanvasRender {
    };

    void _render_batch_items(RenderTarget p_to_render_target, int p_item_count, const Transform2D &p_canvas_transform_inverse, Light *p_lights, bool &r_sdf_used, bool p_to_backbuffer = false, RenderingMethod::RenderInfo *r_render_info = nullptr);
-   void _record_item_commands(const Item *p_item, RenderTarget p_render_target, const Transform2D &p_base_transform, Item *&r_current_clip, Light *p_lights, uint32_t &r_index, bool &r_batch_broken, bool &r_sdf_used);
+   void _record_item_commands(const Item *p_item, RenderTarget p_render_target, const Transform2D &p_base_transform, Item *&r_current_clip, Light *p_lights, uint32_t &r_index, bool &r_batch_broken, bool &r_sdf_used, Batch *&r_current_batch);
    void _render_batch(RD::DrawListID p_draw_list, PipelineVariants *p_pipeline_variants, RenderingDevice::FramebufferFormatID p_framebuffer_format, Light *p_lights, Batch const *p_batch, RenderingMethod::RenderInfo *r_render_info = nullptr);
    void _prepare_batch_texture(Batch *p_current_batch, RID p_texture) const;
    void _bind_canvas_texture(RD::DrawListID p_draw_list, RID p_uniform_set);

godotengine / godot