-
Notifications
You must be signed in to change notification settings - Fork 938
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 geometric shapes related memory leaks #2138
Fix geometric shapes related memory leaks #2138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address my remark.
moveit_core/collision_distance_field/src/collision_distance_field_types.cpp
Show resolved
Hide resolved
@@ -1018,8 +1018,8 @@ bool PlanningScene::loadGeometryFromStream(std::istream& in, const Eigen::Isomet | |||
in >> shape_count; | |||
for (std::size_t i = 0; i < shape_count && in.good() && !in.eof(); ++i) | |||
{ | |||
shapes::Shape* s = shapes::constructShapeFromText(in); | |||
if (!s) | |||
const auto shape = shapes::ShapeConstPtr(shapes::constructShapeFromText(in)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are reasonable.
Codecov Report
@@ Coverage Diff @@
## master #2138 +/- ##
==========================================
- Coverage 57.79% 57.79% -0.01%
==========================================
Files 328 328
Lines 25700 25700
==========================================
- Hits 14854 14853 -1
- Misses 10846 10847 +1
Continue to review full report at Codecov.
|
* Fix memory leak with a shape object when malformed scene geometry file is loaded. * Fix memory leak by removing unnecessary and leaky call to clone() (cherry picked from commit 9bdce08)
* Fix memory leak with a shape object when malformed scene geometry file is loaded. * Fix memory leak by removing unnecessary and leaky call to clone() (cherry picked from commit 9bdce08)
* Fix memory leak with a shape object when malformed scene geometry file is loaded. * Fix memory leak by removing unnecessary and leaky call to clone() (cherry picked from commit 9bdce08)
Couple fixes to memory leaks caused by raw pointer usage with
geometric_shapes
objects.These fixes are related to the efforts and finding in this issue #2075. However, this PR does not fix the leaks reported by @tylerjw, but rather these are new findings made using address sanitizer.
Running address sanitizer revealed couple memory leaks related raw pointer usage with
geometric_shapes
objects. I added two fixes to this PR since they are both very small.First fix is wrapping return value of
shapes::constructShapeFromText(in)
in shared_ptr so that if the geometry being loaded is corrupted or malformed, the constructed shape will always be freed undepending on the code path.Second fix is removing call to
clone()
inBodyDecomposition::init()
. Prior to the fix, each of the cloned objects was heap allocated and raw pointer was passed toaddBody
without calling free on the objects at any point, which caused every single object to leak. Looking more closely at theaddBody
function I realized that there doesn't seem to be any reason to call clone there so I removed the operation altogether. Now, the raw pointers passed to the function point to theshared_ptr<Shape>
objects which will get freed correctly eventually thanks to reference counting.