App: Bugfix for NULL-Pointer dereference of Property->getName()

[totaru]

Some parts of FreeCAD use prop->getName() without testing
for NULL first, e.g.:

src/Mod/PartDesign/Gui/ViewProviderAddSub.cpp:258
ViewProviderAddSub::updateData()
src/Mod/PartDesign/Gui/ViewProviderDatumCS.cpp:193
ViewProviderDatumCoordinateSystem::updateData()

See also:
https://forum.freecadweb.org/viewtopic.php?f=3&t=58603

I modified Property::getName() to return "?" if name is NULL, but I feel
not familiar enough to judge wether this is safe.

• [o] Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
• [o] Your branch is rebased on latest master git pull --rebase upstream master
• All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
• All commit messages are well-written ex: Fixes typo in Draft Move command text
• Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
• Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

---

[luzpaz]

Please amend the summary using git commit --ammend and preprend App: to the commit title. Then git push --force

[marioalexis84]

getName() is used in conditional expressions. This could break that code.

[yorikvanhavre]

I agree that a getName() function should always return a string, no matter what (I would personally prefer "Unnamed" over "?" because it's easier to grep for if needed and it doesn't look like an error). Indeed it will render conditional tests on that function useless, but in any case I don't think there is anything really useful to do when a property has no name (just use a default name, I guess), so this should be safe enough anyway

[marioalexis84]

situations like this should be detected and modified:

$ grep -RnE "if\s?\\(.*prop->getName\\(\\)\\)" src/
src/Mod/PartDesign/Gui/ViewProvider.cpp:307:        if(!vp->getPropertyByName(prop->getName()))
src/App/PropertyLinks.cpp:184:    if(!prop->getContainer() || !prop->getName()) {
src/App/DynamicProperty.cpp:186:    if(!prop || !prop->getName())
src/App/DynamicProperty.cpp:189:    if(index.count(prop->getName()))

[chennes]

I have one minor nitpick on the code itself as submitted. I also agree with @yorikvanhavre that "Unnamed" or "(Unnamed)" or something like that would be nicer. And I think that @marioalexis84 is right, that this PR should also include the necessary changes to ensure that the checks for empty names aren't broken by this change.

[chennes]

The C++ Core Guidelines suggest this would be better written:

Suggested change

	if (myName == NULL)
	if (!myName)

(see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es87-dont-add-redundant--or--to-conditions)

[berndhahnebach]

Following a link to the branch on the CI-repository:

https://gitlab.com/freecad/FreeCAD-CI/-/commits/PR_4943

The CI-status is available on the latest commit of the branch.
If there is no status available the PR should be rebased on latest master.
Check pipeline branches to see if your PR has been run by the CI.

https://gitlab.com/freecad/FreeCAD-CI/-/pipelines?scope=branches

[donovaly]

@totaru , what is the status of the PR? There are comments. an you please reply and rebase the PR?

[wwmayer]

The class Property has the private member myName that is by default set to null. The class itself doesn't offer methods to set the name from outside but instead there are two friend classes PropertyData and DynamicProperty that set a name when adding a property.

While for DynamicProperty::addDynamicProperty() it's guaranteed that myName cannot be null this is not the case for PropertyData::addProperty(). However, the latter is used with the macros ADD_PROPERTY() and ADD_PROPERTY_TYPE() and I am not aware that somewhere a feature passes a null pointer when registering a property. Nevertheless, to be 100% on the safe side PropertyData::addProperty() could be changed to print an error or throw an exception if the name is null.

So, normally it should not happen that for a property myName is null. In case it is unexpectedly null then it's an indication of a deeper problem elsewhere. The only allowed case when myName is reset to null is in DynamicProperty::removeDynamicProperty().

Modifying getName() to return a valid string in case myName is null makes the reported crash disappear but it also makes it impossible to find the actual reason for the crash and thus I recommend to leave it as it is without checking beforehand if the change would break anything else (e.g. if it's explicitly checked if getName() returns null and handles it accordingly).

And as it seems the problem has been resolved in the meantime as with current master the crash doesn't occur any more. However, it would still be interesting to know the real reason for the crash in v0.19 and what has fixed it in v0.20.

[wwmayer]

It seems the problem was fixed with fc9d354. I have built the version before this commit (6780ded) and there I can still reproduce it.

Adding some printf() statements revealed that a property of type App::PropertyXLink is created at runtime without having set a name. So, it was definitely a bug with the old behaviour when a link between two documents existed.

[wwmayer]

Since there some real use cases where e.g. a tmp. property is created on the stack that lacks of a name it's best to change getName() to always return either the name or an empty string. However, in several places it is checked if getName() returns null and this requires some further modifications, too.

See f147986

The changes:

• getName() always returns a string that might be empty
• hasName() has been added to conveniently check beforehand if a name is set
• isValidName() is a static helper function to check the returned value of getName()

[wwmayer]

This PR is obsolete now.