From 8934f00ac5701ea349f2bcccab32e252c79aa730 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Sat, 17 Sep 2022 14:38:30 -0500 Subject: 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 --- source/blender/blenkernel/BKE_attribute.hh | 35 +++++++++++++- source/blender/blenkernel/BKE_curves.hh | 2 +- .../blender/blenkernel/intern/attribute_access.cc | 11 +++++ .../blenkernel/intern/attribute_access_intern.hh | 40 ++++++++++++++-- .../blenkernel/intern/geometry_component_curves.cc | 55 +++++++++++++++++++--- .../blenkernel/intern/geometry_component_mesh.cc | 12 ++++- source/blender/modifiers/intern/MOD_nodes.cc | 6 ++- .../geometry/nodes/node_geo_set_material_index.cc | 4 +- .../nodes/node_geo_store_named_attribute.cc | 4 +- 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 { @@ -162,6 +166,27 @@ template 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 @@ -343,7 +368,7 @@ struct AttributeAccessorFunctions { eAttrDomain to_domain); bool (*for_all)(const void *owner, FunctionRef 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, @@ -497,6 +522,14 @@ class AttributeAccessor { return VArray::ForSingle(default_value, this->domain_size(domain)); } + /** + * 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. */ 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 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 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), @@ -378,6 +392,21 @@ inline bool for_all(const void *owner, return true; } +template +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 inline bool contains(const void *owner, const blender::bke::AttributeIDRef &attribute_id) { @@ -489,6 +518,7 @@ inline AttributeAccessorFunctions accessor_functions_for_providers() lookup, nullptr, for_all, + lookup_validator, lookup_for_write, remove, add}; 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, tag_component_positions_changed); + static const fn::CustomMF_SI_SO handle_type_clamp{ + "Handle Type Validate", + [](int8_t value) { + return std::clamp(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, make_array_write_attribute, - 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, make_array_write_attribute, - 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, tag_component_positions_changed); + static const fn::CustomMF_SI_SO nurbs_order_clamp{ + "NURBS Order Validate", + [](int8_t value) { return std::max(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, make_array_write_attribute, - tag_component_topology_changed); + tag_component_topology_changed, + AttributeValidator{&nurbs_order_clamp}); + static const fn::CustomMF_SI_SO normal_mode_clamp{ + "Normal Mode Validate", + [](int8_t value) { + return std::clamp(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, make_array_write_attribute, - tag_component_normals_changed); + tag_component_normals_changed, + AttributeValidator{&normal_mode_clamp}); + static const fn::CustomMF_SI_SO knots_mode_clamp{ + "Knots Mode Validate", + [](int8_t value) { + return std::clamp(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, make_array_write_attribute, - tag_component_topology_changed); + tag_component_topology_changed, + AttributeValidator{&knots_mode_clamp}); + static const fn::CustomMF_SI_SO curve_type_clamp{ + "Curve Type Validate", + [](int8_t value) { + return std::clamp(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, make_array_write_attribute, - tag_component_curve_types_changed); + tag_component_curve_types_changed, + AttributeValidator{&curve_type_clamp}); + static const fn::CustomMF_SI_SO resolution_clamp{ + "Resolution Validate", + [](int value) { return std::max(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, make_array_write_attribute, - 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, nullptr); + static const fn::CustomMF_SI_SO 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(value, 0, std::numeric_limits::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, make_array_write_attribute, - 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 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 indices = attributes.lookup_or_add_for_write("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)) { -- cgit v1.2.3