Skip to content

Commit

Permalink
Extension: Fix order-of-initialisation crash
Browse files Browse the repository at this point in the history
FreeCADs property system utilises some pointer math to calculate the offset between
property and base class. Due to virtual inheritance of th ePropertyContainer the memory
layout has been changed to rather random, which has lead to crashes dependend on the
order of object initialisation.

The solution is to not make PropertyContaner virtual but a class below, Base::Persitance.
Then the memory layout is random for Persistance, but it is perfectly aligned for the
base class chains from PropertyContainer onwards as well as from Extension onwards.
Hence the proeprty system was changed to take the offset always from those two.
  • Loading branch information
ickby authored and wwmayer committed Oct 8, 2016
1 parent 773c79d commit 22fa3b3
Show file tree
Hide file tree
Showing 31 changed files with 311 additions and 170 deletions.
2 changes: 1 addition & 1 deletion src/App/DocumentObjectExtension.cpp
Expand Up @@ -33,7 +33,7 @@

using namespace App;

PROPERTY_SOURCE(App::DocumentObjectExtension, App::Extension)
EXTENSION_PROPERTY_SOURCE(App::DocumentObjectExtension, App::Extension)

DocumentObjectExtension::DocumentObjectExtension()
{
Expand Down
2 changes: 1 addition & 1 deletion src/App/DocumentObjectExtension.h
Expand Up @@ -38,7 +38,7 @@ class AppExport DocumentObjectExtension : public App::Extension

//The cass does not have properties itself, but it is important to provide the property access
//functions. see cpp file for details
PROPERTY_HEADER(App::DocumentObjectExtension );
EXTENSION_PROPERTY_HEADER(App::DocumentObjectExtension );

public:

Expand Down
2 changes: 1 addition & 1 deletion src/App/DocumentObjectGroup.cpp
Expand Up @@ -33,7 +33,7 @@

using namespace App;

PROPERTY_SOURCE_WITH_EXTENSIONS(App::DocumentObjectGroup, App::DocumentObject, (App::GroupExtension))
PROPERTY_SOURCE_WITH_EXTENSIONS(App::DocumentObjectGroup, App::DocumentObject)

DocumentObjectGroup::DocumentObjectGroup(void): DocumentObject(), GroupExtension() {

Expand Down
33 changes: 33 additions & 0 deletions src/App/DynamicProperty.cpp
Expand Up @@ -30,6 +30,7 @@
#include "Property.h"
#include "PropertyContainer.h"
#include "Application.h"
#include "ExtensionContainer.h"
#include <Base/Reader.h>
#include <Base/Writer.h>
#include <Base/Console.h>
Expand Down Expand Up @@ -69,6 +70,10 @@ Property *DynamicProperty::getPropertyByName(const char* name) const
std::map<std::string,PropData>::const_iterator it = props.find(name);
if (it != props.end())
return it->second.property;

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyByName(name);

return this->pc->PropertyContainer::getPropertyByName(name);
}

Expand Down Expand Up @@ -113,6 +118,10 @@ const char* DynamicProperty::getPropertyName(const Property* prop) const
if (it->second.property == prop)
return it->first.c_str();
}

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyName(prop);

return this->pc->PropertyContainer::getPropertyName(prop);
}

Expand All @@ -139,6 +148,10 @@ short DynamicProperty::getPropertyType(const Property* prop) const
return attr;
}
}

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyType(prop);

return this->pc->PropertyContainer::getPropertyType(prop);
}

Expand All @@ -153,6 +166,10 @@ short DynamicProperty::getPropertyType(const char *name) const
attr |= Prop_ReadOnly;
return attr;
}

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyType(name);

return this->pc->PropertyContainer::getPropertyType(name);
}

Expand All @@ -162,6 +179,10 @@ const char* DynamicProperty::getPropertyGroup(const Property* prop) const
if (it->second.property == prop)
return it->second.group.c_str();
}

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyGroup(prop);

return this->pc->PropertyContainer::getPropertyGroup(prop);
}

Expand All @@ -170,6 +191,10 @@ const char* DynamicProperty::getPropertyGroup(const char *name) const
std::map<std::string,PropData>::const_iterator it = props.find(name);
if (it != props.end())
return it->second.group.c_str();

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyGroup(name);

return this->pc->PropertyContainer::getPropertyGroup(name);
}

Expand All @@ -179,6 +204,10 @@ const char* DynamicProperty::getPropertyDocumentation(const Property* prop) cons
if (it->second.property == prop)
return it->second.doc.c_str();
}

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyDocumentation(prop);

return this->pc->PropertyContainer::getPropertyDocumentation(prop);
}

