-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaConstraint] fix segfault in GenericConstraintSolver #2265
[SofaConstraint] fix segfault in GenericConstraintSolver #2265
Conversation
for(unsigned int i=0; i<constraintsResolutions.size(); i++) | ||
{ | ||
delete constraintsResolution; | ||
if (constraintsResolutions[i] != nullptr) | ||
{ | ||
delete constraintsResolutions[i]; | ||
constraintsResolutions[i] = nullptr; | ||
} | ||
} | ||
} |
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.
Maybe if we want to keep the modern for-loop, the fix would be like this: (cannot make a suggestion)
for(auto* constraintsResolution : constraintsResolutions)
{
if (constraintsResolution != nullptr)
{
delete constraintsResolution;
constraintsResolution = nullptr;
}
}
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.
I am not sure it would do it. If I am not wrong, constraintsResolution
is a copy of a pointer, and not the actual element of the vector. See my suggestion in my review.
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.
Also, nullptr
checking is not necessary. Deallocation of a null pointer does nothing (see https://en.cppreference.com/w/cpp/memory/new/operator_delete)
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.
I am not sure it would do it. If I am not wrong,
constraintsResolution
is a copy of a pointer, and not the actual element of the vector. See my suggestion in my review.
nice catch 👍
@EulalieCoevoet Thank you for reporting the error (too bad it skipped the reviews 😕) |
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.
I was surprised by this fix, but it is explained by the fact that this function is not only used in the destructor. When it is not in the destructor, the std::vector
is not cleared, but resized, and the pointers remain. Since they were not set to nullptr
, it leads to pointer dereference on an invalid address.
Note that I prefer the range-based for loop. It would look like:
for(auto*& constraintsResolution : constraintsResolutions)
{
delete constraintsResolution;
constraintsResolution = nullptr;
}
to be tested.
Alternatively, we could keep the current implementation and clear constraintsResolutions
after the loop.
I am surprised that the CI did not catch this problem. I suggest to add a unit test for it. I can do it, but I need a minimal example leading to a crash.
It seems to be a major source of crash. @fredroy @guparan would a patch for v21.06 be conceivable?
I confirm this version works in my scenes:
The alternative of clearing the vector after the loop also seems to work well: for (auto*& constraintsResolution : constraintsResolutions)
{
delete constraintsResolution;
}
constraintsResolutions.clear(); It is a pretty important bug to fix, half of my scenes are also crashing. |
I'm sorry I can't make a minimal example. I tried in vain, all the crashing scenes I found are using private plugins. |
@EulalieCoevoet, same for me, unfortunately.... all my scenes crashing are using private components |
I am catching up with all PRs of the last four weeks. |
* [SofaConstraint] fix segfault in GenericConstraintSolver (bug introduced in PR#2225) * [SofaConstraint] range based for loop Co-authored-by: EulalieCoevoet <> Co-authored-by: alxbilger <alxbilger@users.noreply.github.com>
This PR fixes a bug introduced in #2225. It's basically just a revert.
Half of my scenes were crashing :(
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if