From 21ea4995585931ad54f51c1878c06c526c3355a5 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Tue, 23 Aug 2022 22:56:54 +1000 Subject: Fix T100286: Crash accessing freed depsgraph object instances Invalidate depsgraph.object_instances when freed, this resolves a crash when accessing the object instances after iteration has finished. Unlike most other collections, object_instances is only valid while the iterator is in-memory. The Python/RNA API needs to inline int/string collection lookups so the Python instance can be created before the iterator ends. Reviewed By: mont29, sergey Ref D15755 --- source/blender/makesrna/RNA_access.h | 4 + source/blender/makesrna/intern/rna_access.c | 14 ++++ source/blender/makesrna/intern/rna_depsgraph.c | 109 ++++++++++++++++--------- source/blender/python/intern/bpy_rna.c | 92 ++++++++++++++++++++- 4 files changed, 176 insertions(+), 43 deletions(-) diff --git a/source/blender/makesrna/RNA_access.h b/source/blender/makesrna/RNA_access.h index de9fa60aa5d..ddc010f27a1 100644 --- a/source/blender/makesrna/RNA_access.h +++ b/source/blender/makesrna/RNA_access.h @@ -430,6 +430,10 @@ int RNA_property_collection_lookup_string(PointerRNA *ptr, PointerRNA *r_ptr); int RNA_property_collection_lookup_string_index( PointerRNA *ptr, PropertyRNA *prop, const char *key, PointerRNA *r_ptr, int *r_index); + +bool RNA_property_collection_lookup_int_has_fn(PropertyRNA *prop); +bool RNA_property_collection_lookup_string_has_fn(PropertyRNA *prop); + /** * Zero return is an assignment error. */ diff --git a/source/blender/makesrna/intern/rna_access.c b/source/blender/makesrna/intern/rna_access.c index 04707c01d6b..56cbcb2a7f2 100644 --- a/source/blender/makesrna/intern/rna_access.c +++ b/source/blender/makesrna/intern/rna_access.c @@ -4080,6 +4080,20 @@ int RNA_property_collection_lookup_index(PointerRNA *ptr, return -1; } +bool RNA_property_collection_lookup_int_has_fn(PropertyRNA *prop) +{ + BLI_assert(RNA_property_type(prop) == PROP_COLLECTION); + CollectionPropertyRNA *cprop = (CollectionPropertyRNA *)rna_ensure_property(prop); + return cprop->lookupint != NULL; +} + +bool RNA_property_collection_lookup_string_has_fn(PropertyRNA *prop) +{ + BLI_assert(RNA_property_type(prop) == PROP_COLLECTION); + CollectionPropertyRNA *cprop = (CollectionPropertyRNA *)rna_ensure_property(prop); + return cprop->lookupstring != NULL; +} + int RNA_property_collection_lookup_int(PointerRNA *ptr, PropertyRNA *prop, int key, diff --git a/source/blender/makesrna/intern/rna_depsgraph.c b/source/blender/makesrna/intern/rna_depsgraph.c index 6196f8d1ca0..f0d26362cad 100644 --- a/source/blender/makesrna/intern/rna_depsgraph.c +++ b/source/blender/makesrna/intern/rna_depsgraph.c @@ -43,23 +43,41 @@ /* **************** Object Instance **************** */ +typedef struct RNA_DepsgraphIterator { + BLI_Iterator iter; +# ifdef WITH_PYTHON + /** + * Store the Python instance so the #BPy_StructRNA can be set as invalid iteration is completed. + * Otherwise accessing from Python (console auto-complete for e.g.) crashes, see: T100286. */ + void *py_instance; +# endif +} RNA_DepsgraphIterator; + +# ifdef WITH_PYTHON +void **rna_DepsgraphIterator_instance(PointerRNA *ptr) +{ + RNA_DepsgraphIterator *di = ptr->data; + return &di->py_instance; +} +# endif + static PointerRNA rna_DepsgraphObjectInstance_object_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - return rna_pointer_inherit_refine(ptr, &RNA_Object, iterator->current); + RNA_DepsgraphIterator *di = ptr->data; + return rna_pointer_inherit_refine(ptr, &RNA_Object, di->iter.current); } static int rna_DepsgraphObjectInstance_is_instance_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; return (deg_iter->dupli_object_current != NULL); } static PointerRNA rna_DepsgraphObjectInstance_instance_object_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; Object *instance_object = NULL; if (deg_iter->dupli_object_current != NULL) { instance_object = deg_iter->dupli_object_current->ob; @@ -69,24 +87,24 @@ static PointerRNA rna_DepsgraphObjectInstance_instance_object_get(PointerRNA *pt static bool rna_DepsgraphObjectInstance_show_self_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; - int ob_visibility = BKE_object_visibility(iterator->current, deg_iter->eval_mode); + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; + int ob_visibility = BKE_object_visibility(di->iter.current, deg_iter->eval_mode); return (ob_visibility & OB_VISIBLE_SELF) != 0; } static bool rna_DepsgraphObjectInstance_show_particles_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; - int ob_visibility = BKE_object_visibility(iterator->current, deg_iter->eval_mode); + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; + int ob_visibility = BKE_object_visibility(di->iter.current, deg_iter->eval_mode); return (ob_visibility & OB_VISIBLE_PARTICLES) != 0; } static PointerRNA rna_DepsgraphObjectInstance_parent_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; Object *dupli_parent = NULL; if (deg_iter->dupli_object_current != NULL) { dupli_parent = deg_iter->dupli_parent; @@ -96,8 +114,8 @@ static PointerRNA rna_DepsgraphObjectInstance_parent_get(PointerRNA *ptr) static PointerRNA rna_DepsgraphObjectInstance_particle_system_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; struct ParticleSystem *particle_system = NULL; if (deg_iter->dupli_object_current != NULL) { particle_system = deg_iter->dupli_object_current->particle_system; @@ -107,8 +125,8 @@ static PointerRNA rna_DepsgraphObjectInstance_particle_system_get(PointerRNA *pt static void rna_DepsgraphObjectInstance_persistent_id_get(PointerRNA *ptr, int *persistent_id) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; if (deg_iter->dupli_object_current != NULL) { memcpy(persistent_id, deg_iter->dupli_object_current->persistent_id, @@ -121,8 +139,8 @@ static void rna_DepsgraphObjectInstance_persistent_id_get(PointerRNA *ptr, int * static unsigned int rna_DepsgraphObjectInstance_random_id_get(PointerRNA *ptr) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; if (deg_iter->dupli_object_current != NULL) { return deg_iter->dupli_object_current->random_id; } @@ -133,23 +151,23 @@ static unsigned int rna_DepsgraphObjectInstance_random_id_get(PointerRNA *ptr) static void rna_DepsgraphObjectInstance_matrix_world_get(PointerRNA *ptr, float *mat) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; if (deg_iter->dupli_object_current != NULL) { copy_m4_m4((float(*)[4])mat, deg_iter->dupli_object_current->mat); } else { /* We can return actual object's matrix here, no reason to return identity matrix * when this is not actually an instance... */ - Object *ob = (Object *)iterator->current; + Object *ob = (Object *)di->iter.current; copy_m4_m4((float(*)[4])mat, ob->obmat); } } static void rna_DepsgraphObjectInstance_orco_get(PointerRNA *ptr, float *orco) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; if (deg_iter->dupli_object_current != NULL) { copy_v3_v3(orco, deg_iter->dupli_object_current->orco); } @@ -160,8 +178,8 @@ static void rna_DepsgraphObjectInstance_orco_get(PointerRNA *ptr, float *orco) static void rna_DepsgraphObjectInstance_uv_get(PointerRNA *ptr, float *uv) { - BLI_Iterator *iterator = ptr->data; - DEGObjectIterData *deg_iter = (DEGObjectIterData *)iterator->data; + RNA_DepsgraphIterator *di = ptr->data; + DEGObjectIterData *deg_iter = (DEGObjectIterData *)di->iter.data; if (deg_iter->dupli_object_current != NULL) { copy_v2_v2(uv, deg_iter->dupli_object_current->uv); } @@ -321,7 +339,7 @@ static PointerRNA rna_Depsgraph_objects_get(CollectionPropertyIterator *iter) * so that previous one remains valid memory for python to access to. Yuck. */ typedef struct RNA_Depsgraph_Instances_Iterator { - BLI_Iterator iterators[2]; + RNA_DepsgraphIterator iterators[2]; DEGObjectIterData deg_data[2]; DupliObject dupli_object_current[2]; int counter; @@ -337,9 +355,9 @@ static void rna_Depsgraph_object_instances_begin(CollectionPropertyIterator *ite data->flag = DEG_ITER_OBJECT_FLAG_LINKED_DIRECTLY | DEG_ITER_OBJECT_FLAG_LINKED_VIA_SET | DEG_ITER_OBJECT_FLAG_VISIBLE | DEG_ITER_OBJECT_FLAG_DUPLI; - di_it->iterators[0].valid = true; - DEG_iterator_objects_begin(&di_it->iterators[0], data); - iter->valid = di_it->iterators[0].valid; + di_it->iterators[0].iter.valid = true; + DEG_iterator_objects_begin(&di_it->iterators[0].iter, data); + iter->valid = di_it->iterators[0].iter.valid; } static void rna_Depsgraph_object_instances_next(CollectionPropertyIterator *iter) @@ -348,12 +366,12 @@ static void rna_Depsgraph_object_instances_next(CollectionPropertyIterator *iter iter->internal.custom; /* We need to copy current iterator status to next one being worked on. */ - di_it->iterators[(di_it->counter + 1) % 2] = di_it->iterators[di_it->counter % 2]; + di_it->iterators[(di_it->counter + 1) % 2].iter = di_it->iterators[di_it->counter % 2].iter; di_it->deg_data[(di_it->counter + 1) % 2] = di_it->deg_data[di_it->counter % 2]; di_it->counter++; - di_it->iterators[di_it->counter % 2].data = &di_it->deg_data[di_it->counter % 2]; - DEG_iterator_objects_next(&di_it->iterators[di_it->counter % 2]); + di_it->iterators[di_it->counter % 2].iter.data = &di_it->deg_data[di_it->counter % 2]; + DEG_iterator_objects_next(&di_it->iterators[di_it->counter % 2].iter); /* Dupli_object_current is also temp memory generated during the iterations, * it may be freed when last item has been iterated, * so we have same issue as with the iterator itself: @@ -365,15 +383,24 @@ static void rna_Depsgraph_object_instances_next(CollectionPropertyIterator *iter di_it->deg_data[di_it->counter % 2].dupli_object_current = &di_it->dupli_object_current[di_it->counter % 2]; } - iter->valid = di_it->iterators[di_it->counter % 2].valid; + iter->valid = di_it->iterators[di_it->counter % 2].iter.valid; } static void rna_Depsgraph_object_instances_end(CollectionPropertyIterator *iter) { RNA_Depsgraph_Instances_Iterator *di_it = (RNA_Depsgraph_Instances_Iterator *) iter->internal.custom; - DEG_iterator_objects_end(&di_it->iterators[0]); - DEG_iterator_objects_end(&di_it->iterators[1]); + for (int i = 0; i < ARRAY_SIZE(di_it->iterators); i++) { + RNA_DepsgraphIterator *di = &di_it->iterators[i]; + DEG_iterator_objects_end(&di->iter); + +# ifdef WITH_PYTHON + if (di->py_instance) { + BPY_DECREF_RNA_INVALIDATE(di->py_instance); + } +# endif + } + MEM_freeN(di_it); } @@ -381,8 +408,8 @@ static PointerRNA rna_Depsgraph_object_instances_get(CollectionPropertyIterator { RNA_Depsgraph_Instances_Iterator *di_it = (RNA_Depsgraph_Instances_Iterator *) iter->internal.custom; - BLI_Iterator *iterator = &di_it->iterators[di_it->counter % 2]; - return rna_pointer_inherit_refine(&iter->parent, &RNA_DepsgraphObjectInstance, iterator); + RNA_DepsgraphIterator *di = &di_it->iterators[di_it->counter % 2]; + return rna_pointer_inherit_refine(&iter->parent, &RNA_DepsgraphObjectInstance, di); } /* Iteration over evaluated IDs */ @@ -498,6 +525,10 @@ static void rna_def_depsgraph_instance(BlenderRNA *brna) "Extended information about dependency graph object iterator " "(Warning: All data here is 'evaluated' one, not original .blend IDs)"); +# ifdef WITH_PYTHON + RNA_def_struct_register_funcs(srna, NULL, NULL, "rna_DepsgraphIterator_instance"); +# endif + prop = RNA_def_property(srna, "object", PROP_POINTER, PROP_NONE); RNA_def_property_struct_type(prop, "Object"); RNA_def_property_ui_text(prop, "Object", "Evaluated object the iterator points to"); diff --git a/source/blender/python/intern/bpy_rna.c b/source/blender/python/intern/bpy_rna.c index d9c004fb6fa..b22c5e60eb3 100644 --- a/source/blender/python/intern/bpy_rna.c +++ b/source/blender/python/intern/bpy_rna.c @@ -2213,6 +2213,26 @@ static int pyrna_prop_collection_bool(BPy_PropertyRNA *self) } \ (void)0 +/** + * \param result: The result of calling a subscription operation on a collection (never NULL). + */ +static int pyrna_prop_collection_subscript_is_valid_or_error(const PyObject *value) +{ + if (value != Py_None) { + BLI_assert(BPy_StructRNA_Check(value)); + const BPy_StructRNA *value_pyrna = (const BPy_StructRNA *)value; + if (UNLIKELY(value_pyrna->ptr.type == NULL)) { + /* It's important to use a `TypeError` as that is what's returned when `__getitem__` is + * called on an object that doesn't support item access. */ + PyErr_Format(PyExc_TypeError, + "'%.200s' object is not subscriptable (only iteration is supported)", + Py_TYPE(value)->tp_name); + return -1; + } + } + return 0; +} + /* Internal use only. */ static PyObject *pyrna_prop_collection_subscript_int(BPy_PropertyRNA *self, Py_ssize_t keynum) { @@ -2223,8 +2243,35 @@ static PyObject *pyrna_prop_collection_subscript_int(BPy_PropertyRNA *self, Py_s PYRNA_PROP_COLLECTION_ABS_INDEX(NULL); - if (RNA_property_collection_lookup_int(&self->ptr, self->prop, keynum_abs, &newptr)) { - return pyrna_struct_CreatePyObject(&newptr); + if (RNA_property_collection_lookup_int_has_fn(self->prop)) { + if (RNA_property_collection_lookup_int(&self->ptr, self->prop, keynum_abs, &newptr)) { + return pyrna_struct_CreatePyObject(&newptr); + } + } + else { + /* No callback defined, just iterate and find the nth item. */ + const int key = (int)keynum; + PyObject *result = NULL; + bool found = false; + CollectionPropertyIterator iter; + RNA_property_collection_begin(&self->ptr, self->prop, &iter); + for (int i = 0; iter.valid; RNA_property_collection_next(&iter), i++) { + if (i == key) { + result = pyrna_struct_CreatePyObject(&iter.ptr); + found = true; + break; + } + } + /* It's important to end the iterator after `result` has been created + * so iterators may optionally invalidate items that were iterated over, see: T100286. */ + RNA_property_collection_end(&iter); + if (found) { + if (result && (pyrna_prop_collection_subscript_is_valid_or_error(result) == -1)) { + Py_DECREF(result); + result = NULL; /* The exception has been set. */ + } + return result; + } } const int len = RNA_property_collection_length(&self->ptr, self->prop); @@ -2306,8 +2353,45 @@ static PyObject *pyrna_prop_collection_subscript_str(BPy_PropertyRNA *self, cons PYRNA_PROP_CHECK_OBJ(self); - if (RNA_property_collection_lookup_string(&self->ptr, self->prop, keyname, &newptr)) { - return pyrna_struct_CreatePyObject(&newptr); + if (RNA_property_collection_lookup_string_has_fn(self->prop)) { + if (RNA_property_collection_lookup_string(&self->ptr, self->prop, keyname, &newptr)) { + return pyrna_struct_CreatePyObject(&newptr); + } + } + else { + /* No callback defined, just iterate and find the nth item. */ + const int keylen = strlen(keyname); + char name[256]; + int namelen; + PyObject *result = NULL; + bool found = false; + CollectionPropertyIterator iter; + RNA_property_collection_begin(&self->ptr, self->prop, &iter); + for (int i = 0; iter.valid; RNA_property_collection_next(&iter), i++) { + PropertyRNA *nameprop = RNA_struct_name_property(iter.ptr.type); + char *nameptr = RNA_property_string_get_alloc( + &iter.ptr, nameprop, name, sizeof(name), &namelen); + if ((keylen == namelen) && STREQ(nameptr, keyname)) { + found = true; + } + if ((char *)&name != nameptr) { + MEM_freeN(nameptr); + } + if (found) { + result = pyrna_struct_CreatePyObject(&iter.ptr); + break; + } + } + /* It's important to end the iterator after `result` has been created + * so iterators may optionally invalidate items that were iterated over, see: T100286. */ + RNA_property_collection_end(&iter); + if (found) { + if (result && (pyrna_prop_collection_subscript_is_valid_or_error(result) == -1)) { + Py_DECREF(result); + result = NULL; /* The exception has been set. */ + } + return result; + } } PyErr_Format(PyExc_KeyError, "bpy_prop_collection[key]: key \"%.200s\" not found", keyname); -- cgit v1.2.3