Pcl: Fix Memory Leaks

[kunaltyagi]

Your Environment

• PCL Version: HEAD, CI

Context

Most of the leaks are in application/test code, which will not impact the core library. But they should be removed nonetheless.

Some of the leaks have been fixed, but there are still some left.

List of files with leaks

ml/src/svm.cpp:2354  error memleakOnRealloc: Common realloc mistake: 'label' nulled but not freed upon failure
ml/src/svm.cpp:2355  error memleakOnRealloc: Common realloc mistake: 'count' nulled but not freed upon failure
ml/src/svm.cpp:3122  error memleakOnRealloc: Common realloc mistake: 'line' nulled but not freed upon failure
ml/src/svm.cpp:3523  error memleakOnRealloc: Common realloc mistake: 'label' nulled but not freed upon failure
ml/src/svm.cpp:3524  error memleakOnRealloc: Common realloc mistake: 'count' nulled but not freed upon failure
ml/src/svm.cpp:3181  error memleak: Memory leak: model.scaling
people/apps/main_ground_based_people_detection.cpp:255  error memleak: Memory leak: interface
surface/src/3rdparty/opennurbs/opennurbs_beam.cpp:868  warning nullPointerRedundantCheck: Either the condition '0!=polycurve' is redundant or there is possible null pointer dereference: polycurve.
surface/src/3rdparty/opennurbs/opennurbs_beam.cpp:2517  error nullPointer: Possible null pointer dereference: k
surface/src/3rdparty/opennurbs/opennurbs_beam.cpp:2962  error nullPointer: Possible null pointer dereference: null_brep_pointer
surface/src/3rdparty/opennurbs/opennurbs_beam.cpp:2936  warning nullPointerRedundantCheck: Either the condition 'newbrep' is redundant or there is possible null pointer dereference: newbrep.
surface/src/3rdparty/opennurbs/opennurbs_brep_tools.cpp:1269  warning nullPointerRedundantCheck: Either the condition '!brep' is redundant or there is possible null pointer dereference: brep.
surface/src/3rdparty/opennurbs/opennurbs_brep_tools.cpp:1860  warning nullPointerRedundantCheck: Either the condition '!Srf' is redundant or there is possible null pointer dereference: Srf.
surface/src/3rdparty/opennurbs/opennurbs_polycurve.cpp:2803  warning nullPointerRedundantCheck: Either the condition 'pLeftSide' is redundant or there is possible null pointer dereference: pLeftSide.
surface/src/3rdparty/opennurbs/opennurbs_polycurve.cpp:2808  warning nullPointerRedundantCheck: Either the condition 'pRightSide' is redundant or there is possible null pointer dereference: pRightSide.
surface/src/3rdparty/poisson4/marching_cubes_poisson.cpp:752  error uninitvar: Uninitialized variable: v
surface/src/3rdparty/poisson4/marching_cubes_poisson.cpp:867  error uninitvar: Uninitialized variable: v
surface/src/3rdparty/poisson4/marching_cubes_poisson.cpp:883  error uninitvar: Uninitialized variable: v
test/2d/test_2d.cpp:124  error memleak: Memory leak: k
test/2d/test_2d.cpp:124  error memleak: Memory leak: conv
test/2d/test_2d.cpp:157  error memleak: Memory leak: k
test/2d/test_2d.cpp:157  error memleak: Memory leak: conv
test/2d/test_2d.cpp:296  error memleak: Memory leak: morph
test/2d/test_2d.cpp:329  error memleak: Memory leak: morph
test/2d/test_2d.cpp:362  error memleak: Memory leak: morph
test/2d/test_2d.cpp:395  error memleak: Memory leak: morph
test/io/test_octree_compression.cpp:111  error memleak: Memory leak: pointcloud_encoder
test/io/test_octree_compression.cpp:111  error memleak: Memory leak: pointcloud_decoder
test/io/test_octree_compression.cpp:156  error memleak: Memory leak: pointcloud_encoder
test/io/test_octree_compression.cpp:156  error memleak: Memory leak: pointcloud_decoder
test/search/test_auto_search.cpp:187  error memleak: Memory leak: octree
test/search/test_auto_search.cpp:311  error memleak: Memory leak: kdtree
test/search/test_auto_search.cpp:309  error memleak: Memory leak: kdtree
test/search/test_auto_search.cpp:325  error memleak: Memory leak: search
test/search/test_auto_search.cpp:344  error memleak: Memory leak: kdtree
test/search/test_flann_search.cpp:128  error memleak: Memory leak: FlannSearch
test/search/test_flann_search.cpp:127  error memleak: Memory leak: FlannSearch
test/search/test_flann_search.cpp:174  error memleak: Memory leak: FlannSearch
test/search/test_flann_search.cpp:207  error memleak: Memory leak: FlannSearch
test/search/test_flann_search.cpp:252  error memleak: Memory leak: flann_search
test/search/test_flann_search.cpp:378  error memleak: Memory leak: FlannSearch
test/search/test_kdtree.cpp:123  error memleak: Memory leak: kdtree
test/search/test_kdtree.cpp:122  error memleak: Memory leak: kdtree
test/search/test_kdtree.cpp:166  error memleak: Memory leak: kdtree
test/search/test_kdtree.cpp:196  error memleak: Memory leak: kdtree
test/search/test_kdtree.cpp:208  error memleak: Memory leak: kdtree
test/search/test_octree.cpp:167  error memleak: Memory leak: octree
test/search/test_organized_index.cpp:173  error memleak: Memory leak: organizedNeighborSearch
test/search/test_organized_index.cpp:202  error memleak: Memory leak: organizedNeighborSearch
test/search/test_organized_index.cpp:375  error memleak: Memory leak: organizedNeighborSearch
test/search/test_organized_index.cpp:461  error memleak: Memory leak: organizedNeighborSearch
test/search/test_organized_index.cpp:600  error memleak: Memory leak: organizedNeighborSearch
test/test_recognition_ransac_based_ORROctree.cpp:117  error memleak: Memory leak: new_model
tools/mesh_sampling.cpp:92  error nullPointer: Possible null pointer dereference: ptIds
tools/mesh_sampling.cpp:93  error nullPointer: Possible null pointer dereference: ptIds
tools/mesh_sampling.cpp:94  error nullPointer: Possible null pointer dereference: ptIds
tools/mesh_sampling.cpp:115  error nullPointer: Possible null pointer dereference: ptIds
tools/mesh_sampling.cpp:116  error nullPointer: Possible null pointer dereference: ptIds
tools/mesh_sampling.cpp:117  error nullPointer: Possible null pointer dereference: ptIds
tools/oni_viewer_simple.cpp:210  error memleak: Memory leak: grabber
visualization/src/point_cloud_handlers.cpp:741  error memleak: Memory leak: pts

