Added static assert asserting custom types have no overloaded operator new

[iv461]

Description

Fixes #2951

Is this user-facing behavior change?

When users publish custom message types using type adapters, they must now publish by unique_ptr if the custom message type overloads operator new. This is to prevent new/delete mismatch.

Did you use Generative AI?

No

Additional Information

Tested, and correctly fails to compile if I try to publish a pcl::PointCloud by value AND I'm pulling the dependency on PCL via pcl_ros.

According to cppreference, there are 22 overloads of operator new. I only implement detecting the class-specific ones 15 and 17. I do not implement the array-operators (16 and 18) since the detection becomes ambiguous and causes compile error.
I do not think it is reasonable to guard against overloaded global operator new/delete, i.e. overloads 1-10.

Also, overloads 11-14 and 19-22 accept arbitrary arguments and are therefore likely to be undetectable.

[fujitatomoya]

although I think this is a right thing to do to avoid undefined behavior by new/delete mismatch when the data object is copied by global allocator, this possibly breaks the application not to be able to compile once this fix is merged. again, i think this is also good to let the user application know the correct approach to publish the data in this particular case. but i would like to have other opinions from other developers.

[fujitatomoya]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[iv461]

Yes, this is undefined behavior, more speficially it might cause heap corruption and with this random crashes.
Therefore, I think it is reasonable to aim to fix all packages that have this new/delete mismatch bug and now would fail to compile.

But you are right, the problem is that there might be many places that need to be fixed, i.e. many packages that would fail to compile.

To fix the compilation, generally it suffices to replace publish(my_message) with publish(std::make_unique<MyMessage>(my_message)). Since rclcpp would copy the message anyway, this also has no performance impact.

Two ideas:

• Bump pcl_ros to use C++17 on rolling to prevent pcl::PointCloud from using custom allocators.
• Make an exception for the pcl::PointCloud type, (i.e. allow it to be published by value) if we can prove that calling the Eigen aligned free function does not cause heap corruption when freeing memory allocated from the regular global new operator. But unfortunately, it looks like it would cause heap corruption.

By the way, I looked at the CI and it failed again due to an apparently unrelated issue in rclcpp_action.

[christophebedard]

Minor suggestion. Please double-check the line length.

Suggested change

	"When publishing by value (i.e. when calling publish(const T& msg)), the published message"
	"type must not have an overloaded operator new. In this case, please use the"
	"When publishing by value (i.e. when calling publish(const T& msg)), the published message "
	"type must not have an overloaded operator new. In this case, please use the "

[iv461]

I've used now ament_uncrustify --reformat --linelength 100 for code formatting, which formatter do you use ?

[alsora]

We discussed this during the client library working group.
Overall we are in favor of this.

If possible it would be better to make it a static warning rather than an assertion, to have a deprecation cycle.
If not possible, we think it would be ok to merge as is.

[iv461]

Thanks for the update. I would also be fine with a deprecation warning instead of a compile error. However, I was unable to to implement a deprecation warning. I tried an approach in df1e782 which generally works, but for some strange reason I can't get it to work inside rclcpp. Therefore, I'll leave it as it is, with the static_assert.