Attributes: Validate some builtin attributes for untrusted inputs

We expect some builtin attributes to have positive values or values
within a certain range, but currently there some cases where users
can set attributes to arbitrary values: the store named attribute node,
and the output attributes of the geometry nodes modifier. The set
material index node also needs validation.

This patch adds an `AttributeValidator` to the attribute API, which
can be used to correct values from these untrusted inputs if necessary.
As an alternative to D15548, this approach makes it much easier to
understand when validation is being applied, without the need to add
arguments to every attribute API method or complicate the virtual
array system.

Currently validation is provided with a multi-function. That integrates
well with the field evaluations that set these values now, but it could
be wrapped to be friendlier to other areas of Blender in the future.

The Python API is not handled here either. Currently I would prefer to
wait until we can integrate the C++ and C attribute APIs better before
addressing that.

Fixes T100952

Differential Revision: https://developer.blender.org/D15990

9 files changed, 151 insertions, 18 deletions

diff --git a/source/blender/blenkernel/BKE_attribute.hh b/source/blender/blenkernel/BKE_attribute.hh
index fbdacee139c..946a7d21580 100644
--- a/source/blender/blenkernel/BKE_attribute.hh
+++ b/source/blender/blenkernel/BKE_attribute.hh
@@ -16,6 +16,10 @@
 
 struct Mesh;
 struct PointCloud;
