[SofaConstraint] fix segfault in GenericConstraintSolver

[EulalieCoevoet]

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

• it builds with SUCCESS for all platforms on the CI.
• it does not generate new warnings.
• it does not generate new unit test failures.
• it does not generate new scene test failures.
• it does not break API compatibility.
• it is more than 1 week old (or has fast-merge label).

[fredroy]

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;
        }
    }

[alxbilger]

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.

[alxbilger]

Also, nullptr checking is not necessary. Deallocation of a null pointer does nothing (see https://en.cppreference.com/w/cpp/memory/new/operator_delete)

[fredroy]

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 👍

[fredroy]

@EulalieCoevoet Thank you for reporting the error (too bad it skipped the reviews 😕)
By any chance, would have a simple scene to submit as a test? thanks ! 😙

[alxbilger]

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?

[pedroperrusi]

I confirm this version works in my scenes:

Note that I prefer the range-based for loop. It would look like:

for(auto*& constraintsResolution : constraintsResolutions)
{
    delete constraintsResolution;
    constraintsResolution = nullptr;
}

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.
Thanks for finding the bug and the solution ideas

[EulalieCoevoet]

I'm sorry I can't make a minimal example. I tried in vain, all the crashing scenes I found are using private plugins.
@pedroperrusi is it the same for you?

[pedroperrusi]

@EulalieCoevoet, same for me, unfortunately.... all my scenes crashing are using private components

[hugtalbot]

I am catching up with all PRs of the last four weeks.
@pedroperrusi I would be awesome if you could contribute a test ensuring that we detect the possible problem induced. Would you agree?