From ada367a0e9278038d76f0aef4fd82d42412b9cbc Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Thu, 20 Jun 2024 15:24:38 +0200 Subject: [PATCH] Fix #119589: use-after-free when accessing not-fully-evaluated object geometry While the evaluated result is not well defined, we expect Blender to not crash when there are dependency cycles. The evaluation of one object often takes the evaluated geometry of another object into account. This works fine if the other object is already fully evaluated. However, if there is a dependency cycle, the other object may not be evaluated already. Currently, we have no way to check for this and were mostly just relying on luck that the other objects geometry is in some valid state (even if it's not the fully evaluated geometry). This patch adds the ability to explicitly check if an objects geometry is fully evaluated already, so that it can be accessed by other objects. If there are not dependency cycles, this should always be true. If not, it may be false sometimes, and in this case the other objects geometry should be ignored. The same also applies to the object transforms and the geometry of a collection. For that, new functions are added in `DEG_depsgraph_query.hh`. Those should be used whenever accessing another objects or collections object during depsgraph evaluation. More similar functions may be added in the future. ``` bool DEG_object_geometry_is_evaluated(const Object &object); bool DEG_object_transform_is_evaluated(const Object &object); bool DEG_collection_geometry_is_evaluated(const Collection &collection); ``` To determine if the these components are fully evaluated, a reference to the corresponding depsgraph is needed. A possible solution to that is to pass the depsgraph through the call stack to these functions. While possible, there are a couple of annoyances. For one, the parameter would need to be added in many new places. I don't have an exact number, but it's like 50 or so. Another complication is that under some circumstances, multiple depsgraphs may have to be passed around, for example when evaluating node tools (also see `GeoNodesOperatorDepsgraphs`). To simplify the patch and other code in the future, a different route is taken where the depsgraph pointer is added to `ID_Runtime`, making it readily accessible similar to the `ID.orig_id`. The depsgraph pointer is set in the same place where the `orig_id` is set. As a nice side benefit, this also improves the situation in simple cases like having two cubes with a boolean modifier and they union each other. Pull Request: https://projects.blender.org/blender/blender/pulls/123444 --- .../intern/geometry_set_instances.cc | 5 ++ source/blender/blenkernel/intern/modifier.cc | 4 ++ source/blender/blenkernel/intern/object.cc | 8 +++ .../blender/depsgraph/DEG_depsgraph_query.hh | 28 ++++++++++ source/blender/depsgraph/intern/depsgraph.cc | 2 +- .../depsgraph/intern/depsgraph_query.cc | 56 +++++++++++++++++++ .../intern/eval/deg_eval_copy_on_write.cc | 5 +- .../intern/eval/deg_eval_copy_on_write.h | 4 +- .../depsgraph/intern/node/deg_node_id.cc | 4 +- .../depsgraph/intern/node/deg_node_id.hh | 2 +- source/blender/makesdna/DNA_ID.h | 8 +++ .../nodes/node_geo_collection_info.cc | 9 +++ .../geometry/nodes/node_geo_object_info.cc | 21 +++++++ 13 files changed, 148 insertions(+), 8 deletions(-) diff --git a/source/blender/blenkernel/intern/geometry_set_instances.cc b/source/blender/blenkernel/intern/geometry_set_instances.cc index 44fe085a2e7..593c6e88dff 100644 --- a/source/blender/blenkernel/intern/geometry_set_instances.cc +++ b/source/blender/blenkernel/intern/geometry_set_instances.cc @@ -13,6 +13,8 @@ #include "DNA_layer_types.h" #include "DNA_object_types.h" +#include "DEG_depsgraph_query.hh" + namespace blender::bke { static void add_final_mesh_as_geometry_component(const Object &object, GeometrySet &geometry_set) @@ -28,6 +30,9 @@ static void add_final_mesh_as_geometry_component(const Object &object, GeometryS GeometrySet object_get_evaluated_geometry_set(const Object &object) { + if (!DEG_object_geometry_is_evaluated(object)) { + return {}; + } if (object.type == OB_MESH && object.mode == OB_MODE_EDIT) { GeometrySet geometry_set; if (object.runtime->geometry_set_eval != nullptr) { diff --git a/source/blender/blenkernel/intern/modifier.cc b/source/blender/blenkernel/intern/modifier.cc index c7e1affa4c6..1382a1fff58 100644 --- a/source/blender/blenkernel/intern/modifier.cc +++ b/source/blender/blenkernel/intern/modifier.cc @@ -944,6 +944,10 @@ void BKE_modifier_deform_vertsEM(ModifierData *md, Mesh *BKE_modifier_get_evaluated_mesh_from_evaluated_object(Object *ob_eval) { + if (!DEG_object_geometry_is_evaluated(*ob_eval)) { + return nullptr; + } + Mesh *mesh = nullptr; if ((ob_eval->type == OB_MESH) && (ob_eval->mode & OB_MODE_EDIT)) { diff --git a/source/blender/blenkernel/intern/object.cc b/source/blender/blenkernel/intern/object.cc index 652daf9ca1f..003f1db5245 100644 --- a/source/blender/blenkernel/intern/object.cc +++ b/source/blender/blenkernel/intern/object.cc @@ -4138,6 +4138,10 @@ bool BKE_object_obdata_texspace_get(Object *ob, Mesh *BKE_object_get_evaluated_mesh_no_subsurf(const Object *object) { + if (!DEG_object_geometry_is_evaluated(*object)) { + return nullptr; + } + /* First attempt to retrieve the evaluated mesh from the evaluated geometry set. Most * object types either store it there or add a reference to it if it's owned elsewhere. */ blender::bke::GeometrySet *geometry_set_eval = object->runtime->geometry_set_eval; @@ -4165,6 +4169,10 @@ Mesh *BKE_object_get_evaluated_mesh_no_subsurf(const Object *object) Mesh *BKE_object_get_evaluated_mesh(const Object *object) { + if (!DEG_object_geometry_is_evaluated(*object)) { + return nullptr; + } + Mesh *mesh = BKE_object_get_evaluated_mesh_no_subsurf(object); if (!mesh) { return nullptr; diff --git a/source/blender/depsgraph/DEG_depsgraph_query.hh b/source/blender/depsgraph/DEG_depsgraph_query.hh index 0d0b73bb386..bfe83c22952 100644 --- a/source/blender/depsgraph/DEG_depsgraph_query.hh +++ b/source/blender/depsgraph/DEG_depsgraph_query.hh @@ -105,6 +105,16 @@ Object *DEG_get_original_object(Object *object); /** Get original version of given evaluated ID data-block. */ ID *DEG_get_original_id(ID *id); +/** + * Get the depsgraph that owns the given ID. This is efficient because the depsgraph is cached on + * the ID. + * + * Only IDs that use the copy-on-eval mechanism of depsgraph evaluation have a corresponding + * depsgraph. Original IDs as well as temporary IDs e.g. generated by geometry nodes do not have a + * corresponding depsgraph, so null is returned here for these IDs. + */ +Depsgraph *DEG_get_depsgraph_by_id(const ID &id); + /** * Check whether given ID is an original. * @@ -134,6 +144,24 @@ bool DEG_is_fully_evaluated(const Depsgraph *depsgraph); */ bool DEG_id_is_fully_evaluated(const Depsgraph *depsgraph, const ID *id_eval); +/** + * Returns false when the objects geometry is not fully evaluated in its depsgraph yet. In this + * case, the geometry must not be accessed. Otherwise returns true when geometry is fully evaluated + * or the object does not belong to any specific depsgraph. + * + * The result of this function is non deterministic when multi-threading is used because the + * depsgraph nodes are not totally ordered. When the depsgraph contains all correct relations and + * there are no cycles, the result should always be `true` here though. So it does not break + * determinism when there are no dependency graph cycles. + */ +bool DEG_object_geometry_is_evaluated(const Object &object); + +/** Same as above but for the transformation of the object. */ +bool DEG_object_transform_is_evaluated(const Object &object); + +/** Same as above but for the geometry of all objects in the collection. */ +bool DEG_collection_geometry_is_evaluated(const Collection &collection); + /** \} */ /* -------------------------------------------------------------------- */ diff --git a/source/blender/depsgraph/intern/depsgraph.cc b/source/blender/depsgraph/intern/depsgraph.cc index 2f76ee84f4b..0c05659668d 100644 --- a/source/blender/depsgraph/intern/depsgraph.cc +++ b/source/blender/depsgraph/intern/depsgraph.cc @@ -113,7 +113,7 @@ IDNode *Depsgraph::add_id_node(ID *id, ID *id_cow_hint) if (!id_node) { DepsNodeFactory *factory = type_get_factory(NodeType::ID_REF); id_node = (IDNode *)factory->create_node(id, "", id->name); - id_node->init_copy_on_write(id_cow_hint); + id_node->init_copy_on_write(*this, id_cow_hint); /* Register node in ID hash. * * NOTE: We address ID nodes by the original ID pointer they are diff --git a/source/blender/depsgraph/intern/depsgraph_query.cc b/source/blender/depsgraph/intern/depsgraph_query.cc index d6cba4e61c4..2174370edf6 100644 --- a/source/blender/depsgraph/intern/depsgraph_query.cc +++ b/source/blender/depsgraph/intern/depsgraph_query.cc @@ -288,6 +288,11 @@ ID *DEG_get_original_id(ID *id) return deg::get_original_id(id); } +Depsgraph *DEG_get_depsgraph_by_id(const ID &id) +{ + return id.runtime.depsgraph; +} + bool DEG_is_original_id(const ID *id) { /* Some explanation of the logic. @@ -360,3 +365,54 @@ bool DEG_id_is_fully_evaluated(const Depsgraph *depsgraph, const ID *id_eval) } return true; } + +static bool operation_needs_update(const ID &id, + const deg::NodeType component_type, + const deg::OperationCode opcode) +{ + const Depsgraph *depsgraph = DEG_get_depsgraph_by_id(id); + if (!depsgraph) { + return false; + } + const deg::Depsgraph °_graph = *reinterpret_cast(depsgraph); + /* Only us the original ID pointer to look up the IDNode, do not dereference it. */ + const ID *id_orig = deg::get_original_id(&id); + if (!id_orig) { + return false; + } + const deg::IDNode *id_node = deg_graph.find_id_node(id_orig); + if (!id_node) { + return false; + }; + const deg::ComponentNode *component_node = id_node->find_component(component_type); + if (!component_node) { + return false; + } + const deg::OperationNode *operation_node = component_node->find_operation(opcode); + if (!operation_node) { + return false; + } + /* Technically, there is potential for a race condition here, because the depsgraph evaluation + * might update this flag, but it's very unlikely to cause issues right now. Maybe this should + * become an atomic eventually. */ + const bool needs_update = operation_node->flag & deg::DEPSOP_FLAG_NEEDS_UPDATE; + return needs_update; +} + +bool DEG_object_geometry_is_evaluated(const Object &object) +{ + return !operation_needs_update( + object.id, deg::NodeType::GEOMETRY, deg::OperationCode::GEOMETRY_EVAL); +} + +bool DEG_object_transform_is_evaluated(const Object &object) +{ + return !operation_needs_update( + object.id, deg::NodeType::TRANSFORM, deg::OperationCode::TRANSFORM_FINAL); +} + +bool DEG_collection_geometry_is_evaluated(const Collection &collection) +{ + return !operation_needs_update( + collection.id, deg::NodeType::GEOMETRY, deg::OperationCode::GEOMETRY_EVAL_DONE); +} diff --git a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc index c350d8afe60..7c4a24f1558 100644 --- a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc +++ b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc @@ -832,7 +832,7 @@ ID *deg_expand_eval_copy_datablock(const Depsgraph *depsgraph, const IDNode *id_ #endif /* Do it now, so remapping will understand that possibly remapped self ID * is not to be remapped again. */ - deg_tag_eval_copy_id(id_cow, id_orig); + deg_tag_eval_copy_id(const_cast(*depsgraph), id_cow, id_orig); /* Perform remapping of the nodes. */ RemapCallbackUserData user_data = {nullptr}; user_data.depsgraph = depsgraph; @@ -1050,7 +1050,7 @@ bool deg_validate_eval_copy_datablock(ID *id_cow) return data.is_valid; } -void deg_tag_eval_copy_id(ID *id_cow, const ID *id_orig) +void deg_tag_eval_copy_id(deg::Depsgraph &depsgraph, ID *id_cow, const ID *id_orig) { BLI_assert(id_cow != id_orig); BLI_assert((id_orig->tag & LIB_TAG_COPIED_ON_EVAL) == 0); @@ -1058,6 +1058,7 @@ void deg_tag_eval_copy_id(ID *id_cow, const ID *id_orig) /* This ID is no longer localized, is a self-sustaining copy now. */ id_cow->tag &= ~LIB_TAG_LOCALIZED; id_cow->orig_id = (ID *)id_orig; + id_cow->runtime.depsgraph = &reinterpret_cast<::Depsgraph &>(depsgraph); } bool deg_eval_copy_is_expanded(const ID *id_cow) diff --git a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h index 3a060f1cdb1..f722f374ff3 100644 --- a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h +++ b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h @@ -56,8 +56,8 @@ void deg_create_eval_copy(struct ::Depsgraph *depsgraph, const struct IDNode *id */ bool deg_validate_eval_copy_datablock(ID *id_cow); -/** Tag given ID block as being copy-on-written. */ -void deg_tag_eval_copy_id(struct ID *id_cow, const struct ID *id_orig); +/** Tag given ID block as being copy-on-eval. */ +void deg_tag_eval_copy_id(Depsgraph &depsgraph, struct ID *id_cow, const struct ID *id_orig); /** * Check whether ID data-block is expanded. diff --git a/source/blender/depsgraph/intern/node/deg_node_id.cc b/source/blender/depsgraph/intern/node/deg_node_id.cc index b497f2e6037..61f627e4f77 100644 --- a/source/blender/depsgraph/intern/node/deg_node_id.cc +++ b/source/blender/depsgraph/intern/node/deg_node_id.cc @@ -79,7 +79,7 @@ void IDNode::init(const ID *id, const char * /*subdata*/) previously_visible_components_mask = 0; } -void IDNode::init_copy_on_write(ID *id_cow_hint) +void IDNode::init_copy_on_write(Depsgraph &depsgraph, ID *id_cow_hint) { /* Create pointer as early as possible, so we can use it for function * bindings. Rest of data we'll be copying to the new datablock when @@ -97,7 +97,7 @@ void IDNode::init_copy_on_write(ID *id_cow_hint) id_cow = (ID *)BKE_libblock_alloc_notest(GS(id_orig->name)); DEG_COW_PRINT( "Create shallow copy for %s: id_orig=%p id_cow=%p\n", id_orig->name, id_orig, id_cow); - deg_tag_eval_copy_id(id_cow, id_orig); + deg_tag_eval_copy_id(depsgraph, id_cow, id_orig); } else { id_cow = id_orig; diff --git a/source/blender/depsgraph/intern/node/deg_node_id.hh b/source/blender/depsgraph/intern/node/deg_node_id.hh index b2740c7275a..cf87e9f3c63 100644 --- a/source/blender/depsgraph/intern/node/deg_node_id.hh +++ b/source/blender/depsgraph/intern/node/deg_node_id.hh @@ -44,7 +44,7 @@ struct IDNode : public Node { /** Initialize 'id' node - from pointer data given. */ virtual void init(const ID *id, const char *subdata) override; - void init_copy_on_write(ID *id_cow_hint = nullptr); + void init_copy_on_write(Depsgraph &depsgraph, ID *id_cow_hint = nullptr); ~IDNode(); void destroy(); diff --git a/source/blender/makesdna/DNA_ID.h b/source/blender/makesdna/DNA_ID.h index e0f2bec6ec9..6262fe0ef77 100644 --- a/source/blender/makesdna/DNA_ID.h +++ b/source/blender/makesdna/DNA_ID.h @@ -33,6 +33,7 @@ struct ID; struct Library; struct PackedFile; struct UniqueName_Map; +struct Depsgraph; /* Runtime display data */ struct DrawData; @@ -401,6 +402,13 @@ typedef struct ID_Runtime_Remap { typedef struct ID_Runtime { ID_Runtime_Remap remap; + /** + * The depsgraph that owns this data block. This is only set on data-blocks which are + * copied-on-eval by the depsgraph. Additional data-blocks created during depsgraph evaluation + * are not owned by any specific depsgraph and thus this pointer is null for those. + */ + struct Depsgraph *depsgraph; + void *_pad; } ID_Runtime; typedef struct ID { diff --git a/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc b/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc index b52c1155cd5..9ce4f93aad5 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc @@ -14,6 +14,8 @@ #include "BKE_collection.hh" #include "BKE_instances.hh" +#include "DEG_depsgraph_query.hh" + #include "node_geometry_util.hh" #include @@ -70,6 +72,13 @@ static void node_geo_exec(GeoNodeExecParams params) params.set_default_remaining_outputs(); return; } + if (!DEG_collection_geometry_is_evaluated(*collection)) { + params.error_message_add(NodeWarningType::Error, + TIP_("Can't access collections geometry because it's not evaluated " + "yet. This can happen when there is a dependency cycle")); + params.set_default_remaining_outputs(); + return; + } const NodeGeometryCollectionInfo &storage = node_storage(params.node()); const bool use_relative_transform = (storage.transform_space == diff --git a/source/blender/nodes/geometry/nodes/node_geo_object_info.cc b/source/blender/nodes/geometry/nodes/node_geo_object_info.cc index 638f9135ecd..4f9696f9625 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_object_info.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_object_info.cc @@ -19,6 +19,8 @@ #include "GEO_transform.hh" +#include "BLT_translation.hh" + #include "node_geometry_util.hh" namespace blender::nodes::node_geo_object_info_cc { @@ -59,6 +61,25 @@ static void node_geo_exec(GeoNodeExecParams params) params.set_default_remaining_outputs(); return; } + if (!DEG_object_geometry_is_evaluated(*object)) { + params.error_message_add(NodeWarningType::Error, + TIP_("Can't access object's geometry because it's not evaluated yet. " + "This can happen when there is a dependency cycle")); + params.set_default_remaining_outputs(); + return; + } + if (transform_space_relative) { + if (!DEG_object_transform_is_evaluated(*object) || + !DEG_object_transform_is_evaluated(*self_object)) + { + params.error_message_add( + NodeWarningType::Error, + TIP_("Can't access objects transforms because it's not evaluated yet. " + "This can happen when there is a dependency cycle")); + params.set_default_remaining_outputs(); + return; + } + } const float4x4 &object_matrix = object->object_to_world(); const float4x4 transform = self_object->world_to_object() * object_matrix;