Sketcher: Freeing a non-pointer in App/planegcs/GCS.cpp

[andre-caldas]

Is there an existing issue for this?

• I have searched the existing issues

Forums discussion

https://forum.freecad.org/viewtopic.php?t=76724

Version

0.21 (Development)

Full version info

I saw the bug in the source code, not in a compiled program.

FreeCAD/src/Mod/Sketcher/App/planegcs/GCS.cpp

Line 641 in 45001fa

std::vector<Constraint *> constrvec;

Subproject(s) affected?

Sketcher

Issue description

The function constructs a std::vector<Constraint*> constrvec. Then, it pushes back a pointer and then calls free(constrvec).

This is wrong because constrvec is not a pointer. It is a local object that is automatically destroyed.

It seems the intention was to free the pushed back pointer. In this case, no std::vector<Constraint*> constrvec should be used! There are two possible fixes:

1. remove the three last lines of code from the removeConstraint() function.
2. substitute the three last lines of code by the appropriate destruction of constr.

I do believe we are not supposed to destroy anything. It would be wierd to have removeConstraint destroying/freeing constr, but not having addConstraint constructing/allocating it. However, it is a possible memory leak that needs to be investigated.

Anything else?

I can submit a patch, but I don't know if it is correct to not destruct/free constr.

Code of Conduct

• I agree to follow this project's Code of Conduct

[luzpaz]

@abdullahtahiriyo I've assigned to you. If not appropriate, apologies.

[abdullahtahiriyo]

@luzpaz

It is ok. Thank you.

@andre-caldas

Thanks André for your observation.

If I understand you well, you might be assuming that free is the eponymous C function. However that function takes a pointer as argument. If there weren't another function taking a vector of constraint pointers as argument, the code would not compile at all.

The actual function called is this one:

FreeCAD/src/Mod/Sketcher/App/planegcs/GCS.cpp

Line 4821 in 45001fa

void free(std::vector<Constraint *> &constrvec)

This function iterates the std::vector and deletes an static_cast -ed version of the pointer and clears the vector.

As you know, clearing a vector will not release the dynamically allocated memory of the pointers in it. So explicitly deleting the pointers is necessary before clearing the vector.

Leaving aside the fact that it is quite old code and things could be done differently (and certainly I will look into it with more calm), what it does ensure is that there is no memory leak when a constraint needs to be removed from the system.
So I see no memory leak.

I say things could be done differently because, at least now, constraint has a virtual destructor, so if every inherited class provided an appropriate destructor (either the compiler provided default one, which will be virtual due to inheritance, or a specific virtual override where necessary), this type checking and deleting appears to me, at a first glance, it should not be needed at all (dynamic dispatch would do it for you), and then indeed there would no use of reusing the free function for just one constraint and the code could be simplified to a simple delete constraint, as there will be no need to construct the vector just to reuse the free function (which I presume was the intention of the developer here). So, there might be indeed an opportunity to improve the code so that it is more readable (with proper compiler optimizations, the assembly may end up being the same, but code readability is important). But perhaps I should look into this after having some sleep, as now I have been almost 18 hours awake and I may be missing something.

Regarding your observation of addConstraint not allocating memory, it indeed does not, because it takes the pointer from specific functions which call it. These specific functions allocate with new the memory for the constraint dynamically. This pointer needs to be released. If what you mean is that constraints do not having any dynamically allocated memory of its own (i.e. data members), I think that is the case today (have not checked all of them), but it could happen in the future. Then, they should have an appropriate destructor, so that when delete is called on a base pointer, dynamic dispatch gets to the destructor of the specific constraint and memory is properly released.

Please, let me know if you think I am missing something.

[andre-caldas]

LOL...

I am so sorry (again!). :-)

[abdullahtahiriyo]

@andre-caldas

You should not be sorry.

You point out to some code that would benefit from a review. And I am leaving this unread to review it.

[luzpaz]

Thanks so much abdullah for your patience and your multi-faceted skills. ❤️