Page MenuHome

Fix T74888: Operators called within spline loops invalidate PointerRNA pointer
Needs ReviewPublic

Authored by Germano Cavalcante (mano-wii) on Fri, Mar 20, 1:12 AM.

Details

Summary

Usually Curve operators calls ED_curve_editnurb_load and ED_curve_editnurb_make which creates another cu->nurb and free the old one.
But the old cu->nurb may be being referenced by a PyObject (type pyrna_prop_collection_Type with pointer inited in rna_Curve_splines_begin).
This can lead to crash or other nasty problems since RNA_POINTER_INVALIDATE(nu_ptr) is not called.

The idea of this patch is not to free the cu->nurb and editnurb->nurbs pointers.

How this patch solves the problem:
Although not used, the BKE_nurbList_duplicate logic is duplicated in ED_curve_editnurb_load and ED_curve_editnurb_make (with minor differences).
Basically these functions copy the ListBase of nurbs.
But to copy, the old nurb is always freed and a new one is allocated.
This patch changes the behavior of BKE_nurbList_duplicate to reuse pointers, and deduplicates the code in` ED_curve_editnurb_load` and ED_curve_editnurb_make by using that BKE function.

Ref T74888

Diff Detail

Repository
rB Blender
Branch
master
Build Status
Buildable 7238
Build 7238: arc lint + arc unit

Event Timeline

source/blender/blenkernel/intern/curve.c
494–498

General note: this is too vague of a name. What exactly is happening in this function? When is it to be used? Name should be helping in such questions, and also worth adding comment.

source/blender/editors/curve/editcurve.c
1312

Are there cases where this branch is executed and there was something else between previous editnub_load and this editnurb_make?

source/blender/blenkernel/intern/curve.c
494–498

This is a name that I have a hard time defining.
The idea is to reuse what is already allocated (when possible).
If there is no space, the member must be reallocated.
My ideas for names:
BKE_nurb_compare_and_update (remembers COW)
BKE_nurb_realloc_and_copy_data

source/blender/editors/curve/editcurve.c
1312

Apparently not.
This function is either called when there is no editnurb or it is called right after ED_curve_editnurb_load which makes this copy redundant (since ED_curve_editnurb_load copies from cu->nurb from editnurb->nurbs and this does the opposite).

In a general case is hard to keep iterators always valid. But here i think it's quite isolated and clear fix, and covers something which people are running into quite often.

So from me once the naming is figured out this is a nice fix to have.

source/blender/blenkernel/intern/curve.c
494–498

Doesn't seem that it compares anything than number of points and such. As in, it dones't compare coordinates.

Think best is to make it non-public function and be something like:

/* Copy content of nu_src to nu_dst without doing any unnecessary (re)allocations.
 * The destination container itself is expected to be allocated and is never touched here.
 * The points arrays and other fields will only be re-allocated if their size does not match.
 */
static void nurbs_copy_inplace(const Nurb *nu_src, Nurb *nu_dst)
{
}

The reason to make it non-public is because it's quite obscure function, and there is no really need for it to be exposed.

Germano Cavalcante (mano-wii) marked 3 inline comments as done.
  • Rename and comment BKE_nurb_update
  • make nurbs_copy_inplace a non-public and edit BKE_nurbList_duplicate instead
  • Use BKE_nurbList_duplicate to avoid free a nurbList

Am not really keen on this fix, mainly because it's limited to curves, the same problems remain for meshes.

In general, if we try to keep arrays valid between mode switching, it's going to complicate code and not work reliably - since in some cases the arrays size wont match.

Since so few operators flush edits back to the object data, it would be interesting to see if we could support adding hooks without writing edit-changes back to the original object-data, perhaps use the edit-data in place until edit-mode exists.

In general, if we try to keep arrays valid between mode switching, it's going to complicate code and not work reliably - since in some cases the arrays size wont match.

In general - yes. In a specific case it is possible to have it work reliably quite easily.
Sure, the limitation is that iteration need to happen through edit mode curves, otherwise you are in trouble.

Since so few operators flush edits back to the object data, it would be interesting to see if we could support adding hooks without writing edit-changes back to the original object-data, perhaps use the edit-data in place until edit-mode exists.

I'm not sure how would that work. Have separate vertex indices in hook modifier for edit mode? Change meaning of indices depending on object mode?
How to support rendered viewport next to edit mode one? How to support hook modifiers after constructive modifier?

In the original bug (reported), the problem is concentrated in the LinkList pointer.
So just reusing LinkList links would be enough to solve the problem.
Reusing the arrays is a bonus (I confess I should have done it separately).
On arrays, BMeshs solve this problem by maintaining a PyObject reference as a CustomData.
We could do something like that for curves but that would be a big change.

EDIT
Okay, after a second test, I realized that to fix the bug it is important to keep the reference of the arrays as well. (In addition to LinkList).