From 2d56f4ad162548cf776cfc6506f0295109d4f3c3 Mon Sep 17 00:00:00 2001 From: Falk David Date: Tue, 23 Jul 2024 13:38:20 +0200 Subject: [PATCH 1/2] Fix #124995: Crashes in rendering code due to invalid material indices The issue was that some `material_index` were `-1`. This meant that the rendering code was accessing out-of-bounds memory to read from the material pool. The fix adds an assert to the `gpencil_material_resources_get` and clamps the `material_index` to >= 0 to ensure that the material is valid. It also changes the default read value for the material indices in `create_curves_outline` and `retrieve_visible_strokes` to be 0 instead of -1. --- source/blender/draw/engines/gpencil/gpencil_draw_data.cc | 1 + source/blender/draw/engines/gpencil/gpencil_engine_c.cc | 5 ++++- .../editors/grease_pencil/intern/grease_pencil_geom.cc | 2 +- .../editors/grease_pencil/intern/grease_pencil_utils.cc | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/source/blender/draw/engines/gpencil/gpencil_draw_data.cc b/source/blender/draw/engines/gpencil/gpencil_draw_data.cc index 97e80c65bdb..ae3798df3eb 100644 --- a/source/blender/draw/engines/gpencil/gpencil_draw_data.cc +++ b/source/blender/draw/engines/gpencil/gpencil_draw_data.cc @@ -304,6 +304,7 @@ void gpencil_material_resources_get(GPENCIL_MaterialPool *first_pool, GPUUniformBuf **r_ubo_mat) { GPENCIL_MaterialPool *matpool = first_pool; + BLI_assert(mat_id >= 0); int pool_id = mat_id / GPENCIL_MATERIAL_BUFFER_LEN; for (int i = 0; i < pool_id; i++) { matpool = matpool->next; diff --git a/source/blender/draw/engines/gpencil/gpencil_engine_c.cc b/source/blender/draw/engines/gpencil/gpencil_engine_c.cc index c34fa5346c6..f07441d3093 100644 --- a/source/blender/draw/engines/gpencil/gpencil_engine_c.cc +++ b/source/blender/draw/engines/gpencil/gpencil_engine_c.cc @@ -790,7 +790,10 @@ static GPENCIL_tObject *grease_pencil_object_cache_populate(GPENCIL_PrivateData visible_strokes.foreach_index([&](const int stroke_i, const int pos) { const IndexRange points = points_by_curve[stroke_i]; - const int material_index = stroke_materials[stroke_i]; + /* The material index shouldn't be negative. We clamp it here to avoid crashing in the + * rendering code. Any stroke with a material < 0 will use the first material in the first + * material slot.*/ + const int material_index = std::max(stroke_materials[stroke_i], 0); const MaterialGPencilStyle *gp_style = BKE_gpencil_material_settings(ob, material_index + 1); const bool hide_material = (gp_style->flag & GP_MATERIAL_HIDE) != 0; diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_geom.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_geom.cc index 3063cb58eb3..2cb9c669bfc 100644 --- a/source/blender/editors/grease_pencil/intern/grease_pencil_geom.cc +++ b/source/blender/editors/grease_pencil/intern/grease_pencil_geom.cc @@ -697,7 +697,7 @@ bke::CurvesGeometry create_curves_outline(const bke::greasepencil::Drawing &draw const VArray src_end_caps = *src_attributes.lookup_or_default( "end_cap", bke::AttrDomain::Curve, GP_STROKE_CAP_ROUND); const VArray src_material_index = *src_attributes.lookup_or_default( - "material_index", bke::AttrDomain::Curve, -1); + "material_index", bke::AttrDomain::Curve, 0); /* Transform positions. */ Array transformed_positions(src_positions.size()); diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc index 0f140c4f6c7..48a62129174 100644 --- a/source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc +++ b/source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc @@ -888,7 +888,7 @@ IndexMask retrieve_visible_strokes(Object &object, /* Get all the strokes that have their material visible. */ const VArray materials = *attributes.lookup_or_default( - "material_index", bke::AttrDomain::Curve, -1); + "material_index", bke::AttrDomain::Curve, 0); return IndexMask::from_predicate( curves_range, GrainSize(4096), memory, [&](const int64_t curve_i) { const int material_index = materials[curve_i]; -- 2.30.2 From bad12765f1d3077bcefd6f01f9ff117a23677c94 Mon Sep 17 00:00:00 2001 From: Falk David Date: Tue, 23 Jul 2024 13:50:54 +0200 Subject: [PATCH 2/2] Update comment --- source/blender/draw/engines/gpencil/gpencil_engine_c.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/blender/draw/engines/gpencil/gpencil_engine_c.cc b/source/blender/draw/engines/gpencil/gpencil_engine_c.cc index f07441d3093..84f79e5a544 100644 --- a/source/blender/draw/engines/gpencil/gpencil_engine_c.cc +++ b/source/blender/draw/engines/gpencil/gpencil_engine_c.cc @@ -790,9 +790,9 @@ static GPENCIL_tObject *grease_pencil_object_cache_populate(GPENCIL_PrivateData visible_strokes.foreach_index([&](const int stroke_i, const int pos) { const IndexRange points = points_by_curve[stroke_i]; - /* The material index shouldn't be negative. We clamp it here to avoid crashing in the - * rendering code. Any stroke with a material < 0 will use the first material in the first - * material slot.*/ + /* The material index is allowed to be negative as it's stored as a generic attribure. We + * clamp it here to avoid crashing in the rendering code. Any stroke with a material < 0 will + * use the first material in the first material slot.*/ const int material_index = std::max(stroke_materials[stroke_i], 0); const MaterialGPencilStyle *gp_style = BKE_gpencil_material_settings(ob, material_index + 1); -- 2.30.2