Fix #160029: Crash adding UV layer after converting attributes

Regression in [0] meant converting the active UV map to a vertex
group then adding a new UV map caused an out-of-bounds read
in AttributeStorage::lookup.

Vertex groups share the attribute namespace,
so an attribute can't be added with a name that matches a vertex group.
BKE_attribute_calc_unique_name didn't account for this,
so the name generated for the new UV map collided with the vertex group,
the attribute was never added,
and ED_mesh_uv_add returned a stale index.

Prevent collisions with vertex group names in
BKE_attribute_calc_unique_name.

Also convert attributes using BKE_attribute_remove instead of the
raw accessor, so the active UV map and color attribute stay valid
(matching the remove operator).

Ref !160314

[0]: 8adba33029

source/blender/blenkernel/intern/attribute.cc

@@ -30,6 +30,7 @@
 #include "BKE_attribute_legacy_convert.hh"
 #include "BKE_curves.hh"
 #include "BKE_customdata.hh"
+#include "BKE_deform.hh"
 #include "BKE_editmesh.hh"
 #include "BKE_grease_pencil.hh"
 #include "BKE_mesh.hh"
@@ -352,6 +353,13 @@ std::string BKE_attribute_calc_unique_name(const AttributeOwner &owner, const St
   const StringRef name_final = name.is_empty() ? DATA_("Attribute") : name;
   if (owner.type() == AttributeOwnerType::Mesh) {
     const Mesh &mesh = *owner.get_mesh();
+    /* While attribute names may collide with vertex group names,
+     * it's important not to allow this when requesting a unique name
+     * because #MutableAttributeAccessor::add will consider the layer as "existing",
+     * and not add the new attribute as expected. See: #160029. */
+    const auto is_used_vertex_group = [&](const StringRef check_name) {
+      return BKE_defgroup_name_index(&mesh.vertex_group_names, check_name) != -1;
+    };
     if (mesh.runtime->edit_mesh) {
       Set<StringRef, 8> names;
       const auto add_names = [&](const CustomData &data) {
@@ -368,11 +376,19 @@ std::string BKE_attribute_calc_unique_name(const AttributeOwner &owner, const St
       add_names(bm.ldata);
       return BLI_uniquename_cb(
           [&](const StringRef new_name) {
-            return names.contains(new_name) || BM_attribute_stored_in_bmesh_builtin(new_name);
+            return names.contains(new_name) || BM_attribute_stored_in_bmesh_builtin(new_name) ||
+                   is_used_vertex_group(new_name);
           },
           '.',
           name_final);
     }
+    const bke::AttributeStorage &storage = *owner.get_storage();
+    return BLI_uniquename_cb(
+        [&](const StringRef check_name) {
+          return storage.lookup(check_name) != nullptr || is_used_vertex_group(check_name);
+        },
+        '.',
+        name_final);
   }
 
   bke::AttributeStorage &storage = *owner.get_storage();

source/blender/editors/geometry/geometry_attributes.cc

@@ -407,7 +407,7 @@ static wmOperatorStatus geometry_attribute_add_exec(bContext *C, wmOperator *op)
 
   const CPPType &cpp_type = bke::attribute_type_to_cpp_type(type);
   bke::Attribute &attr = attributes.add(
-      attributes.unique_name_calc(name),
+      BKE_attribute_calc_unique_name(owner, name),
       bke::AttrDomain(domain),
       type,
       bke::Attribute::ArrayData::from_default_value(cpp_type, domain_size));
@@ -642,6 +642,13 @@ bool convert_attribute(AttributeOwner &owner,
 
   const bool was_active = BKE_attributes_active_name_get(owner) == name;
 
+  /* Support restoring names after removing, note that this could be a utility. */
+  Mesh *mesh = owner.type() == AttributeOwnerType::Mesh ? owner.get_mesh() : nullptr;
+  const bool was_active_color = mesh && name == StringRef(mesh->active_color_attribute);
+  const bool was_default_color = mesh && name == StringRef(mesh->default_color_attribute);
+  const bool was_active_uv = mesh && name == mesh->active_uv_map_name();
+  const bool was_default_uv = mesh && name == mesh->default_uv_map_name();
+
   const std::string name_copy = name;
   const GVArray varray = *attributes.lookup_or_default(name_copy, dst_domain, dst_type);
 
@@ -649,7 +656,10 @@ bool convert_attribute(AttributeOwner &owner,
   void *new_data = MEM_new_uninitialized_aligned(
       varray.size() * cpp_type.size, cpp_type.alignment, __func__);
   varray.materialize_to_uninitialized(new_data);
-  attributes.remove(name_copy);
+  if (!BKE_attribute_remove(owner, name_copy, reports)) {
+    MEM_delete_void(new_data);
+    return false;
+  }
   if (!attributes.add(name_copy, dst_domain, dst_type, bke::AttributeInitMoveArray(new_data))) {
     MEM_delete_void(new_data);
   }
@@ -659,6 +669,24 @@ bool convert_attribute(AttributeOwner &owner,
      * change its index, so reassign the active attribute if necessary. */
     BKE_attributes_active_set(owner, name_copy);
   }
+  if (mesh) {
+    if (bke::mesh::is_color_attribute({dst_domain, dst_type})) {
+      if (was_active_color) {
+        BKE_id_attributes_active_color_set(&mesh->id, name_copy);
+      }
+      if (was_default_color) {
+        BKE_id_attributes_default_color_set(&mesh->id, name_copy);
+      }
+    }
+    else if (bke::mesh::is_uv_map({dst_domain, dst_type})) {
+      if (was_active_uv) {
+        mesh->uv_maps_active_set(name_copy);
+      }
+      if (was_default_uv) {
+        mesh->uv_maps_default_set(name_copy);
+      }
+    }
+  }
 
   return true;
 }
@@ -690,7 +718,9 @@ static wmOperatorStatus geometry_attribute_convert_exec(bContext *C, wmOperator
         VArray<float> src_varray = *attributes.lookup_or_default<float>(
             name, bke::AttrDomain::Point, 0.0f);
         src_varray.materialize(src_weights);
-        attributes.remove(name);
+        if (!BKE_attribute_remove(owner, name, op->reports)) {
+          return OPERATOR_CANCELLED;
+        }
 
         bDeformGroup *defgroup = BKE_object_defgroup_new(ob, name);
         const int defgroup_index = BLI_findindex(BKE_id_defgroup_list_get(&mesh->id), defgroup);
@@ -702,7 +732,6 @@ static wmOperatorStatus geometry_attribute_convert_exec(bContext *C, wmOperator
           }
         }
         BKE_object_defgroup_active_index_set(ob, defgroup_index + 1);
-        AttributeOwner owner = AttributeOwner::from_id(&mesh->id);
         int *active_index = BKE_attributes_active_index_p(owner);
         if (*active_index > 0) {
           *active_index -= 1;

source/blender/makesrna/intern/rna_attribute.cc

@@ -874,7 +874,7 @@ static PointerRNA rna_AttributeGroupID_new(
   bke::AttributeStorage &attributes = *owner.get_storage();
   const CPPType &cpp_type = *bke::custom_data_type_to_cpp_type(eCustomDataType(type));
   bke::Attribute &attr = attributes.add(
-      attributes.unique_name_calc(name),
+      BKE_attribute_calc_unique_name(owner, name),
       AttrDomain(domain),
       *bke::custom_data_type_to_attr_type(eCustomDataType(type)),
       bke::Attribute::ArrayData::from_default_value(cpp_type, domain_size));

tests/python/bl_geometry_attributes.py

@@ -101,6 +101,79 @@ class TestMesh(unittest.TestCase):
         self.assertTrue(self.mesh.uv_layers.active.name == "a")
 
 
+class MeshObjectTest(unittest.TestCase):
+    def setUp(self):
+        self.mesh = bpy.data.meshes.new("test")
+        self.mesh.from_pydata([(0, 0, 0), (1, 0, 0), (1, 1, 0), (0, 1, 0)], [], [(0, 1, 2, 3)])
+        self.obj = bpy.data.objects.new("test", self.mesh)
+        bpy.context.scene.collection.objects.link(self.obj)
+        bpy.context.view_layer.objects.active = self.obj
+
+    def tearDown(self):
+        bpy.data.objects.remove(self.obj)
+        bpy.data.meshes.remove(self.mesh)
+        del self.obj
+        del self.mesh
+
+
+class TestMeshVertexGroupNameClash(MeshObjectTest):
+    def test_add_attribute(self):
+        self.obj.vertex_groups.new(name="UVMap")
+        attribute = self.mesh.attributes.new("UVMap", 'FLOAT2', 'CORNER')
+        self.assertFalse(attribute.name == "UVMap")
+        self.assertTrue("UVMap" in [group.name for group in self.obj.vertex_groups])
+
+    def test_add_attribute_operator(self):
+        self.obj.vertex_groups.new(name="UVMap")
+        bpy.ops.geometry.attribute_add(name="UVMap", domain='CORNER', data_type='FLOAT2')
+        self.assertFalse(self.mesh.attributes.active.name == "UVMap")
+        self.assertTrue("UVMap" in [group.name for group in self.obj.vertex_groups])
+
+    def test_add_uv_map(self):
+        self.obj.vertex_groups.new(name="UVMap")
+        uv_map = self.mesh.uv_layers.new(name="UVMap")
+        self.assertFalse(uv_map.name == "UVMap")
+        self.assertTrue("UVMap" in [group.name for group in self.obj.vertex_groups])
+
+    def test_convert_to_vertex_group_then_add_uv(self):
+        self.mesh.uv_layers.new(name="UVMap")
+        self.mesh.attributes.active = self.mesh.attributes["UVMap"]
+        bpy.ops.geometry.attribute_convert(mode='VERTEX_GROUP')
+        uv_map = self.mesh.uv_layers.new()
+        self.assertFalse(uv_map.name == "UVMap")
+
+
+class TestMeshAttributeConvert(MeshObjectTest):
+    def test_convert_active_color_to_generic(self):
+        self.mesh.attributes.new("Col", 'FLOAT_COLOR', 'POINT')
+        self.mesh.attributes.active = self.mesh.attributes["Col"]
+        self.assertTrue(self.mesh.attributes.active_color_name == "Col")
+        bpy.ops.geometry.attribute_convert(mode='GENERIC', domain='POINT', data_type='FLOAT')
+        self.assertTrue(self.mesh.attributes.active_color_name == "")
+
+    def test_convert_active_color_to_generic_picks_next(self):
+        self.mesh.attributes.new("ColA", 'FLOAT_COLOR', 'POINT')
+        self.mesh.attributes.new("ColB", 'FLOAT_COLOR', 'POINT')
+        self.assertTrue(self.mesh.attributes.active_color_name == "ColA")
+        self.mesh.attributes.active = self.mesh.attributes["ColA"]
+        bpy.ops.geometry.attribute_convert(mode='GENERIC', domain='POINT', data_type='FLOAT')
+        self.assertTrue(self.mesh.attributes.active_color_name == "ColB")
+
+    def test_convert_active_color_to_color_keeps_active(self):
+        self.mesh.attributes.new("Col", 'FLOAT_COLOR', 'POINT')
+        self.mesh.attributes.active = self.mesh.attributes["Col"]
+        bpy.ops.geometry.attribute_convert(mode='GENERIC', domain='POINT', data_type='BYTE_COLOR')
+        self.assertTrue(self.mesh.attributes.active_color_name == "Col")
+
+    def test_convert_active_uv_to_uv_keeps_active(self):
+        self.mesh.uv_layers.new(name="UVA")
+        self.mesh.uv_layers.new(name="UVB")
+        self.mesh.uv_layers.active = self.mesh.uv_layers["UVA"]
+        self.mesh.attributes.active = self.mesh.attributes["UVA"]
+        bpy.ops.geometry.attribute_convert(mode='GENERIC', domain='CORNER', data_type='FLOAT2')
+        self.assertTrue(self.mesh.uv_layers.active.name == "UVA")
+
+
 if __name__ == '__main__':
     import sys
     sys.argv = [__file__] + (sys.argv[sys.argv.index("--") + 1:] if "--" in sys.argv else [])