WIP : Use an extra step of indirection in accessing CustomDataLayers via rna. #108319

Martijn Versteegh · 2023-05-26T16:25:11+02:00

Martijn Versteegh commented

2023-05-26 16:25:11 +02:00

The RNA CustomDataLayer api stores pointers to CustomDataLayer structs in the
PointerRNA, but when CustomData layers are added/removed those get reordered.
This leads to pythons MeshUVLoopLayer referencing the wrong layer. Potentially
even pointing into freed memory.

see #107500

This patch is a 'proof of concept' and is unfinished in a few ways:

Only MeshUVLoopLayers are handled, other CustomDataLayer accesses
(attributes, vertex_create, vertex_color etc etc) are still broken, possibly
more broken.
The indirection table used is currently hardcoded to 64 entries. This will
crash & burn whenever there are more than 64 simultaneous customdata layers.
the MeshUVLoopLayer.remove function is not implemented.

Possible ways to move forward:

Don't use an indirection table, use a unique id on every CustomDataLayer
and do a search for every access. This is slower, but the speeds maybe not
too important for the layer lookups, as long as the element access still is
fast.
Store the indirection table in a (to be created) C++ array-like type without
reallocation.
Ignore this problem for 3.6 and for 4.0 redesign attribute/customdata storage
in a way that the layer(-header)s don't move around.

The RNA CustomDataLayer api stores pointers to CustomDataLayer structs in the PointerRNA, but when CustomData layers are added/removed those get reordered. This leads to pythons MeshUVLoopLayer referencing the wrong layer. Potentially even pointing into freed memory. see https://projects.blender.org/blender/blender/issues/107500 This patch is a 'proof of concept' and is unfinished in a few ways: - Only MeshUVLoopLayers are handled, other CustomDataLayer accesses (attributes, vertex_create, vertex_color etc etc) are still broken, possibly *more* broken. - The indirection table used is currently hardcoded to 64 entries. This will crash & burn whenever there are more than 64 simultaneous customdata layers. - the MeshUVLoopLayer.remove function is not implemented. Possible ways to move forward: - Don't use an indirection table, use a unique id on every CustomDataLayer and do a search for every access. This is slower, but the speeds maybe not too important for the layer lookups, as long as the element access still is fast. - Store the indirection table in a (to be created) C++ array-like type without reallocation. - Ignore this problem for 3.6 and for 4.0 redesign attribute/customdata storage in a way that the layer(-header)s don't move around.

Martijn Versteegh added 1 commit 2023-05-26 16:25:22 +02:00

b7e2b37d25 WIP : Use an extra step of indirection in accessing CustomDataLayers via rna.

The RNA CustomDataLayer api stores pointers to CustomDataLayer structs in the
PointerRNA, but when CustomData layers are added/removed those get reordered.
This leads to pythons MeshUVLoopLayer referencing the wrong layer. Potentially
even pointing into freed memory.

see #107500

This patch is a 'proof of concept' and is unfinished in a few ways:
- Only MeshUVLoopLayers are handled, other CustomDataLayer accesses
(attributes,  vertex_create, vertex_color etc etc) are still broken, possibly
*more* broken.
- The indirection table used is currently hardcoded to 64 entries. This will
crash & burn whenever there are more than 64 simultaneous customdata layers.

Possible ways to move forward:
- Don't use an indirection table, use a unique id on every CustomDataLayer
and do a search for every access. This is slower, but the speeds maybe not
too important for the layer lookups, as long as the element access still is
fast.
- Store the indirection table in a (to be created) C++ array-like type without
 reallocation.
- Ignore this problem for 3.6 and for 4.0 redesign attribute/customdata storage
in a way that the layer(-header)s don't move around.

Martijn Versteegh added 2 commits 2023-05-26 16:45:49 +02:00

3b94c39085 Remove extraneous initialisation of indirection table.

2ad8de628b Mark indirection table entries as unused on layer remove.

Martijn Versteegh commented

2023-06-01 13:52:18 +02:00

I did some work trying to remove the CustomDataLayer ** table and replace it with a layer_uuid. But got stuck on the fact that I can store the uuid in the PointerRNA.data pointer (just by hackishly casting the int to void *) , but then when I need to get to the actual CustomDataLayer I have no way to find the correct CustomData the layer is in (ldata/vdata/pdata etc) .

This would mean I'd need the uuid to be unique over all CustomData repositories on the mesh and scan over all CustomData domains existing on the mesh to find the layer. Which seems slow and error-prone, especially as I'm not sure if there are ever layers created from multiple threads. I think GN paralellizes stuff over different domains.

I could maybe pack the domain type and the uuid together into an intptr_t (urg) but that seems very ugly as well...

I did some work trying to remove the CustomDataLayer ** table and replace it with a layer_uuid. But got stuck on the fact that I can store the uuid in the PointerRNA.data pointer (just by hackishly casting the int to void *) , but then when I need to get to the actual CustomDataLayer I have no way to find the correct CustomData the layer is in (ldata/vdata/pdata etc) . This would mean I'd need the uuid to be unique over *all* CustomData repositories on the mesh and scan over *all* CustomData domains existing on the mesh to find the layer. Which seems slow and error-prone, especially as I'm not sure if there are ever layers created from multiple threads. I *think* GN paralellizes stuff over different domains. I could maybe pack the domain type and the uuid together into an intptr_t (urg) but that seems very ugly as well...

Iliya Katushenock added this to the Modeling project 2023-06-01 14:17:36 +02:00

Martijn Versteegh commented

2023-06-14 16:15:49 +02:00

I'm currently at the stage where I'm trying to get it to work with the extra lookup table, however I'm not sure how feasible it is either, since the CustomData struct is copied (verbatim) in rather a lot of places so it's very hard to keep track of the ownership of the table.

This pull request has changes conflicting with the target branch.

source/blender/blenkernel/intern/customdata.cc
source/blender/makesdna/DNA_customdata_types.h
source/blender/makesrna/intern/rna_mesh.c
source/blender/makesrna/intern/rna_mesh_utils.h

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u fix_dangling_customdata_pointers:Baardaap-fix_dangling_customdata_pointers

git checkout Baardaap-fix_dangling_customdata_pointers

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP : Use an extra step of indirection in accessing CustomDataLayers via rna. #108319

Checkout