+namespace blender::fn {
+class MultiFunction;
+class GField;
+}  // namespace blender::fn
 
 namespace blender::bke {
 
@@ -163,6 +167,27 @@ template<typename T> struct AttributeReader {
 };
 
 /**
+ * A utility to make sure attribute values are valid, for attributes like "material_index" which
+ * can only be positive, or attributes that represent enum options. This is usually only necessary
+ * when writing attributes from an untrusted/arbitrary user input.
+ */
+struct AttributeValidator {
+  /**
+   * Single input, single output function that corrects attribute values if necessary.
+   */
+  const fn::MultiFunction *function;
+
+  operator bool() const
+  {
+    return this->function != nullptr;
+  }
+  /**
+   * Return a field that creates corrected attribute values.
+   */
+  fn::GField validate_field_if_necessary(const fn::GField &field) const;
+};
+
+/**
  * Result when looking up an attribute from some geometry with read and write access. After writing
  * to the attribute, the #finish method has to be called. This may invalidate caches based on this
  * attribute.
@@ -343,7 +368,7 @@ struct AttributeAccessorFunctions {
                           eAttrDomain to_domain);
   bool (*for_all)(const void *owner,
                   FunctionRef<bool(const AttributeIDRef &, const AttributeMetaData &)> fn);
-
+  AttributeValidator (*lookup_validator)(const void *owner, const AttributeIDRef &attribute_id);
   GAttributeWriter (*lookup_for_write)(void *owner, const AttributeIDRef &attribute_id);
   bool (*remove)(void *owner, const AttributeIDRef &attribute_id);
   bool (*add)(void *owner,
@@ -498,6 +523,14 @@ class AttributeAccessor {
   }
 
   /**
+   * Same as the generic version above, but should be used when the type is known at compile time.
+   */
+  AttributeValidator lookup_validator(const AttributeIDRef &attribute_id) const
+  {
+    return fn_->lookup_validator(owner_, attribute_id);
+  }
+
+  /**
    * Interpolate data from one domain to another.
    */
   GVArray adapt_domain(const GVArray &varray,
diff --git a/source/blender/blenkernel/BKE_curves.hh b/source/blender/blenkernel/BKE_curves.hh
index 9f150c13d6e..6758e5c268b 100644
--- a/source/blender/blenkernel/BKE_curves.hh
+++ b/source/blender/blenkernel/BKE_curves.hh
@@ -218,7 +218,7 @@ class CurvesGeometry : public ::CurvesGeometry {
 
   /**
    * How many evaluated points to create for each segment when evaluating Bezier,
-   * Catmull Rom, and NURBS curves. On the curve domain.
+   * Catmull Rom, and NURBS curves. On the curve domain. Values must be zero or greater.
    */
   VArray<int> resolution() const;
   /** Mutable access to curve resolution. Call #tag_topology_changed after changes. */
diff --git a/source/blender/blenkernel/intern/attribute_access.cc b/source/blender/blenkernel/intern/attribute_access.cc
index e7c1e35f851..df7787986db 100644
--- a/source/blender/blenkernel/intern/attribute_access.cc
+++ b/source/blender/blenkernel/intern/attribute_access.cc
@@ -19,6 +19,8 @@
 #include "BLI_math_vec_types.hh"
 #include "BLI_span.hh"
 
+#include "FN_field.hh"
+
 #include "BLT_translation.h"
 
 #include "CLG_log.h"
@@ -945,6 +947,15 @@ GSpanAttributeWriter MutableAttributeAccessor::lookup_or_add_for_write_only_span
   return {};
 }
 
+fn::GField AttributeValidator::validate_field_if_necessary(const fn::GField &field) const
+{
+  if (function) {
+    auto validate_op = fn::FieldOperation::Create(*function, {field});
+    return fn::GField(validate_op);
+  }
+  return field;
+}
+
 Vector<AttributeTransferData> retrieve_attributes_for_transfer(
     const bke::AttributeAccessor src_attributes,
     bke::MutableAttributeAccessor dst_attributes,
diff --git a/source/blender/blenkernel/intern/attribute_access_intern.hh b/source/blender/blenkernel/intern/attribute_access_intern.hh
index 8050f45da94..e9023e28297 100644
--- a/source/blender/blenkernel/intern/attribute_access_intern.hh
+++ b/source/blender/blenkernel/intern/attribute_access_intern.hh
@@ -53,6 +53,7 @@ class BuiltinAttributeProvider {
   const CreatableEnum createable_;
   const WritableEnum writable_;
   const DeletableEnum deletable_;
+  const AttributeValidator validator_;
 
  public:
   BuiltinAttributeProvider(std::string name,
@@ -60,13 +61,15 @@ class BuiltinAttributeProvider {
                            const eCustomDataType data_type,
                            const CreatableEnum createable,
                            const WritableEnum writable,
-                           const DeletableEnum deletable)
+                           const DeletableEnum deletable,
+                           AttributeValidator validator = {})
       : name_(std::move(name)),
         domain_(domain),
         data_type_(data_type),
         createable_(createable),
         writable_(writable),
-        deletable_(deletable)
+        deletable_(deletable),
+        validator_(validator)
   {
   }
 
@@ -90,6 +93,11 @@ class BuiltinAttributeProvider {
   {
     return data_type_;
   }
+
+  AttributeValidator validator() const
+  {
+    return validator_;
+  }
 };
 
 /**
@@ -241,9 +249,15 @@ class BuiltinCustomDataLayerProvider final : public BuiltinAttributeProvider {
                                  const CustomDataAccessInfo custom_data_access,
                                  const AsReadAttribute as_read_attribute,
                                  const AsWriteAttribute as_write_attribute,
-                                 const UpdateOnChange update_on_write)
-      : BuiltinAttributeProvider(
-            std::move(attribute_name), domain, attribute_type, creatable, writable, deletable),
+                                 const UpdateOnChange update_on_write,
+                                 const AttributeValidator validator = {})
+      : BuiltinAttributeProvider(std::move(attribute_name),
+                                 domain,
+                                 attribute_type,
+                                 creatable,
+                                 writable,
+                                 deletable,
+                                 validator),
         stored_type_(stored_type),
         custom_data_access_(custom_data_access),
         as_read_attribute_(as_read_attribute),
@@ -379,6 +393,21 @@ inline bool for_all(const void *owner,
 }
 
 template<const ComponentAttributeProviders &providers>
+inline AttributeValidator lookup_validator(const void * /*owner*/,
+                                           const blender::bke::AttributeIDRef &attribute_id)
+{
+  if (!attribute_id.is_named()) {
+    return {};
+  }
+  const auto &builtin_providers = providers.builtin_attribute_providers();
+  const BuiltinAttributeProvider *provider = builtin_providers.lookup_as(attribute_id.name());
+  if (!provider) {
+    return {};
+  }
+  return provider->validator();
+}
+
+template<const ComponentAttributeProviders &providers>
 inline bool contains(const void *owner, const blender::bke::AttributeIDRef &attribute_id)
 {
   bool found = false;
@@ -489,6 +518,7 @@ inline AttributeAccessorFunctions accessor_functions_for_providers()
                                     lookup<providers>,
                                     nullptr,
                                     for_all<providers>,
+                                    lookup_validator<providers>,
                                     lookup_for_write<providers>,
                                     remove<providers>,
                                     add<providers>};
diff --git a/source/blender/blenkernel/intern/geometry_component_curves.cc b/source/blender/blenkernel/intern/geometry_component_curves.cc
index 83a35534c01..d8cfe1374dc 100644
--- a/source/blender/blenkernel/intern/geometry_component_curves.cc
+++ b/source/blender/blenkernel/intern/geometry_component_curves.cc
@@ -12,6 +12,8 @@
 #include "BKE_geometry_set.hh"
 #include "BKE_lib_id.h"
 
+#include "FN_multi_function_builder.hh"
+
 #include "attribute_access_intern.hh"
 
 using blender::GVArray;
@@ -426,6 +428,12 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                     make_array_write_attribute<float3>,
                                                     tag_component_positions_changed);
 
+  static const fn::CustomMF_SI_SO<int8_t, int8_t> handle_type_clamp{
+      "Handle Type Validate",
+      [](int8_t value) {
+        return std::clamp<int8_t>(value, BEZIER_HANDLE_FREE, BEZIER_HANDLE_ALIGN);
+      },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider handle_type_right("handle_type_right",
                                                           ATTR_DOMAIN_POINT,
                                                           CD_PROP_INT8,
@@ -436,7 +444,8 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                           point_access,
                                                           make_array_read_attribute<int8_t>,
                                                           make_array_write_attribute<int8_t>,
-                                                          tag_component_topology_changed);
+                                                          tag_component_topology_changed,
+                                                          AttributeValidator{&handle_type_clamp});
 
   static BuiltinCustomDataLayerProvider handle_type_left("handle_type_left",
                                                          ATTR_DOMAIN_POINT,
@@ -448,7 +457,8 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                          point_access,
                                                          make_array_read_attribute<int8_t>,
                                                          make_array_write_attribute<int8_t>,
-                                                         tag_component_topology_changed);
+                                                         tag_component_topology_changed,
+                                                         AttributeValidator{&handle_type_clamp});
 
   static BuiltinCustomDataLayerProvider nurbs_weight("nurbs_weight",
                                                      ATTR_DOMAIN_POINT,
@@ -462,6 +472,10 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                      make_array_write_attribute<float>,
                                                      tag_component_positions_changed);
 
+  static const fn::CustomMF_SI_SO<int8_t, int8_t> nurbs_order_clamp{
+      "NURBS Order Validate",
+      [](int8_t value) { return std::max<int8_t>(value, 0); },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider nurbs_order("nurbs_order",
                                                     ATTR_DOMAIN_CURVE,
                                                     CD_PROP_INT8,
@@ -472,8 +486,15 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                     curve_access,
                                                     make_array_read_attribute<int8_t>,
                                                     make_array_write_attribute<int8_t>,
-                                                    tag_component_topology_changed);
+                                                    tag_component_topology_changed,
+                                                    AttributeValidator{&nurbs_order_clamp});
 
+  static const fn::CustomMF_SI_SO<int8_t, int8_t> normal_mode_clamp{
+      "Normal Mode Validate",
+      [](int8_t value) {
+        return std::clamp<int8_t>(value, NORMAL_MODE_MINIMUM_TWIST, NORMAL_MODE_Z_UP);
+      },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider normal_mode("normal_mode",
                                                     ATTR_DOMAIN_CURVE,
                                                     CD_PROP_INT8,
@@ -484,8 +505,15 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                     curve_access,
                                                     make_array_read_attribute<int8_t>,
                                                     make_array_write_attribute<int8_t>,
-                                                    tag_component_normals_changed);
+                                                    tag_component_normals_changed,
+                                                    AttributeValidator{&normal_mode_clamp});
 
+  static const fn::CustomMF_SI_SO<int8_t, int8_t> knots_mode_clamp{
+      "Knots Mode Validate",
+      [](int8_t value) {
+        return std::clamp<int8_t>(value, NURBS_KNOT_MODE_NORMAL, NURBS_KNOT_MODE_ENDPOINT_BEZIER);
+      },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider nurbs_knots_mode("knots_mode",
                                                          ATTR_DOMAIN_CURVE,
                                                          CD_PROP_INT8,
@@ -496,8 +524,15 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                          curve_access,
                                                          make_array_read_attribute<int8_t>,
                                                          make_array_write_attribute<int8_t>,
-                                                         tag_component_topology_changed);
+                                                         tag_component_topology_changed,
+                                                         AttributeValidator{&knots_mode_clamp});
 
+  static const fn::CustomMF_SI_SO<int8_t, int8_t> curve_type_clamp{
+      "Curve Type Validate",
+      [](int8_t value) {
+        return std::clamp<int8_t>(value, CURVE_TYPE_CATMULL_ROM, CURVE_TYPES_NUM);
+      },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider curve_type("curve_type",
                                                    ATTR_DOMAIN_CURVE,
                                                    CD_PROP_INT8,
@@ -508,8 +543,13 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                    curve_access,
                                                    make_array_read_attribute<int8_t>,
                                                    make_array_write_attribute<int8_t>,
-                                                   tag_component_curve_types_changed);
+                                                   tag_component_curve_types_changed,
+                                                   AttributeValidator{&curve_type_clamp});
 
+  static const fn::CustomMF_SI_SO<int, int> resolution_clamp{
+      "Resolution Validate",
+      [](int value) { return std::max<int>(value, 0); },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider resolution("resolution",
                                                    ATTR_DOMAIN_CURVE,
                                                    CD_PROP_INT32,
@@ -520,7 +560,8 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
                                                    curve_access,
                                                    make_array_read_attribute<int>,
                                                    make_array_write_attribute<int>,
-                                                   tag_component_topology_changed);
+                                                   tag_component_topology_changed,
+                                                   AttributeValidator{&resolution_clamp});
 
   static BuiltinCustomDataLayerProvider cyclic("cyclic",
                                                ATTR_DOMAIN_CURVE,
diff --git a/source/blender/blenkernel/intern/geometry_component_mesh.cc b/source/blender/blenkernel/intern/geometry_component_mesh.cc
index 715c7d6c743..255d0e92964 100644
--- a/source/blender/blenkernel/intern/geometry_component_mesh.cc
+++ b/source/blender/blenkernel/intern/geometry_component_mesh.cc
@@ -14,6 +14,8 @@
 #include "BKE_lib_id.h"
 #include "BKE_mesh.h"
 
+#include "FN_multi_function_builder.hh"
+
 #include "attribute_access_intern.hh"
 
 extern "C" MDeformVert *BKE_object_defgroup_data_create(ID *id);
@@ -1217,6 +1219,13 @@ static ComponentAttributeProviders create_attribute_providers_for_mesh()
                                            make_array_write_attribute<int>,
                                            nullptr);
 
+  static const fn::CustomMF_SI_SO<int, int> material_index_clamp{
+      "Material Index Validate",
+      [](int value) {
+        /* Use #short for the maximum since many areas still use that type for indices. */
+        return std::clamp<int>(value, 0, std::numeric_limits<short>::max());
+      },
+      fn::CustomMF_presets::AllSpanOrSingle()};
   static BuiltinCustomDataLayerProvider material_index("material_index",
                                                        ATTR_DOMAIN_FACE,
                                                        CD_PROP_INT32,
@@ -1227,7 +1236,8 @@ static ComponentAttributeProviders create_attribute_providers_for_mesh()
                                                        face_access,
                                                        make_array_read_attribute<int>,
                                                        make_array_write_attribute<int>,
-                                                       nullptr);
+                                                       nullptr,
+                                                       AttributeValidator{&material_index_clamp});
 
   static BuiltinCustomDataLayerProvider shade_smooth(
       "shade_smooth",
diff --git a/source/blender/modifiers/intern/MOD_nodes.cc b/source/blender/modifiers/intern/MOD_nodes.cc
index ff8f851c1e8..2910f44e37a 100644
--- a/source/blender/modifiers/intern/MOD_nodes.cc
+++ b/source/blender/modifiers/intern/MOD_nodes.cc
@@ -114,7 +114,9 @@ using blender::StringRef;
 using blender::StringRefNull;
 using blender::Vector;
 using blender::bke::AttributeMetaData;
+using blender::bke::AttributeValidator;
 using blender::fn::Field;
+using blender::fn::FieldOperation;
 using blender::fn::GField;
 using blender::fn::ValueOrField;
 using blender::fn::ValueOrFieldCPPType;
@@ -1046,13 +1048,15 @@ static Vector<OutputAttributeToStore> compute_attributes_to_store(
       blender::fn::FieldEvaluator field_evaluator{field_context, domain_size};
       for (const OutputAttributeInfo &output_info : outputs_info) {
         const CPPType &type = output_info.field.cpp_type();
+        const AttributeValidator validator = attributes.lookup_validator(output_info.name);
         OutputAttributeToStore store{
             component_type,
             domain,
             output_info.name,
             GMutableSpan{
                 type, MEM_malloc_arrayN(domain_size, type.size(), __func__), domain_size}};
-        field_evaluator.add_with_destination(output_info.field, store.data);
+        GField field = validator.validate_field_if_necessary(output_info.field);
+        field_evaluator.add_with_destination(std::move(field), store.data);
         attributes_to_store.append(store);
       }
       field_evaluator.evaluate();
diff --git a/source/blender/nodes/geometry/nodes/node_geo_set_material_index.cc b/source/blender/nodes/geometry/nodes/node_geo_set_material_index.cc
index f6dded56315..bb9ac9b5d4c 100644
--- a/source/blender/nodes/geometry/nodes/node_geo_set_material_index.cc
+++ b/source/blender/nodes/geometry/nodes/node_geo_set_material_index.cc
@@ -24,11 +24,13 @@ static void set_material_index_in_component(GeometryComponent &component,
   MutableAttributeAccessor attributes = *component.attributes_for_write();
   bke::GeometryFieldContext field_context{component, domain};
 
+  const bke::AttributeValidator validator = attributes.lookup_validator("material_index");
   AttributeWriter<int> indices = attributes.lookup_or_add_for_write<int>("material_index", domain);
 
   fn::FieldEvaluator evaluator{field_context, domain_size};
   evaluator.set_selection(selection_field);
-  evaluator.add_with_destination(index_field, indices.varray);
+  evaluator.add_with_destination(validator.validate_field_if_necessary(index_field),
+                                 indices.varray);
   evaluator.evaluate();
   indices.finish();
 }
diff --git a/source/blender/nodes/geometry/nodes/node_geo_store_named_attribute.cc b/source/blender/nodes/geometry/nodes/node_geo_store_named_attribute.cc
index 2a590f5bf4a..ad31c8a2191 100644
--- a/source/blender/nodes/geometry/nodes/node_geo_store_named_attribute.cc
+++ b/source/blender/nodes/geometry/nodes/node_geo_store_named_attribute.cc
@@ -103,6 +103,7 @@ static void try_capture_field_on_geometry(GeometryComponent &component,
 
   const CPPType &type = field.cpp_type();
   const eCustomDataType data_type = bke::cpp_type_to_custom_data_type(type);
+  const bke::AttributeValidator validator = attributes.lookup_validator(name);
 
   /* Could avoid allocating a new buffer if:
    * - We are writing to an attribute that exists already with the correct domain and type.
@@ -110,7 +111,8 @@ static void try_capture_field_on_geometry(GeometryComponent &component,
   void *buffer = MEM_mallocN(type.size() * domain_size, __func__);
 
   fn::FieldEvaluator evaluator{field_context, &mask};
-  evaluator.add_with_destination(field, GMutableSpan{type, buffer, domain_size});
+  evaluator.add_with_destination(validator.validate_field_if_necessary(field),
+                                 GMutableSpan{type, buffer, domain_size});
   evaluator.evaluate();
 
   if (GAttributeWriter attribute = attributes.lookup_for_write(name)) {