Use-after-free in canvas batching

[Riteo]

Tested versions

• Reproducible in v4.4.dev.custom_build [74de05a], v4.4.dev.custom_build [a657ea4]
• Not reproducible in v4.4.dev.custom_build [14a7e0a]

System information

Godot v4.4.dev (74de05a) - 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 a657ea4

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:

godot/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp

Lines 2115 to 2119 in 74de05a

	if (ci->final_clip_owner != current_batch->clip) {
	current_batch = _new_batch(batch_broken);
	current_batch->clip = ci->final_clip_owner;
	current_clip = ci->final_clip_owner;
	}

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:

godot/servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp

Lines 2150 to 2167 in 74de05a

	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);
	} 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;
	Point2 pos = start_pos;
	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);
	pos.y += ci->repeat_size.y;
	} while (pos.y < end_pos.y);
	pos.x += ci->repeat_size.x;
	pos.y = start_pos.y;
	} while (pos.x < end_pos.x);
	}
	}

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

1. Create a new project
2. Craete a new 2D scene
3. Try dragging icon.svg into it from the 2D view
4. 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

[Riteo]

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);

[clayjohn]

Cc @stuartcarnie

[stuartcarnie]

Good find!

Passing it in as a reference makes sense; however, I'll update the record_item_commands function to use the argument r_current_batch. This will be safer, as if someone adds an early return in the future, the prologue may not get called, which updates r_current_batch.