Bug 368282 - Make type casting more robust
Summary: Make type casting more robust
Status: RESOLVED FIXED
Alias: None
Product: umbrello
Classification: Applications
Component: general (show other bugs)
Version: 2.20.1 (KDE Applications 16.08.1)
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Umbrello Development Group
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-05 13:49 UTC by Ralf Habacker
Modified: 2016-09-11 11:15 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In: 2.20.80 (KDE Applications 16.11.80)


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2016-09-05 13:49:27 UTC
Umbrello UML model is designed by using several classes named UML.... Mostly of them are based on class UMLObject. Storing uml model objects are saved in lists of type UMLObject* for example in UMLPackage::m_objects and casted back to the original type by using a static_cast. In several places a type check is done before casting by using UMLObject::baseType()  to make sure not to perform invalid casts. While working on the code in the past it turns out that this does not work in any cases. A prominent example is in bool UMLClassifier::resolveRef() 
... 
        if (obj->resolveRef()) {
            UMLClassifierListItem *cli = static_cast<UMLClassifierListItem*>(obj);
            switch (cli->baseType()) {

where 'obj' is casted to UMLClassifierListItem.  Mostly objects are not of type UMLClassifierListItem, which results into an invalid type case. Using the related code to use a dynamic cast

        if (obj->resolveRef()) {
            UMLClassifierListItem *cli = dynamic_cast<UMLClassifierListItem*>(obj);
            switch (cli->baseType()) {

return zero, but let umbrello crash on accessing cli->baseType() on loading a bigger xmi file. To complete the fix the return value from dynamic_cast needs to be checked as shown below.

        if (obj->resolveRef()) {
            UMLClassifierListItem *cli = dynamic_cast<UMLClassifierListItem*>(obj);
            if (cli)
                return false;
            switch (cli->baseType()) {

These issues need to be fixed in the code. Because several of them are reported by the coverity scan there is already a list of related locations.

Another issue with directly using casts is that it could be applied to any object, although it may be not applicable as shown in the following example. In  UMLOperation::toString() there is the following code
... 
  UMLClassifier *ownParent = static_cast<UMLClassifier*>(parent());
... 
which looks like a call to get the parent UML model object.  In fact parent() if of type QObject (the uml model parent is returned with umlPackage()) and the static_cast returns an invalid cast. Also dynamic_cast would fail (it will return 0, which is not expected) 

To avoid such issues and for simplier writing of such fragements class UMLObject should get type wrappers containing casts to related classes as shown in the following example.

  UMLClassifier *ownParent = parent()->asUMLClassifier();

The benefit of having this style is that the compiler detects that parent() is not of type UMLObject and fails to compile with error: 'class QObject' has no member named 'asUMLClassifier', so we are informed at compile time of this issue.

Reproducible: Always
Comment 1 Ralf Habacker 2016-09-06 15:00:19 UTC
Git commit a5f92b67494666e4e6779b1f2443b1a1df2b031d by Ralf Habacker.
Committed on 06/09/2016 at 14:59.
Pushed by habacker into branch 'master'.

Convert casts in UML model classes to use related casting methods.
Reviewed-by: aaron.nottbeck at sag.eu

M  +7    -7    umbrello/clipboard/umlclipboard.cpp
M  +1    -1    umbrello/clipboard/umldragdata.cpp
M  +2    -2    umbrello/codegenerators/ada/adawriter.cpp
M  +2    -2    umbrello/codegenerators/advancedcodegenerator.cpp
M  +1    -1    umbrello/codegenerators/classifiercodedocument.cpp
M  +8    -6    umbrello/codegenerators/codeclassfield.cpp
M  +1    -1    umbrello/codegenerators/codeclassfielddeclarationblock.cpp
M  +2    -2    umbrello/codegenerators/codedocument.cpp
M  +3    -3    umbrello/codegenerators/codegenerator.cpp
M  +1    -1    umbrello/codegenerators/codegenobjectwithtextblocks.cpp
M  +2    -2    umbrello/codegenerators/codeoperation.cpp
M  +3    -3    umbrello/codegenerators/codeparameter.cpp
M  +1    -1    umbrello/codegenerators/cpp/cppcodeclassfield.cpp
M  +2    -2    umbrello/codegenerators/cpp/cppcodegenerator.cpp
M  +2    -2    umbrello/codegenerators/cpp/cppheadercodedocument.cpp
M  +1    -1    umbrello/codegenerators/csharp/csharpwriter.cpp
M  +1    -1    umbrello/codegenerators/d/dclassifiercodedocument.cpp
M  +1    -1    umbrello/codegenerators/d/dcodeclassfield.cpp
M  +1    -1    umbrello/codegenerators/java/javaantcodedocument.cpp
M  +1    -1    umbrello/codegenerators/java/javaclassifiercodedocument.cpp
M  +1    -1    umbrello/codegenerators/java/javacodeclassfield.cpp
M  +2    -2    umbrello/codegenerators/ownedcodeblock.cpp
M  +1    -1    umbrello/codegenerators/ownedhierarchicalcodeblock.cpp
M  +2    -2    umbrello/codegenerators/pascal/pascalwriter.cpp
M  +1    -1    umbrello/codegenerators/ruby/rubyclassifiercodedocument.cpp
M  +1    -1    umbrello/codegenerators/ruby/rubycodeclassfield.cpp
M  +1    -1    umbrello/codegenerators/sql/mysqlwriter.cpp
M  +4    -4    umbrello/codegenerators/sql/sqlwriter.cpp
M  +1    -1    umbrello/codegenerators/vala/valawriter.cpp
M  +1    -1    umbrello/codegenerators/xml/xmlschemawriter.cpp
M  +10   -10   umbrello/codeimport/adaimport.cpp
M  +7    -7    umbrello/codeimport/csharp/csharpimport.cpp
M  +7    -7    umbrello/codeimport/idlimport.cpp
M  +8    -8    umbrello/codeimport/import_utils.cpp
M  +9    -9    umbrello/codeimport/javaimport.cpp
M  +5    -5    umbrello/codeimport/kdevcppparser/cpptree2uml.cpp
M  +6    -6    umbrello/codeimport/pascalimport.cpp
M  +3    -3    umbrello/codeimport/pythonimport.cpp
M  +6    -6    umbrello/codeimport/sqlimport.cpp
M  +7    -7    umbrello/dialogs/codeeditor.cpp
M  +4    -4    umbrello/dialogs/pages/classgeneralpage.cpp
M  +11   -11   umbrello/dialogs/pages/classifierlistpage.cpp
M  +2    -2    umbrello/dialogs/pages/constraintlistpage.cpp
M  +1    -1    umbrello/dialogs/umlattributedialog.cpp
M  +4    -4    umbrello/dialogs/umlforeignkeyconstraintdialog.cpp
M  +1    -1    umbrello/dialogs/umloperationdialog.cpp
M  +1    -1    umbrello/dialogs/umluniqueconstraintdialog.cpp
M  +6    -6    umbrello/dialogs/widgets/umldatatypewidget.cpp
M  +1    -1    umbrello/dialogs/widgets/umlpackagewidget.cpp
M  +1    -1    umbrello/docwindow.cpp
M  +1    -1    umbrello/finder/umllistviewfinder.cpp
M  +5    -6    umbrello/listpopupmenu.cpp
M  +14   -14   umbrello/model_utils.cpp
M  +9    -9    umbrello/object_factory.cpp
M  +12   -12   umbrello/petaltree2uml.cpp
M  +25   -25   umbrello/refactoring/refactoringassistant.cpp
M  +1    -1    umbrello/stereotypeswindow.cpp
M  +20   -20   umbrello/umldoc.cpp
M  +34   -29   umbrello/umllistview.cpp
M  +12   -12   umbrello/umllistviewitem.cpp
M  +1    -1    umbrello/umlmodel/association.cpp
M  +8    -8    umbrello/umlmodel/attribute.cpp
M  +1    -1    umbrello/umlmodel/category.cpp
M  +2    -2    umbrello/umlmodel/checkconstraint.cpp
M  +18   -16   umbrello/umlmodel/classifier.cpp
M  +2    -2    umbrello/umlmodel/classifierlistitem.cpp
M  +5    -5    umbrello/umlmodel/entity.cpp
M  +1    -1    umbrello/umlmodel/entityattribute.cpp
M  +1    -1    umbrello/umlmodel/enumliteral.cpp
M  +5    -5    umbrello/umlmodel/folder.cpp
M  +9    -9    umbrello/umlmodel/foreignkeyconstraint.cpp
M  +3    -3    umbrello/umlmodel/operation.cpp
M  +15   -15   umbrello/umlmodel/package.cpp
M  +5    -5    umbrello/umlmodel/umlcanvasobject.cpp
M  +1    -0    umbrello/umlmodel/umlobject.cpp
M  +2    -0    umbrello/umlmodel/umlobject.h
M  +1    -1    umbrello/umlmodel/umlrole.cpp
M  +6    -6    umbrello/umlmodel/uniqueconstraint.cpp
M  +15   -15   umbrello/umlscene.cpp
M  +7    -7    umbrello/umlwidgets/associationwidget.cpp
M  +1    -1    umbrello/umlwidgets/categorywidget.cpp
M  +2    -2    umbrello/umlwidgets/entitywidget.cpp
M  +2    -2    umbrello/umlwidgets/floatingtextwidget.cpp
M  +2    -2    umbrello/umlwidgets/messagewidget.cpp
M  +1    -1    umbrello/umlwidgets/toolbarstateonewidget.cpp
M  +27   -27   umbrello/umlwidgets/widget_factory.cpp
M  +3    -3    umbrello/umlwidgets/widgetbase.cpp

http://commits.kde.org/umbrello/a5f92b67494666e4e6779b1f2443b1a1df2b031d
Comment 2 Ralf Habacker 2016-09-06 15:00:20 UTC
Git commit fa3f51e764c4296c81454f25b340d00241f172d3 by Ralf Habacker.
Committed on 06/09/2016 at 14:59.
Pushed by habacker into branch 'master'.

Fix QPointer related dynamic_casts to use asUML... wrapper().
Reviewed-by: aaron.nottbeck at sag.eu

M  +5    -5    umbrello/umlmodel/classifier.cpp
M  +1    -1    umbrello/umlmodel/classifierlistitem.cpp
M  +2    -2    umbrello/umlwidgets/artifactwidget.cpp
M  +8    -8    umbrello/umlwidgets/associationwidget.cpp
M  +1    -1    umbrello/umlwidgets/categorywidget.cpp
M  +1    -1    umbrello/umlwidgets/classifierwidget.cpp
M  +3    -3    umbrello/umlwidgets/componentwidget.cpp
M  +8    -8    umbrello/umlwidgets/entitywidget.cpp
M  +4    -4    umbrello/umlwidgets/enumwidget.cpp
M  +1    -1    umbrello/umlwidgets/messagewidget.cpp

http://commits.kde.org/umbrello/fa3f51e764c4296c81454f25b340d00241f172d3
Comment 3 Ralf Habacker 2016-09-06 15:00:20 UTC
Git commit dd847f1576b37f9da31df51084fa3dda243ccc3c by Ralf Habacker.
Committed on 06/09/2016 at 15:00.
Pushed by habacker into branch 'master'.

Replace usage of UMLObject::m_pUMLPackage by QObject member 'parent' through new method umlParent().

In the past UMLObject::m_pUMLPackage has been used in some classes,
while other uses the QObject class member 'parent'. This commit unify
both approaches by using QObject class member 'parent' in all classes.

class UMLObject now has two new functions setUMLParent() and umlParent()
to provides a UMLObject based interface to the parent uml object;
method umlPackage() is now a shortcut of umlParent()->asUMLPackage().
Reviewed-by: aaron.nottbeck at sag.eu

M  +1    -1    umbrello/codegenerators/ada/adawriter.cpp
M  +1    -1    umbrello/codeimport/import_utils.cpp
M  +1    -1    umbrello/dialogs/umlforeignkeyconstraintdialog.cpp
M  +1    -1    umbrello/dialogs/umloperationdialog.cpp
M  +1    -1    umbrello/dialogs/umluniqueconstraintdialog.cpp
M  +3    -3    umbrello/dialogs/widgets/umldatatypewidget.cpp
M  +2    -2    umbrello/model_utils.cpp
M  +4    -4    umbrello/refactoring/refactoringassistant.cpp
M  +3    -3    umbrello/umldoc.cpp
M  +2    -2    umbrello/umllistview.cpp
M  +6    -6    umbrello/umllistviewitem.cpp
M  +9    -9    umbrello/umlmodel/association.cpp
M  +6    -6    umbrello/umlmodel/attribute.cpp
M  +1    -1    umbrello/umlmodel/checkconstraint.cpp
M  +3    -5    umbrello/umlmodel/classifierlistitem.cpp
M  +2    -2    umbrello/umlmodel/entity.cpp
M  +1    -1    umbrello/umlmodel/entityattribute.cpp
M  +1    -1    umbrello/umlmodel/enumliteral.cpp
M  +2    -2    umbrello/umlmodel/folder.cpp
M  +4    -4    umbrello/umlmodel/foreignkeyconstraint.cpp
M  +4    -4    umbrello/umlmodel/operation.cpp
M  +1    -1    umbrello/umlmodel/template.cpp
M  +50   -24   umbrello/umlmodel/umlobject.cpp
M  +4    -2    umbrello/umlmodel/umlobject.h
M  +8    -7    umbrello/umlmodel/uniqueconstraint.cpp
M  +3    -3    umbrello/umlwidgets/associationwidget.cpp
M  +1    -1    umbrello/umlwidgets/linkwidget.cpp

http://commits.kde.org/umbrello/dd847f1576b37f9da31df51084fa3dda243ccc3c
Comment 4 Ralf Habacker 2016-09-06 15:54:48 UTC
Git commit badc01d753ca0cb46b51a3fd2ed07d265cf9bfed by Ralf Habacker.
Committed on 06/09/2016 at 15:54.
Pushed by habacker into branch 'master'.

Removed obsolate UMLObject::m_pUMLPackage from uml model.

M  +288  -2195 models/UmbrelloArchitecture/umlmodel.xmi

http://commits.kde.org/umbrello/badc01d753ca0cb46b51a3fd2ed07d265cf9bfed
Comment 5 Ralf Habacker 2016-09-06 17:12:44 UTC
Git commit 6d5686f264ddfda0e6bf030e16902569e7dc02df by Ralf Habacker.
Committed on 06/09/2016 at 16:59.
Pushed by habacker into branch 'master'.

Fix crash in TEST_UMLObject::test_fullyQualifiedName() caused by creating uml model related objects on stack.

Because QCOMPARE() returns the function immediatly on error we need to
delete related objects on ourself on exit. This is performed by adding
objects to delete with cleanupOnExit().

M  +15   -12   unittests/TEST_umlobject.cpp
M  +8    -0    unittests/testbase.cpp
M  +4    -0    unittests/testbase.h

http://commits.kde.org/umbrello/6d5686f264ddfda0e6bf030e16902569e7dc02df
Comment 6 Ralf Habacker 2016-09-06 17:12:44 UTC
Git commit 8c6bad8a76df59a11bf67ce62e76c3bda0f12007 by Ralf Habacker.
Committed on 06/09/2016 at 17:08.
Pushed by habacker into branch 'master'.

Fix warning about missing brackets and logic.

M  +2    -2    umbrello/umlmodel/umlobject.cpp

http://commits.kde.org/umbrello/8c6bad8a76df59a11bf67ce62e76c3bda0f12007
Comment 7 Ralf Habacker 2016-09-06 17:12:48 UTC
Git commit 191e52ef5dc2e9a03b3a6c42a6566c4a82630d66 by Ralf Habacker.
Committed on 06/09/2016 at 17:11.
Pushed by habacker into branch 'master'.

Reduce the number of calls to umlPackage() in UMLObject::fullyQualifiedName() by making a local copy.

M  +5    -4    umbrello/umlmodel/umlobject.cpp

http://commits.kde.org/umbrello/191e52ef5dc2e9a03b3a6c42a6566c4a82630d66
Comment 8 Ralf Habacker 2016-09-07 07:07:05 UTC
Git commit d9b4afea8444320ca51f62dc5fa49d4ee2bfa8b3 by Ralf Habacker.
Committed on 07/09/2016 at 07:06.
Pushed by habacker into branch 'master'.

Do not exclude parents of UMLClassifierListItem instances from copying and comparing.

After adding a related testcase it raises the issue that the parent of
UMLClassifierListItem based objects are not been copied. Because we use
the same parent store for all UMLObject based objects now it is possible
to unify this behavior.

M  +3    -3    umbrello/umlmodel/umlobject.cpp
M  +10   -0    unittests/TEST_umlobject.cpp

http://commits.kde.org/umbrello/d9b4afea8444320ca51f62dc5fa49d4ee2bfa8b3
Comment 9 Ralf Habacker 2016-09-08 19:17:39 UTC
Git commit 972a4253237b93dae76b18b6980eb8e8c1464ebf by Ralf Habacker.
Committed on 08/09/2016 at 19:05.
Pushed by habacker into branch 'master'.

Fix regression in type cast migration.

With commit a5f92b67494666e4e6779b1f2443b1a1df2b031d a regression
has been introduced which removes UMLClassifier instances
containing associations from their parent folder while loading
from xmi file. On saving the uml model into a xmi file those
instances are excluded because they are not in the parents object
list (class member UMLPackage::m_objects). After (re)loading such
files some LOST_xxxx items in the tree view indicates this issue.

M  +1    -1    umbrello/umlmodel/classifier.cpp

http://commits.kde.org/umbrello/972a4253237b93dae76b18b6980eb8e8c1464ebf
Comment 10 Ralf Habacker 2016-09-09 09:18:59 UTC
Git commit 120af5d34c2424c2eb4dd631aea925edb227fded by Ralf Habacker.
Committed on 09/09/2016 at 09:18.
Pushed by habacker into branch 'master'.

Complete migration of UMLObject related static_casts to UMLObject::asUML...() wrapper.

M  +1    -1    umbrello/clipboard/umldragdata.cpp

http://commits.kde.org/umbrello/120af5d34c2424c2eb4dd631aea925edb227fded
Comment 11 Ralf Habacker 2016-09-09 09:18:59 UTC
Git commit 3e9f089d6f90085d3e312f3a06d780d9d1c026ff by Ralf Habacker.
Committed on 09/09/2016 at 09:18.
Pushed by habacker into branch 'master'.

Complete migration of dynamic_cast to UMLObject::asUML...() wrapper.

M  +1    -1    umbrello/clipboard/umlclipboard.cpp
M  +1    -1    umbrello/codegenerators/ada/adawriter.cpp
M  +2    -2    umbrello/codegenerators/classifiercodedocument.cpp
M  +1    -1    umbrello/codegenerators/codeclassfield.cpp
M  +2    -2    umbrello/codegenerators/cpp/cppwriter.cpp
M  +1    -1    umbrello/codegenerators/d/dcodeclassfielddeclarationblock.cpp
M  +1    -1    umbrello/codegenerators/java/javacodeclassfielddeclarationblock.cpp
M  +1    -1    umbrello/codegenerators/pascal/pascalwriter.cpp
M  +2    -2    umbrello/codegenerators/ruby/rubycodeclassfielddeclarationblock.cpp
M  +2    -2    umbrello/codegenerators/xml/xmlschemawriter.cpp
M  +2    -2    umbrello/dialogs/umlattributedialog.cpp
M  +2    -2    umbrello/dialogs/umlentityattributedialog.cpp
M  +1    -1    umbrello/dialogs/umlenumliteraldialog.cpp
M  +1    -1    umbrello/dialogs/umltemplatedialog.cpp
M  +3    -3    umbrello/umllistview.cpp
M  +3    -3    umbrello/umlmodel/classifier.cpp
M  +2    -2    umbrello/umlmodel/umlcanvasobject.cpp
M  +2    -2    umbrello/umlmodel/umlobject.cpp
M  +1    -1    umbrello/umlscene.cpp
M  +2    -2    umbrello/umlwidgets/messagewidget.cpp

http://commits.kde.org/umbrello/3e9f089d6f90085d3e312f3a06d780d9d1c026ff
Comment 12 Ralf Habacker 2016-09-11 11:15:14 UTC
Git commit c036a0c04d9c967ba1ad2566eb731a663cc066d3 by Ralf Habacker.
Committed on 11/09/2016 at 11:13.
Pushed by habacker into branch 'master'.

Check return value from dynamic_cast against zero in ClassGeneralPage constructor.

M  +1    -1    umbrello/dialogs/pages/classgeneralpage.cpp

http://commits.kde.org/umbrello/c036a0c04d9c967ba1ad2566eb731a663cc066d3