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

FreeCAD · Jan 16, 2022 · f147986 · f147986
1 parent b836712
commit f147986
Show file tree

Hide file tree

Showing 14 changed files with 40 additions and 16 deletions.
diff --git a/src/App/DocumentObserver.cpp b/src/App/DocumentObserver.cpp
@@ -180,7 +180,7 @@ void DocumentObjectT::operator=(const DocumentObject* obj)
 }
 
 void DocumentObjectT::operator=(const Property *prop) {
-    if(!prop || !prop->getName()
+    if(!prop || !prop->hasName()
              || !prop->getContainer()
              || !prop->getContainer()->isDerivedFrom(App::DocumentObject::getClassTypeId()))
     {

diff --git a/src/App/DynamicProperty.cpp b/src/App/DynamicProperty.cpp
@@ -211,7 +211,7 @@ Property* DynamicProperty::addDynamicProperty(PropertyContainer &pc, const char*
 
 bool DynamicProperty::addProperty(Property *prop)
 {
-    if(!prop || !prop->getName())
+    if(!prop || !prop->hasName())
         return false;
     auto &index = props.get<0>();
     if(index.count(prop->getName()))

diff --git a/src/App/GeoFeaturePyImp.cpp b/src/App/GeoFeaturePyImp.cpp
@@ -66,8 +66,8 @@ PyObject* GeoFeaturePy::getPropertyNameOfGeometry(PyObject * args)
 
     GeoFeature* object = this->getGeoFeaturePtr();
     const PropertyComplexGeoData* prop = object->getPropertyOfGeometry();
-    const char* name = prop ? prop->getName() : 0;
-    if (name) {
+    const char* name = prop ? prop->getName() : nullptr;
+    if (Property::isValidName(name)) {
         return Py::new_reference_to(Py::String(std::string(name)));
     }
     return Py::new_reference_to(Py::None());

diff --git a/src/App/Link.cpp b/src/App/Link.cpp
@@ -184,7 +184,7 @@ void LinkBaseExtension::setProperty(int idx, Property *prop) {
             propName = prop->getName();
         else
             propName = extensionGetPropertyName(prop);
-        if(!propName)
+        if(!Property::isValidName(propName))
             propName = "?";
         FC_TRACE("set property " << infos[idx].name << ": " << propName);
     }

diff --git a/src/App/ObjectIdentifier.cpp b/src/App/ObjectIdentifier.cpp
@@ -165,7 +165,9 @@ ObjectIdentifier::ObjectIdentifier(const Property &prop, int index)
     DocumentObject * docObj = freecad_dynamic_cast<DocumentObject>(prop.getContainer());
 
     if (!docObj)
-        FC_THROWM(Base::TypeError,"Property must be owned by a document object.");
+        FC_THROWM(Base::TypeError, "Property must be owned by a document object.");
+    if (!prop.hasName())
+        FC_THROWM(Base::RuntimeError, "Property must have a name.");
 
     owner = const_cast<DocumentObject*>(docObj);
 

diff --git a/src/App/Property.cpp b/src/App/Property.cpp
@@ -61,9 +61,19 @@ Property::~Property()
 
 }
 
-const char* Property::getName(void) const
+const char* Property::getName() const
 {
-    return myName;
+    return myName ? myName : "";
+}
+
+bool Property::hasName() const
+{
+    return isValidName(myName);
+}
+
+bool Property::isValidName(const char* name)
+{
+    return name && name[0] != '\0';
 }
 
 std::string Property::getFullName() const {

diff --git a/src/App/Property.h b/src/App/Property.h
@@ -118,8 +118,20 @@ class AppExport Property : public Base::Persistence
         return sizeof(father) + sizeof(StatusBits);
     }
 
-    /// get the name of this property in the belonging container
+    /** Get the name of this property in the belonging container
+     * With \ref hasName() it can be checked beforehand if a valid name is set.
+     * @note If no name is set this function returns an empty string, i.e. "".
+     */
     const char* getName(void) const;
+    /** Check if the property has a name set.
+     * If no name is set then \ref getName() will return an empty string
+     */
+    bool hasName() const;
+    /** Check if the passed name is valid.
+     * If \a name is null or an empty string it's considered invalid,
+     * and valid otherwise.
+     */
+    static bool isValidName(const char* name);
 
     std::string getFullName() const;
 

diff --git a/src/App/PropertyLinks.cpp b/src/App/PropertyLinks.cpp
@@ -204,7 +204,7 @@ PropertyLinkBase::updateLabelReferences(App::DocumentObject *obj, const char *ne
 static std::string propertyName(const Property *prop) {
     if(!prop)
         return std::string();
-    if(!prop->getContainer() || !prop->getName()) {
+    if(!prop->getContainer() || !prop->hasName()) {
         auto xlink = Base::freecad_dynamic_cast<const PropertyXLink>(prop);
         if(xlink)
             return propertyName(xlink->parent());

diff --git a/src/Gui/DAGView/DAGModel.cpp b/src/Gui/DAGView/DAGModel.cpp
@@ -302,7 +302,7 @@ void Model::slotDeleteObject(const ViewProviderDocumentObject &VPDObjectIn)
 void Model::slotChangeObject(const ViewProviderDocumentObject &VPDObjectIn, const App::Property& propertyIn)
 {
   std::string name("Empty Name");
-  if (propertyIn.getName()) //getName can return 0.
+  if (propertyIn.hasName())
     name = propertyIn.getName();
   assert(!name.empty());
 

diff --git a/src/Gui/SelectionView.cpp b/src/Gui/SelectionView.cpp
@@ -551,7 +551,7 @@ QString SelectionView::getProperty(App::DocumentObject* obj) const
         App::GeoFeature* geo = static_cast<App::GeoFeature*>(obj);
         const App::PropertyComplexGeoData* data = geo->getPropertyOfGeometry();
         const char* name = data ? data->getName() : nullptr;
-        if (name) {
+        if (App::Property::isValidName(name)) {
             property = QString::fromLatin1(name);
         }
     }

diff --git a/src/Mod/Part/App/PartFeature.cpp b/src/Mod/Part/App/PartFeature.cpp
@@ -236,7 +236,7 @@ struct ShapeCache {
 
     void slotChanged(const App::DocumentObject &obj, const App::Property &prop) {
         const char *propName = prop.getName();
-        if(!propName)
+        if(!App::Property::isValidName(propName))
             return;
         if(strcmp(propName,"Shape")==0 
                 || strcmp(propName,"Group")==0 

diff --git a/src/Mod/PartDesign/Gui/TaskFeaturePick.cpp b/src/Mod/PartDesign/Gui/TaskFeaturePick.cpp
@@ -340,7 +340,7 @@ App::DocumentObject* TaskFeaturePick::makeCopy(App::DocumentObject* obj, std::st
 
             App::Property* cprop = *it++;
 
-            if( strcmp(prop->getName(), "Label") == 0 ) {
+            if( prop->getName() && strcmp(prop->getName(), "Label") == 0 ) {
                 static_cast<App::PropertyString*>(cprop)->setValue(name.c_str());
                 continue;
             }

diff --git a/src/Mod/PartDesign/Gui/Utils.cpp b/src/Mod/PartDesign/Gui/Utils.cpp
@@ -435,7 +435,7 @@ void relinkToBody (PartDesign::Feature *feature) {
                     }
                 }
 
-                if ( !valueStr.empty () ) {
+                if ( !valueStr.empty () && prop->hasName()) {
                     FCMD_OBJ_CMD(obj,prop->getName() << '=' << valueStr);
                 }
             }

diff --git a/src/Mod/Spreadsheet/Gui/DlgSheetConf.cpp b/src/Mod/Spreadsheet/Gui/DlgSheetConf.cpp
@@ -141,7 +141,7 @@ App::Property *DlgSheetConf::prepare(CellAddress &from, CellAddress &to,
                                 vexpr->getPath().getProperty());
             if(prop) {
                 auto obj = Base::freecad_dynamic_cast<DocumentObject>(prop->getContainer());
-                if(obj) {
+                if (obj && prop->hasName()) {
                     path = ObjectIdentifier(sheet);
                     path.setDocumentObjectName(obj,true);
                     path << ObjectIdentifier::SimpleComponent(prop->getName());