Bug 306785 - Reproducible segfault when trying to create a synchronous message
Summary: Reproducible segfault when trying to create a synchronous message
Status: RESOLVED FIXED
Alias: None
Product: umbrello
Classification: Applications
Component: general (show other bugs)
Version: SVN
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Umbrello Development Group
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-14 10:26 UTC by J.H.M. Dassen (Ray)
Modified: 2013-06-25 18:54 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.10.4
Sentry Crash Report:


Attachments
Umbrello model file used in reproducer (45.66 KB, text/x-xmi)
2012-09-14 10:27 UTC, J.H.M. Dassen (Ray)
Details
gdb backtrace of crash (27.82 KB, application/x-xz)
2012-09-14 10:46 UTC, J.H.M. Dassen (Ray)
Details
simple test case for this kind of problem (20.51 KB, text/x-xmi)
2013-05-28 16:43 UTC, Ralf Habacker
Details
updated testcase (26.28 KB, application/x-uml)
2013-05-30 13:21 UTC, Ralf Habacker
Details
Proposed fix (3.54 KB, patch)
2013-05-31 10:27 UTC, Martin Bříza
Details
fix recursive traversing (3.71 KB, patch)
2013-06-03 06:32 UTC, Ralf Habacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description J.H.M. Dassen (Ray) 2012-09-14 10:26:54 UTC
This issue was originally reported against kdesdk-4.3.4-4.el6.x86_64 in Red Hat Enterprise Linux 6, but proved reproducible with current upstream sources (SVN r1314540 built on Fedora 17).

Reproducible: Always

Steps to Reproduce:
Original reproducer steps:
1. Open the attached model it in Umbrello on RHEL6.
2. Go to the "Set diagnostic setting sequence" tab.
3. Click on "Synchronous Message" in the Diagram toolbar.
4. Add the message between the "SetDiagnosticSettingObject" and the 
   "AbstractLogging" objects by first clicking on the end of the dashed line 
   coming from "SetDiagnosticSetting", and then clicking on dashed line coming 
   from "AbstractLogging".

With SVN HEAD, there is a step 0 of clicking "OK" twice on popups about a naming conflict.
Actual Results:  
Segfault.

Expected Results:  
Synchronous message is created.
Comment 1 J.H.M. Dassen (Ray) 2012-09-14 10:27:29 UTC
Created attachment 73905 [details]
Umbrello model file used in reproducer
Comment 2 J.H.M. Dassen (Ray) 2012-09-14 10:46:24 UTC
Created attachment 73906 [details]
gdb backtrace of crash
Comment 3 Ralf Habacker 2013-05-28 04:17:14 UTC
Git commit 72ee9088d4b8f3f9dd1bef2094c07772db9d0be7 by Ralf Habacker.
Committed on 28/05/2013 at 02:22.
Pushed by habacker into branch 'master'.

Fixed "duplicate entry" error.

M  +2    -1    umbrello/widgets/messagewidget.cpp

http://commits.kde.org/umbrello/72ee9088d4b8f3f9dd1bef2094c07772db9d0be7
Comment 4 Ralf Habacker 2013-05-28 04:21:35 UTC
Git commit ba9c3b19f89eac16cee8d8215bf81e5dd99c730c by Ralf Habacker.
Committed on 28/05/2013 at 04:58.
Pushed by habacker into branch '306785'.

Fixed crash on adding sequence diagram messages by breaking recursive inheritance loops.

Thanks to Martin Briza for pointing to this bug.

M  +9    -2    umbrello/classifier.cpp
M  +1    -1    umbrello/classifier.h

http://commits.kde.org/umbrello/ba9c3b19f89eac16cee8d8215bf81e5dd99c730c
Comment 5 Ralf Habacker 2013-05-28 04:29:36 UTC
Git commit 2961f865229f9aaacc61de40b6f6999373ff022d by Ralf Habacker.
Committed on 28/05/2013 at 04:58.
Pushed by habacker into branch 'master'.

Fixed crash on adding sequence diagram messages by breaking recursive inheritance loops.

Thanks to Martin Briza for pointing to this bug.

M  +9    -2    umbrello/classifier.cpp
M  +1    -1    umbrello/classifier.h

http://commits.kde.org/umbrello/2961f865229f9aaacc61de40b6f6999373ff022d
Comment 6 Ralf Habacker 2013-05-28 16:43:43 UTC
Created attachment 80129 [details]
simple test case for this kind of problem
Comment 7 Ralf Habacker 2013-05-30 08:12:52 UTC
Git commit e5f6b455e70f610171ba1abea790eae3f1cf7ea7 by Ralf Habacker.
Committed on 28/05/2013 at 04:58.
Pushed by habacker into branch 'KDE/4.10'.

Fixed crash on adding sequence diagram messages by breaking recursive inheritance loops.

Thanks to Martin Briza for pointing to this bug.

M  +9    -2    umbrello/classifier.cpp
M  +1    -1    umbrello/classifier.h

http://commits.kde.org/umbrello/e5f6b455e70f610171ba1abea790eae3f1cf7ea7
Comment 8 Ralf Habacker 2013-05-30 08:26:43 UTC
Git commit 82e817e96f2eae23b68a6bc2c48958be3321a15c by Ralf Habacker.
Committed on 28/05/2013 at 02:22.
Pushed by habacker into branch 'KDE/4.10'.

Fixed "duplicate entry" error.

M  +2    -1    umbrello/widgets/messagewidget.cpp