Expand All @@ -187,6 +216,10 @@ const char* DynamicProperty::getPropertyDocumentation(const char *name) const
std::map<std::string,PropData>::const_iterator it = props.find(name);
if (it != props.end())
return it->second.doc.c_str();

if(this->pc->isDerivedFrom(App::ExtensionContainer::getClassTypeId()))
return static_cast<App::ExtensionContainer*>(this->pc)->ExtensionContainer::getPropertyDocumentation(name);

return this->pc->PropertyContainer::getPropertyDocumentation(name);
}

Expand Down
78 changes: 67 additions & 11 deletions src/App/Extension.cpp
Expand Up @@ -34,19 +34,15 @@
#include <Base/Console.h>
#include <Base/PyObjectBase.h>

/* We do not use a standart property macro for type initiation. The reason is that we want to expose all property functions,
* to allow the derived classes to access the private property data, but we do not want to have our
* property data a reference to the parent data. That is because the extension is used in a multi
* inheritance way, and hence our propertydata partent data would point to the same property data
* as any other parent of the inherited class. It makes more sense to create a total unrelated line
* of property datas which are added as additional parent to the extended class.
/* We do not use a standart property macro for type initiation. The reason is that we have the first
* PropertyData in the extension chain, there is no parent property data.
*/
TYPESYSTEM_SOURCE_P(App::Extension);
const App::PropertyData * App::Extension::getPropertyDataPtr(void){return &propertyData;}
const App::PropertyData & App::Extension::getPropertyData(void) const{return propertyData;}
const App::PropertyData * App::Extension::extensionGetPropertyDataPtr(void){return &propertyData;}
const App::PropertyData & App::Extension::extensionGetPropertyData(void) const{return propertyData;}
App::PropertyData App::Extension::propertyData;
void App::Extension::init(void){
initSubclass(App::Extension::classTypeId, "App::Extension" , "App::PropertyContainer", &(App::Extension::create) );
initSubclass(App::Extension::classTypeId, "App::Extension" , "Base::Persistence", &(App::Extension::create) );
}

using namespace App;
Expand All @@ -57,7 +53,6 @@ Extension::Extension()

Extension::~Extension()
{
Base::Console().Message("Delete extension\n");
if (!ExtensionPythonObject.is(Py::_None())){
// Remark: The API of Py::Object has been changed to set whether the wrapper owns the passed
// Python object or not. In the constructor we forced the wrapper to own the object so we need
Expand All @@ -82,6 +77,13 @@ void Extension::initExtension(ExtensionContainer* obj) {
if(m_extensionType.isBad())
throw Base::Exception("Extension: Extension type not set");

//all properties are initialised without PropertyContainer father. Now that we know it we can
//finaly finsih the property initialisation
std::vector<Property*> list;
extensionGetPropertyData().getPropertyList(this, list);
for(Property* prop : list)
prop->setContainer(obj);

m_base = obj;
m_base->registerExtension( m_extensionType, this );
}
Expand All @@ -106,8 +108,62 @@ const char* Extension::name() {
return std::string().c_str();
}



Property* Extension::extensionGetPropertyByName(const char* name) const {

return extensionGetPropertyData().getPropertyByName(this, name);
}

short int Extension::extensionGetPropertyType(const Property* prop) const {

return extensionGetPropertyData().getType(this, prop);
}

short int Extension::extensionGetPropertyType(const char* name) const {

return extensionGetPropertyData().getType(this, name);
}

const char* Extension::extensionGetPropertyName(const Property* prop) const {

return extensionGetPropertyData().getName(this,prop);
}

const char* Extension::extensionGetPropertyGroup(const Property* prop) const {

return extensionGetPropertyData().getGroup(this,prop);
}

const char* Extension::extensionGetPropertyGroup(const char* name) const {

return extensionGetPropertyData().getGroup(this,name);
}


const char* Extension::extensionGetPropertyDocumentation(const Property* prop) const {

return extensionGetPropertyData().getDocumentation(this, prop);
}

const char* Extension::extensionGetPropertyDocumentation(const char* name) const {

return extensionGetPropertyData().getDocumentation(this, name);
}

void Extension::extensionGetPropertyList(std::vector< Property* >& List) const {

extensionGetPropertyData().getPropertyList(this, List);
}

void Extension::extensionGetPropertyMap(std::map< std::string, Property* >& Map) const {

extensionGetPropertyData().getPropertyMap(this, Map);
}


namespace App {
PROPERTY_SOURCE_TEMPLATE(App::ExtensionPython, App::ExtensionPython::Inherited)
EXTENSION_PROPERTY_SOURCE_TEMPLATE(App::ExtensionPython, App::ExtensionPython::Inherited)

// explicit template instantiation
template class AppExport ExtensionPythonT<Extension>;
Expand Down

0 comments on commit 22fa3b3

Please sign in to comment.