Make ImplicitSharingPtr constructor from data pointer explicit #115476

Lukas Tönne · 2023-11-27T13:53:05+01:00

Lukas Tönne commented

2023-11-27 13:53:05 +01:00

The ImplicitSharingPtr has an implicit constructor for raw pointers.
This has unintended effects when comparing an ImplicitSharingPtr to a
raw pointer: The raw pointer is implicitly converted to the shared
pointer (without change in refcount) and when going out of scope will
decrement user count, eventually freeing the data.

Conversion from raw pointer to shared pointer should not happen
implicitly. The constructor is made explicit now. This requires a little
more boilerplate when constructing a sharing pointer. A special
constructor for the nullptr is added so comparison with nullptr can
still happen without writing out a constructor.

The ImplicitSharingPtr has an implicit constructor for raw pointers. This has unintended effects when comparing an ImplicitSharingPtr to a raw pointer: The raw pointer is implicitly converted to the shared pointer (without change in refcount) and when going out of scope will decrement user count, eventually freeing the data. Conversion from raw pointer to shared pointer should not happen implicitly. The constructor is made explicit now. This requires a little more boilerplate when constructing a sharing pointer. A special constructor for the nullptr is added so comparison with nullptr can still happen without writing out a constructor.

👍 1

Lukas Tönne added 1 commit 2023-11-27 13:53:21 +01:00

buildbot/vexp-code-patch-coordinator Build done. Details

3e20fac450 Make ImplicitSharingPtr constructor from data pointer explicit.

The ImplicitSharingPtr has an implicit constructor for raw pointers.
This has unintended effects when comparing an ImplicitSharingPtr to a
raw pointer: The raw pointer is implicitly converted to the shared
pointer (without change in refcount) and when going out of scope will
decrement user count, eventually freeing the data.

Conversion from raw pointer to shared pointer should not happen
implicitly. The constructor is made explicit now. This requires a little
more boilerplate when constructing a sharing pointer. A special
constructor for the nullptr is added so comparison with nullptr can
still happen without writing out a constructor.

Lukas Tönne requested review from Jacques Lucke 2023-11-27 13:53:29 +01:00

Lukas Tönne added this to the Nodes & Physics project 2023-11-27 13:53:34 +01:00

Lukas Tönne commented

2023-11-27 13:55:53 +01:00

Possible improvement: add a make_implicit_sharing_ptr utility function similar to std::make_shared, which forwards arguments to one of the pointee constructors. This avoids writing out multiple constructors and keeps code a bit more readable.

Possible improvement: add a `make_implicit_sharing_ptr` utility function similar to `std::make_shared`, which forwards arguments to one of the pointee constructors. This avoids writing out multiple constructors and keeps code a bit more readable.

Lukas Tönne commented

2023-11-27 13:59:01 +01:00

This problem came up during work on volume grids: the Volume geometry has a list of VolumeGrid, which are becoming ImplicitSharedData. Some API methods loop over grids and compare to a raw pointer, which includes a "hidden" implicit conversion during pointer comparison and ends up freeing all the data:
fe90c82b20/source/blender/blenkernel/intern/volume.cc (L1579)

This problem came up during work on volume grids: the `Volume` geometry has a list of `VolumeGrid`, which are becoming `ImplicitSharedData`. Some API methods loop over grids and compare to a raw pointer, which includes a "hidden" implicit conversion during pointer comparison and ends up freeing all the data: https://projects.blender.org/blender/blender/src/commit/fe90c82b208ae5c7f699040a36b149a7f576cd0d/source/blender/blenkernel/intern/volume.cc#L1579

Jacques Lucke approved these changes 2023-11-27 14:56:25 +01:00

Lukas Tönne commented

2023-11-27 15:13:06 +01:00

@blender-bot build

Lukas Tönne merged commit 227a4eae77 into main

2023-11-27 15:53:38 +01:00

Lukas Tönne referenced this issue from a commit

2023-11-27 15:53:39 +01:00

Make ImplicitSharingPtr constructor from data pointer explicit

Lukas Tönne deleted branch implicit-sharing-ptr-ctor

2023-11-27 15:53:40 +01:00

Hans Goudey referenced this issue from a commit