http://commits.kde.org/umbrello/82e817e96f2eae23b68a6bc2c48958be3321a15c
Comment 9 Martin Bříza 2013-05-30 09:09:52 UTC
Thanks for the fix, Ralf. 
I was thinking about it and I'm quite sure this isn't a complete solution, though, as I think there's still a possibility there is a cycle in the graph somewhere deeper than returning to the root as both your and my patches expect.
Completely solving this bug would involve creating a QSet of all previously processed nodes and checking if a particular node was already traversed through. Or a similar approach.
Comment 10 Ralf Habacker 2013-05-30 13:21:14 UTC
Created attachment 80179 [details]
updated testcase

(In reply to comment #9)
> Thanks for the fix, Ralf. 
> I was thinking about it and I'm quite sure this isn't a complete solution,
> though, as I think there's still a possibility there is a cycle in the graph
> somewhere deeper than returning to the root as both your and my patches
> expect.
I agree,  see appended test case. Try to add a message from "neue_klasse" to "neue_klasse5"

> Completely solving this bug would involve creating a QSet of all previously
> processed nodes and checking if a particular node was already traversed
> through. Or a similar approach.
patches welcome :-)
Comment 11 Ralf Habacker 2013-05-30 14:03:52 UTC
Git commit 01fa09b652cba8044b36ac1eeadcc716c6a5c272 by Ralf Habacker.
Committed on 30/05/2013 at 16:02.
Pushed by habacker into branch 'master'.

Enable guard against resursive inheritance loops to self.

M  +1    -1    umbrello/dialogs/selectopdlg.cpp

http://commits.kde.org/umbrello/01fa09b652cba8044b36ac1eeadcc716c6a5c272
Comment 12 Ralf Habacker 2013-05-30 14:34:14 UTC
Git commit 4b69830b26c81ae280ce3db030ba00315b3ce141 by Ralf Habacker.
Committed on 30/05/2013 at 16:02.
Pushed by habacker into branch 'KDE/4.10'.

Enable guard against resursive inheritance loops to self.

M  +1    -1    umbrello/dialogs/selectopdlg.cpp

http://commits.kde.org/umbrello/4b69830b26c81ae280ce3db030ba00315b3ce141
Comment 13 Ralf Habacker 2013-05-30 14:36:30 UTC
For now I disabled the display of inherited methods.
Comment 14 Martin Bříza 2013-05-31 10:27:32 UTC
Created attachment 80211 [details]
Proposed fix

Hi, I wrote the patch as I proposed in my previous comment. I'm still inclined to use a stack (in a QList) instead of recursion. Maybe it's not as elegant but it at least isn't limited by the stack frame... and it makes adding the set easier.
It reverts your yesterday's changes too, I suppose you'll want to revert them yourself in different commits.
Comment 15 Ralf Habacker 2013-06-03 06:32:47 UTC
Created attachment 80271 [details]
fix recursive traversing

I extended your latest patch with an experimental support for recursive traversing, with which the testcases do not crash anymore and the list of operations includes all inherited methods. [1]
If you still like to implement a linear search instead of recursive search feel free . 

[1] Just got the impression that it may be a good to prefix inherited method with the related class name.
Comment 16 Martin Bříza 2013-06-03 10:07:06 UTC
Ralf, 
it doesn't really matter to me, I'm just trying to use recursion as less as possible because of the risk of overflowing the stack. If you're satisfied with the patch, I agree with applying it.
Thank you,
Martin
Comment 17 Ralf Habacker 2013-06-03 10:34:10 UTC
Git commit b939eb245482ff8a436003169baae8f240bccdbb by Ralf Habacker.
Committed on 03/06/2013 at 07:13.
Pushed by habacker into branch 'master'.

Prevent recursive search loops.

M  +34   -12   umbrello/classifier.cpp
M  +2    -1    umbrello/classifier.h

http://commits.kde.org/umbrello/b939eb245482ff8a436003169baae8f240bccdbb
Comment 18 Ralf Habacker 2013-06-03 11:31:13 UTC
Git commit 44b93882e8a99c6ec9c25cdff198a7679a711c4f by Ralf Habacker.
Committed on 03/06/2013 at 13:30.
Pushed by habacker into branch 'master'.

Reenable including of inherited methods on adding messages to sequence diagram.

M  +1    -1    umbrello/dialogs/selectopdlg.cpp

http://commits.kde.org/umbrello/44b93882e8a99c6ec9c25cdff198a7679a711c4f
Comment 19 Ralf Habacker 2013-06-25 18:54:42 UTC
Git commit b8455e6a66f140903060e8f911ff0315c7aa98a7 by Ralf Habacker.
Committed on 03/06/2013 at 05:13.
Pushed by habacker into branch 'KDE/4.10'.

Prevent recursive search loops.

M  +34   -12   umbrello/classifier.cpp
M  +2    -1    umbrello/classifier.h

http://commits.kde.org/umbrello/b8455e6a66f140903060e8f911ff0315c7aa98a7
Comment 20 Ralf Habacker 2013-06-25 18:54:44 UTC
Git commit d090de300ee303e94a028351feac6e7460a444d9 by Ralf Habacker.
Committed on 03/06/2013 at 11:30.
Pushed by habacker into branch 'KDE/4.10'.

Reenable including of inherited methods on adding messages to sequence diagram.

M  +1    -1    umbrello/dialogs/selectopdlg.cpp

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