Incorrect use of the delete expression in MetaPointCloud.cu generates memory leaks

[AlejoDiaz49]

Hello

I was making some memory leak checks of my code using Valgrind and I tracked some bugs in the library. In this line on MetaPointCoud you assign the array like this

m_point_clouds_local->cloud_sizes = new uint32_t[m_num_clouds];

But in the destruct() method, when you delete it, you are only deleting the first one of them but not the rest

delete (m_point_clouds_local->cloud_sizes);

Instead of that, it should be like:

delete[] (m_point_clouds_local->cloud_sizes);

The problem is also with m_point_clouds_local->clouds_base_addresses and maybe there are more things like this in the file but i didn't check it completly. Here I put a part of the Valgrind result. The lines that appear here wont mach what I wrote before, because right now I am using the preovius commit of this library. The problem is the same anyway

Warning: set address range perms: large range [0x39600000, 0x53200000) (noaccess)
==546== Mismatched free() / delete / delete []
==546==    at 0x4C2F24B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==546==    by 0x698E31B: gpu_voxels::MetaPointCloud::destruct() (MetaPointCloud.cu:399)
==546==    by 0x698F34D: gpu_voxels::MetaPointCloud::~MetaPointCloud() (MetaPointCloud.cu:415)
==546==    by 0x9F39355: gpu_voxels::robot::KinematicChain::~KinematicChain() (KinematicChain.cu:86)
==546==    by 0x6BAF6E1: checked_delete<gpu_voxels::robot::KinematicChain> (checked_delete.hpp:34)
==546==    by 0x6BAF6E1: boost::detail::sp_counted_impl_p<gpu_voxels::robot::KinematicChain>::dispose() (sp_counted_impl.hpp:78)
==546==    by 0x6BB1391: release (sp_counted_base_gcc_x86.hpp:146)
==546==    by 0x6BB1391: ~shared_count (shared_count.hpp:473)
==546==    by 0x6BB1391: ~shared_ptr (shared_ptr.hpp:336)
==546==    by 0x6BB1391: ~pair (stl_pair.h:96)
==546==    by 0x6BB1391: destroy<std::pair<const std::__cxx11::basic_string<char>, boost::shared_ptr<gpu_voxels::robot::RobotInterface> > > (new_allocator.h:124)
==546==    by 0x6BB1391: destroy<std::pair<const std::__cxx11::basic_string<char>, boost::shared_ptr<gpu_voxels::robot::RobotInterface> > > (alloc_traits.h:542)
==546==    by 0x6BB1391: _M_destroy_node (stl_tree.h:553)
==546==    by 0x6BB1391: _M_drop_node (stl_tree.h:561)
==546==    by 0x6BB1391: std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, boost::shared_ptr<gpu_voxels::robot::RobotInterface> >, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, boost::shared_ptr<gpu_voxels::robot::RobotInterface> > >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, boost::shared_ptr<gpu_voxels::robot::RobotInterface> > > >::_M_erase(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, boost::shared_ptr<gpu_voxels::robot::RobotInterface> > >*) (stl_tree.h:1614)
==546==    by 0x6BB161F: clear (stl_tree.h:1075)
==546==    by 0x6BB161F: clear (stl_map.h:810)
==546==    by 0x6BB161F: gpu_voxels::GpuVoxels::~GpuVoxels() (GpuVoxels.cpp:48)
==546==    by 0x6BB16A1: checked_delete<gpu_voxels::GpuVoxels> (checked_delete.hpp:34)
==546==    by 0x6BB16A1: boost::detail::sp_counted_impl_p<gpu_voxels::GpuVoxels>::dispose() (sp_counted_impl.hpp:78)
==546==    by 0x41C3DE: boost::detail::sp_counted_base::release() (sp_counted_base_gcc_x86.hpp:146)
==546==    by 0x50C91EA: ~shared_count (shared_count.hpp:473)
==546==    by 0x50C91EA: ~shared_ptr (shared_ptr.hpp:336)
==546==    by 0x50C91EA: GPUVoxelsCollisionDetector::~GPUVoxelsCollisionDetector() (GPUVoxelsCollisionDetector.cpp:97)
==546==    by 0x50C924A: GPUVoxelsCollisionDetector::~GPUVoxelsCollisionDetector() (GPUVoxelsCollisionDetector.cpp:99)
==546==    by 0x42318E: operator() (unique_ptr.h:76)
==546==    by 0x42318E: ~unique_ptr (unique_ptr.h:236)
==546==    by 0x42318E: GPUVoxelsCollisionDetectorShould::~GPUVoxelsCollisionDetectorShould() (GPUVoxelsCollisionDetectorTests.cpp:38)
==546==  Address 0x288be040 is 0 bytes inside a block of size 33,568,560 alloc d
==546==    at 0x4C2E80F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==546==    by 0x698EEE5: gpu_voxels::MetaPointCloud::init(std::vector<unsigned int, std::allocator<unsigned int> > const&) (MetaPointCloud.cu:50)
==546==    by 0x69902C2: gpu_voxels::MetaPointCloud::addClouds(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool) (MetaPointCloud.cu:136)
==546==    by 0x6991580: gpu_voxels::MetaPointCloud::MetaPointCloud(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool) (MetaPointCloud.cu:157)
==546==    by 0x9F3A818: gpu_voxels::robot::KinematicChain::KinematicChain(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<gpu_voxels::robot::DHParameters, std::allocator<gpu_voxels::robot::DHParameters> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool) (KinematicChain.cu:41)
==546==    by 0x6BB418F: gpu_voxels::GpuVoxels::addRobot(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<gpu_voxels::robot::DHParameters, std::allocator<gpu_voxels::robot::DHParameters> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool) (GpuVoxels.cpp:333)
==546==    by 0x50CD532: GPUVoxelsCollisionDetector::setDenavitHartenbergParameters(std::vector<DHParameter, std::allocator<DHParameter> >) (GPUVoxelsCollisionDetector.cpp:438)
==546==    by 0x50D14C3: GPUVoxelsCollisionDetector::GPUVoxelsCollisionDetector(std::shared_ptr<RobotArmConfiguration>, std::vector<SpaceType, std::allocator<SpaceType> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GPUVoxelsCollisionDetector.cpp:89)
==546==    by 0x419321: GPUVoxelsCollisionDetectorShould_returnFalseIfDimenionsNumberAreDifferent_Test::TestBody() (GPUVoxelsCollisionDetectorTests.cpp:190)
==546==    by 0x4E8093B: HandleSehExceptionsInMethodIfSupported<testing::Test, void> (gtest.cc:2402)
==546==    by 0x4E8093B: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2438)
==546==    by 0x4E772DF: testing::Test::Run() (gtest.cc:2475)
==546==    by 0x4E773AC: testing::TestInfo::Run() (gtest.cc:2656)

[cjue]

Hi, thanks for the report!

On first look it might be a good idea to replace all/most of the new[] uses in there with boost::scoped_array, to guarantee correct cleanup:

https://www.boost.org/doc/libs/1_70_0/libs/smart_ptr/doc/html/smart_ptr.html#scoped_array

[cjue]

I will attempt to get the new release with the fixes out before my vacation.

[AlejoDiaz49]

Hello,

I just realized that you created another branch for indigo and you left the main branch with c++ 11 by default.
I know that it is a lot of work but maybe for the future is better if you use std::shared_ptr or std::unique_ptr pointer.

[cjue]

Yes,
we are currently working on using std::shared_ptr and std::unique_ptr where appropriate.

This would also mean that C++11 mode will be mandatory for future GPU-Voxels releases.

[cjue]

As a side-note, one complication with regard to MetaPointCloud is the presence of raw device pointers as well as non-owning pointer there.

This means that we can use smart pointers for the host memory, but some raw device pointers will remain.