Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak in TextureMapping #3549

Merged

Conversation

RLThomaz
Copy link
Contributor

The member function pcl::TextureMapping<PointInT>::showOcclusions (const PointCloudPtr &input_cloud,... had a memory leak due to the use of a raw pointer (without deleting it after). I substituted it with (what I believe is) the smart pointer OctreePtr previously declared.

kunaltyagi
kunaltyagi previously approved these changes Jan 14, 2020
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RLThomaz 😄 👍

@kunaltyagi kunaltyagi changed the title Texture mapping memory leak Fix memory leak in TextureMapping using OctreePtr Jan 14, 2020
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the pointer really required? Why not construct a simple object and skip creating a shared_ptr?

@kunaltyagi kunaltyagi self-requested a review January 14, 2020 08:09
@kunaltyagi kunaltyagi dismissed their stale review January 14, 2020 08:10

Solution might be not to use pointers

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please convert this into a normal variable so there's no need of memory allocation. No allocation, no leaks 😄

@RLThomaz
Copy link
Contributor Author

Is the pointer really required? Why not construct a simple object and skip creating a shared_ptr?

Not really, indeed. That is where things get a bit fuzzy to me regarding the PCL standard, we can find this sort of thing throughout all modules - if I'm not mistaken.

Please convert this into a normal variable so there's no need of memory allocation. No allocation, no leaks

Sure, will do.

@kunaltyagi
Copy link
Member

we can find this sort of thing throughout all modules

That's true. How did you find this mistake? Maybe we can run the sanity check/script to remove them.

@kunaltyagi kunaltyagi changed the title Fix memory leak in TextureMapping using OctreePtr Fix memory leak in TextureMapping Jan 14, 2020
@RLThomaz
Copy link
Contributor Author

Ok, I removed the unnecessary smart pointer in two places.

That's true. How did you find this mistake? Maybe we can run the sanity check/script to remove them.

I'm using some static code analysis, more precisely cppcheck. But it will only check the headers I am actually using.

@RLThomaz
Copy link
Contributor Author

@kunaltyagi I just realised that PCL has a CPP file for each separate module, so I was able to run the static analyser on it. We got many similar memory leaks throughout the codebase.

/home/ricardo/RLThomaz/pcl/apps/cloud_composer/src/cloud_view.cpp  137  warning nullPointerRedundantCheck: Either the condition 'item_to_remove' is redundant or there is possible null pointer dereference: item_to_remove.
/home/ricardo/RLThomaz/pcl/apps/src/openni_3d_concave_hull.cpp  156  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_3d_convex_hull.cpp  154  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_boundary_estimation.cpp  175  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_change_viewer.cpp  139  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_fast_mesh.cpp  142  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_feature_persistence.cpp  206  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_ii_normal_estimation.cpp  179  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_mls_smoothing.cpp  161  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_octree_compression.cpp  174  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_octree_compression.cpp  231  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_organized_compression.cpp  174  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_organized_compression.cpp  267  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_organized_edge_detection.cpp  249  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_organized_multi_plane_segmentation.cpp  194  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_planar_convex_hull.cpp  139  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_planar_segmentation.cpp  134  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_tracking.cpp  649  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_uniform_sampling.cpp  138  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/apps/src/openni_voxel_grid.cpp  132  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_mapping.cpp  267  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_normals_cuda.cpp  213  error memleak: Memory leak: filegrabber
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_normals_cuda.cpp  242  error memleak: Memory leak: grabber
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_planes_cuda.cpp  299  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_ransac.cpp  219  error memleak: Memory leak: filegrabber
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_segmentation_cuda.cpp  387  error memleak: Memory leak: filegrabber
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_segmentation_cuda.cpp  416  error memleak: Memory leak: grabber
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_segmentation_planes_cuda.cpp  262  error memleak: Memory leak: filegrabber
/home/ricardo/RLThomaz/pcl/cuda/apps/src/kinect_segmentation_planes_cuda.cpp  291  error memleak: Memory leak: grabber
/home/ricardo/RLThomaz/pcl/doc/tutorials/content/sources/ground_based_rgbd_people_detection/src/main_ground_based_people_detection.cpp  248  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/doc/tutorials/content/sources/point_cloud_compression/point_cloud_compression.cpp  83  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/gpu/people/tools/people_tracking.cpp  73  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/gpu/people/tools/people_tracking.cpp  128  error memleak: Memory leak: app.m_proc
/home/ricardo/RLThomaz/pcl/io/src/openni2_grabber.cpp  346  error syntaxError: syntax error
/home/ricardo/RLThomaz/pcl/io/src/openni_grabber.cpp  369  error syntaxError: syntax error
/home/ricardo/RLThomaz/pcl/ml/src/svm.cpp  2354  error memleakOnRealloc: Common realloc mistake: 'label' nulled but not freed upon failure
/home/ricardo/RLThomaz/pcl/ml/src/svm.cpp  2355  error memleakOnRealloc: Common realloc mistake: 'count' nulled but not freed upon failure
/home/ricardo/RLThomaz/pcl/ml/src/svm.cpp  3122  error memleakOnRealloc: Common realloc mistake: 'line' nulled but not freed upon failure
/home/ricardo/RLThomaz/pcl/ml/src/svm.cpp  3523  error memleakOnRealloc: Common realloc mistake: 'label' nulled but not freed upon failure
/home/ricardo/RLThomaz/pcl/ml/src/svm.cpp  3524  error memleakOnRealloc: Common realloc mistake: 'count' nulled but not freed upon failure
/home/ricardo/RLThomaz/pcl/ml/src/svm.cpp  3181  error memleak: Memory leak: model.scaling
/home/ricardo/RLThomaz/pcl/people/apps/main_ground_based_people_detection.cpp  255  error memleak: Memory leak: interface
/home/ricardo/RLThomaz/pcl/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.
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/opennurbs/opennurbs_beam.cpp  2517  error nullPointer: Possible null pointer dereference: k
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/opennurbs/opennurbs_beam.cpp  2962  error nullPointer: Possible null pointer dereference: null_brep_pointer
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/opennurbs/opennurbs_beam.cpp  2936  warning nullPointerRedundantCheck: Either the condition 'newbrep' is redundant or there is possible null pointer dereference: newbrep.
/home/ricardo/RLThomaz/pcl/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.
/home/ricardo/RLThomaz/pcl/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.
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/opennurbs/opennurbs_polycurve.cpp  2803  warning nullPointerRedundantCheck: Either the condition 'pLeftSide' is redundant or there is possible null pointer dereference: pLeftSide.
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/opennurbs/opennurbs_polycurve.cpp  2808  warning nullPointerRedundantCheck: Either the condition 'pRightSide' is redundant or there is possible null pointer dereference: pRightSide.
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/poisson4/marching_cubes_poisson.cpp  752  error uninitvar: Uninitialized variable: v
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/poisson4/marching_cubes_poisson.cpp  867  error uninitvar: Uninitialized variable: v
/home/ricardo/RLThomaz/pcl/surface/src/3rdparty/poisson4/marching_cubes_poisson.cpp  883  error uninitvar: Uninitialized variable: v
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  124  error memleak: Memory leak: k
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  124  error memleak: Memory leak: conv
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  157  error memleak: Memory leak: k
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  157  error memleak: Memory leak: conv
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  296  error memleak: Memory leak: morph
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  329  error memleak: Memory leak: morph
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  362  error memleak: Memory leak: morph
/home/ricardo/RLThomaz/pcl/test/2d/test_2d.cpp  395  error memleak: Memory leak: morph
/home/ricardo/RLThomaz/pcl/test/io/test_octree_compression.cpp  111  error memleak: Memory leak: pointcloud_encoder
/home/ricardo/RLThomaz/pcl/test/io/test_octree_compression.cpp  111  error memleak: Memory leak: pointcloud_decoder
/home/ricardo/RLThomaz/pcl/test/io/test_octree_compression.cpp  156  error memleak: Memory leak: pointcloud_encoder
/home/ricardo/RLThomaz/pcl/test/io/test_octree_compression.cpp  156  error memleak: Memory leak: pointcloud_decoder
/home/ricardo/RLThomaz/pcl/test/search/test_auto_search.cpp  187  error memleak: Memory leak: octree
/home/ricardo/RLThomaz/pcl/test/search/test_auto_search.cpp  311  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_auto_search.cpp  309  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_auto_search.cpp  325  error memleak: Memory leak: search
/home/ricardo/RLThomaz/pcl/test/search/test_auto_search.cpp  344  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_flann_search.cpp  128  error memleak: Memory leak: FlannSearch
/home/ricardo/RLThomaz/pcl/test/search/test_flann_search.cpp  127  error memleak: Memory leak: FlannSearch
/home/ricardo/RLThomaz/pcl/test/search/test_flann_search.cpp  174  error memleak: Memory leak: FlannSearch
/home/ricardo/RLThomaz/pcl/test/search/test_flann_search.cpp  207  error memleak: Memory leak: FlannSearch
/home/ricardo/RLThomaz/pcl/test/search/test_flann_search.cpp  252  error memleak: Memory leak: flann_search
/home/ricardo/RLThomaz/pcl/test/search/test_flann_search.cpp  378  error memleak: Memory leak: FlannSearch
/home/ricardo/RLThomaz/pcl/test/search/test_kdtree.cpp  123  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_kdtree.cpp  122  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_kdtree.cpp  166  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_kdtree.cpp  196  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_kdtree.cpp  208  error memleak: Memory leak: kdtree
/home/ricardo/RLThomaz/pcl/test/search/test_octree.cpp  167  error memleak: Memory leak: octree
/home/ricardo/RLThomaz/pcl/test/search/test_organized_index.cpp  173  error memleak: Memory leak: organizedNeighborSearch
/home/ricardo/RLThomaz/pcl/test/search/test_organized_index.cpp  202  error memleak: Memory leak: organizedNeighborSearch
/home/ricardo/RLThomaz/pcl/test/search/test_organized_index.cpp  375  error memleak: Memory leak: organizedNeighborSearch
/home/ricardo/RLThomaz/pcl/test/search/test_organized_index.cpp  461  error memleak: Memory leak: organizedNeighborSearch
/home/ricardo/RLThomaz/pcl/test/search/test_organized_index.cpp  600  error memleak: Memory leak: organizedNeighborSearch
/home/ricardo/RLThomaz/pcl/test/test_recognition_ransac_based_ORROctree.cpp  117  error memleak: Memory leak: new_model
/home/ricardo/RLThomaz/pcl/tools/mesh_sampling.cpp  92  error nullPointer: Possible null pointer dereference: ptIds
/home/ricardo/RLThomaz/pcl/tools/mesh_sampling.cpp  93  error nullPointer: Possible null pointer dereference: ptIds
/home/ricardo/RLThomaz/pcl/tools/mesh_sampling.cpp  94  error nullPointer: Possible null pointer dereference: ptIds
/home/ricardo/RLThomaz/pcl/tools/mesh_sampling.cpp  115  error nullPointer: Possible null pointer dereference: ptIds
/home/ricardo/RLThomaz/pcl/tools/mesh_sampling.cpp  116  error nullPointer: Possible null pointer dereference: ptIds
/home/ricardo/RLThomaz/pcl/tools/mesh_sampling.cpp  117  error nullPointer: Possible null pointer dereference: ptIds
/home/ricardo/RLThomaz/pcl/tools/oni_viewer_simple.cpp  210  error memleak: Memory leak: grabber
/home/ricardo/RLThomaz/pcl/visualization/src/point_cloud_handlers.cpp  741  error memleak: Memory leak: pts

@kunaltyagi
Copy link
Member

kunaltyagi commented Jan 14, 2020

The errors with Either the condition <insert condition> is redundant or there is possible null pointer dereference are sometimes redundant 😆

@RLThomaz
Copy link
Contributor Author

The errors with Either the condition <insert condition> is redundant or there is possible null pointer dereference is sometimes redundant

Indeed, but they are just warnings - I ran that to output inconclusive issues. I'm now building the PCL trunk in a docker container so I can check against all header files. This output was only for the CPP files found ignoring all headers includes, i.e., actual implementation code.

@taketwo taketwo merged commit 4aa35ab into PointCloudLibrary:master Jan 16, 2020
@taketwo taketwo changed the title Fix memory leak in TextureMapping Fix memory leak in TextureMapping Jan 18, 2020
@RLThomaz RLThomaz deleted the texture_mapping_memory_leak branch January 22, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants