search

ros / geometry2

A set of ROS packages for keeping track of coordinate transforms.
189 stars 273 forks source link

tf2_ros: add virtual destructor to tf2_ros::BufferInterface #560

Open meyerj opened 5 months ago

meyerj commented 5 months ago

Same as https://github.com/ros/geometry2/pull/346, targeting noetic-devel:

Class tf2_ros::BufferInterface did not define a virtual destructor, but has virtual methods. This is dangerous because it may lead to memory leaks if instances are deleted using a pointer to the base class (reference: https://stackoverflow.com/questions/1123044/when-should-your-destructor-be-virtual).

Clang and GCC with -Wnon-virtual-dtor or -Weffc++ warn about this:

include/tf2_ros/buffer_interface.h:48:7: warning: 'tf2_ros::BufferInterface' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]                                                             
class BufferInterface                                                                                                                                                                                                                  
      ^
meyerj commented 5 months ago

I assume this comment still applies:

This looks right, but I believe it will break ABI so we'll need to evaulate whether an ABI change or memory leak is worse. In general since we rebuild everything right now I think the ABI change is likely less of an issue.

So the patch cannot be merged into noetic-devel without breaking the ABI, but should probably have been considered for a new distribution in between. Overall, the severity of the potential memory leak is low, because likely in most applications tf buffers will be created and destroyed once, and not deleted repeatedly via a tf2_ros::BufferInterface pointer. So it may be fine to keep it as-is and close the PR, given that there will be no future ROS 1 release anymore?

By the way, for ROS 2 the same issue has been addressed in https://github.com/ros2/geometry2/commit/5178d48b946bdd60941bc8540eb611076974cd8d.

meyerj commented 2 months ago

What is the policy of breaking the ABI on branch noetic-devel? If ABI breaking changes are not accepted anyway, for a good reason, then maybe I should re-submit this to https://github.com/ros-o/geometry2?