The format allows most editors to open the file using Ctrl+click (no, not vim :stuck_out_tongue: )

[taketwo]

awk -F':| ' '{print "vim "$1" +"$2}'

[kunaltyagi]

That's not a Ctrl+click @taketwo , that's a 'pipe' dream

[shrijitsingh99]

@kunaltyagi Which profiling tool did you use to check for memory leaks?

[kunaltyagi]

IIRC That's cppcheck

[maciejmatuszak]

I came leak across one in OctreeBase assignment operator
https://github.com/PointCloudLibrary/pcl/blob/master/octree/include/pcl/octree/octree_base.h#L250

  OctreeBase&
  operator=(const OctreeBase& source)
  {
    leaf_count_ = source.leaf_count_;
    branch_count_ = source.branch_count_;
    //  root_node_ has already data but it is never deleted
    root_node_ = new (BranchNode)(*(source.root_node_));
    depth_mask_ = source.depth_mask_;
    max_key_ = source.max_key_;
    octree_depth_ = source.octree_depth_;
    return (*this);
  }

one solution would be to delete the root_node_ before we assign new:

  OctreeBase&
  operator=(const OctreeBase& source)
  {
    leaf_count_ = source.leaf_count_;
    branch_count_ = source.branch_count_;
    if(root_node_)
    {
        delete root_node_;
    }
    root_node_ = new (BranchNode)(*(source.root_node_));
    depth_mask_ = source.depth_mask_;
    max_key_ = source.max_key_;
    octree_depth_ = source.octree_depth_;
    return (*this);
  }

Basically any time class own a resource (memory allocated with new operator) the 0/3/5 rule applies. It is lot of work to properly implements all the infrastructure to manage resource without introducing memory leaks, especially if "5" applies. I tend to avoid resources unless performance requirements warrant it.
Since the OctreeBase owns the root_node_ maybe it can be changed to instance variable instead of pointer.
If you do not know what I am talking about here is ref: https://en.cppreference.com/w/cpp/language/rule_of_three

I run my unit tests under valgrind when I found the leak.

[kunaltyagi]

Thanks for finding the leak. Regarding the rules of C++, a lot of the software was written in a pre-peer-review era.

[Dejauxvue]

I think the problem still exists. The operator= should do the same cleanup as ~OctreeBase i.e. deleteTree()
~~~
OctreeBase& operator=(const OctreeBase& source)
{
deleteTree();//otherwise, a memory leak persists
leaf_count_ = source.leaf_count_;
branch_count_ = source.branch_count_;
delete root_node_;

root_node_ = new (BranchNode)(*(source.root_node_));
depth_mask_ = source.depth_mask_;
max_key_ = source.max_key_;
octree_depth_ = source.octree_depth_;
return (*this);

}
~~~

[stale[bot]]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.