Fix #146221: Concurrent access to IDPropertyGroup in liboverride code.
Crash seems to happen due to parallelized access of data in liboverride
diffing code (presumably when a same linked data is used as reference
for several local liboverrides?).
Since there is zero reason to actually create IDGroup properties there
(it's actually fairly bad, also adding useless overhead and trash data
in system IDProperties), add a new API to access a Pointer PropertyRNA,
`RNA_property_pointer_get_never_create`, which ensures that the call
never creates data, and simply returns `PointerRNA_NULL` instead.
Also reverts abd683fcb5 and re-enable liboverride unittests.
NOTE: This only addresses the case from the report, the current behavior
of `RNA_property_pointer_get` is simply wrong and needs to be rethought.
This is not a simple change though most likely. See also #147072.
Pull Request: https://projects.blender.org/blender/blender/pulls/146990
This commit is contained in:
committed by
Bastien Montagne
parent
8edf746621
commit
ada45519a7
@@ -569,7 +569,20 @@ int RNA_property_enum_get_default(PointerRNA *ptr, PropertyRNA *prop);
|
|||||||
int RNA_property_enum_step(
|
int RNA_property_enum_step(
|
||||||
const bContext *C, PointerRNA *ptr, PropertyRNA *prop, int from_value, int step);
|
const bContext *C, PointerRNA *ptr, PropertyRNA *prop, int from_value, int step);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* WARNING: _may_ create data in IDPGroup backend storage case.
|
||||||
|
* While creation of data itself is mutex-protected, potential concurrent _accesses_ to the same
|
||||||
|
* property are not, so threaded calls to #RNA_property_pointer_get() remain highly unsafe.
|
||||||
|
*/
|
||||||
PointerRNA RNA_property_pointer_get(PointerRNA *ptr, PropertyRNA *prop) ATTR_NONNULL(1, 2);
|
PointerRNA RNA_property_pointer_get(PointerRNA *ptr, PropertyRNA *prop) ATTR_NONNULL(1, 2);
|
||||||
|
/**
|
||||||
|
* Same as above, but never creates an empty IDPGroup property for Pointer runtime properties that
|
||||||
|
* are not set yet.
|
||||||
|
*
|
||||||
|
* Ideally this should never be done ever, as it is intrisically not threadsafe, but for the time
|
||||||
|
* being at least provide a way to avoid this bad behavior. */
|
||||||
|
PointerRNA RNA_property_pointer_get_never_create(PointerRNA *ptr, PropertyRNA *prop)
|
||||||
|
ATTR_NONNULL(1, 2);
|
||||||
void RNA_property_pointer_set(PointerRNA *ptr,
|
void RNA_property_pointer_set(PointerRNA *ptr,
|
||||||
PropertyRNA *prop,
|
PropertyRNA *prop,
|
||||||
PointerRNA ptr_value,
|
PointerRNA ptr_value,
|
||||||
|
|||||||
@@ -4434,7 +4434,7 @@ int RNA_property_enum_step(
|
|||||||
return result_value;
|
return result_value;
|
||||||
}
|
}
|
||||||
|
|
||||||
PointerRNA RNA_property_pointer_get(PointerRNA *ptr, PropertyRNA *prop)
|
static PointerRNA property_pointer_get(PointerRNA *ptr, PropertyRNA *prop, const bool do_create)
|
||||||
{
|
{
|
||||||
PointerPropertyRNA *pprop = (PointerPropertyRNA *)prop;
|
PointerPropertyRNA *pprop = (PointerPropertyRNA *)prop;
|
||||||
IDProperty *idprop;
|
IDProperty *idprop;
|
||||||
@@ -4460,7 +4460,7 @@ PointerRNA RNA_property_pointer_get(PointerRNA *ptr, PropertyRNA *prop)
|
|||||||
if (pprop->get) {
|
if (pprop->get) {
|
||||||
return pprop->get(ptr);
|
return pprop->get(ptr);
|
||||||
}
|
}
|
||||||
if (prop->flag & PROP_IDPROPERTY) {
|
if (prop->flag & PROP_IDPROPERTY && do_create) {
|
||||||
/* NOTE: While creating/writing data in an accessor is really bad design-wise, this is
|
/* NOTE: While creating/writing data in an accessor is really bad design-wise, this is
|
||||||
* currently very difficult to avoid in that case. So a global mutex is used to keep ensuring
|
* currently very difficult to avoid in that case. So a global mutex is used to keep ensuring
|
||||||
* thread safety. */
|
* thread safety. */
|
||||||
@@ -4473,6 +4473,16 @@ PointerRNA RNA_property_pointer_get(PointerRNA *ptr, PropertyRNA *prop)
|
|||||||
return PointerRNA_NULL;
|
return PointerRNA_NULL;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
PointerRNA RNA_property_pointer_get(PointerRNA *ptr, PropertyRNA *prop)
|
||||||
|
{
|
||||||
|
return property_pointer_get(ptr, prop, true);
|
||||||
|
}
|
||||||
|
|
||||||
|
PointerRNA RNA_property_pointer_get_never_create(PointerRNA *ptr, PropertyRNA *prop)
|
||||||
|
{
|
||||||
|
return property_pointer_get(ptr, prop, false);
|
||||||
|
}
|
||||||
|
|
||||||
void RNA_property_pointer_set(PointerRNA *ptr,
|
void RNA_property_pointer_set(PointerRNA *ptr,
|
||||||
PropertyRNA *prop,
|
PropertyRNA *prop,
|
||||||
PointerRNA ptr_value,
|
PointerRNA ptr_value,
|
||||||
|
|||||||
@@ -2197,8 +2197,11 @@ void rna_property_override_diff_default(Main *bmain, RNAPropertyOverrideDiffCont
|
|||||||
RNACompareOverrideDiffPropPtrContext ptrdiff_ctx(rnadiff_ctx);
|
RNACompareOverrideDiffPropPtrContext ptrdiff_ctx(rnadiff_ctx);
|
||||||
ptrdiff_ctx.owner_id_a = ptr_a->owner_id;
|
ptrdiff_ctx.owner_id_a = ptr_a->owner_id;
|
||||||
ptrdiff_ctx.owner_id_b = ptr_b->owner_id;
|
ptrdiff_ctx.owner_id_b = ptr_b->owner_id;
|
||||||
ptrdiff_ctx.propptr_a = RNA_property_pointer_get(ptr_a, rawprop_a);
|
/* Do not create empty IDPGroup properties here. This is not only a totally useless
|
||||||
ptrdiff_ctx.propptr_b = RNA_property_pointer_get(ptr_b, rawprop_b);
|
* overhead, but it also remains unsafe in parallelized context, despite the usage of a
|
||||||
|
* mutex in #property_pointer_get(). See also #146221. */
|
||||||
|
ptrdiff_ctx.propptr_a = RNA_property_pointer_get_never_create(ptr_a, rawprop_a);
|
||||||
|
ptrdiff_ctx.propptr_b = RNA_property_pointer_get_never_create(ptr_b, rawprop_b);
|
||||||
ptrdiff_ctx.no_prop_name = true;
|
ptrdiff_ctx.no_prop_name = true;
|
||||||
ptrdiff_ctx.no_ownership = no_ownership;
|
ptrdiff_ctx.no_ownership = no_ownership;
|
||||||
ptrdiff_ctx.property_type = PROP_POINTER;
|
ptrdiff_ctx.property_type = PROP_POINTER;
|
||||||
|
|||||||
@@ -348,16 +348,12 @@ if(TEST_SRC_DIR_EXISTS)
|
|||||||
--output-dir ${TEST_OUT_DIR}/blendfile_io/
|
--output-dir ${TEST_OUT_DIR}/blendfile_io/
|
||||||
)
|
)
|
||||||
|
|
||||||
# This test is intermittently failing. Disable it for now to avoid other work
|
add_blender_test(
|
||||||
# being incorrectly flagged as broken. Once issue #146221 is resolved this
|
blendfile_library_overrides
|
||||||
# test must be re-enabled.
|
--python ${CMAKE_CURRENT_LIST_DIR}/bl_blendfile_library_overrides.py --
|
||||||
#
|
--output-dir ${TEST_OUT_DIR}/blendfile_io/
|
||||||
# add_blender_test(
|
--test-dir "${TEST_SRC_DIR}/libraries_and_linking"
|
||||||
# blendfile_library_overrides
|
)
|
||||||
# --python ${CMAKE_CURRENT_LIST_DIR}/bl_blendfile_library_overrides.py --
|
|
||||||
# --output-dir ${TEST_OUT_DIR}/blendfile_io/
|
|
||||||
# --test-dir "${TEST_SRC_DIR}/libraries_and_linking"
|
|
||||||
# )
|
|
||||||
|
|
||||||
add_blender_test(
|
add_blender_test(
|
||||||
blendfile_header
|
blendfile_header
|
||||||
|
|||||||
Reference in New Issue
Block a user