Bug 202457 - "Next Difference/File" crashes when viewing a "move" diff
Summary: "Next Difference/File" crashes when viewing a "move" diff
Status: REOPENED
Alias: None
Product: kompare
Classification: Applications
Component: general (show other bugs)
Version: 4.1.2
Platform: Ubuntu Linux
: NOR crash
Target Milestone: ---
Assignee: Kompare developers
URL:
Keywords:
: 211456 221544 225543 254274 257835 290454 302273 313813 318860 343535 344532 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-08-04 02:37 UTC by Helder Ribeiro
Modified: 2021-01-01 21:32 UTC (History)
19 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Example patch file for which crashes occured. (30.85 KB, patch)
2009-08-04 02:39 UTC, Helder Ribeiro
Details
New crash information added by DrKonqi (6.25 KB, text/plain)
2010-11-30 23:01 UTC, Raúl
Details
Moving to next file and it crashes (88.31 KB, application/x-gzip)
2012-04-13 10:03 UTC, Vitalie Lazu
Details
New crash information added by DrKonqi (4.31 KB, text/plain)
2013-01-31 20:13 UTC, jckesser
Details
New crash information added by DrKonqi (4.63 KB, text/plain)
2014-02-11 04:54 UTC, eamerritt
Details
New crash information added by DrKonqi (4.43 KB, text/plain)
2021-01-01 21:24 UTC, Raúl
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Helder Ribeiro 2009-08-04 02:37:29 UTC
Version:            (using KDE 4.2.2)
OS:                Linux
Installed from:    Ubuntu Packages

I generated a diff between two git branches with "git diff master...topic_branch > bla.patch". When opening the patch in Kompare, the first difference is a file move, so when I click "next difference" it crashes. Same with "next file". But if I move to a different file through the Source Folder tree navigation, both commands work fine.
Comment 1 Helder Ribeiro 2009-08-04 02:39:01 UTC
Created attachment 35833 [details]
Example patch file for which crashes occured.
Comment 2 Kevin Kofler 2009-08-04 03:31:08 UTC
Confirming, can reproduce with KDE 4.2.4 (Fedora 10 updates).
Comment 3 kde 2009-12-10 18:02:54 UTC
This is a duplicate of Bug 211456
Comment 4 Kevin Kofler 2009-12-11 00:36:44 UTC
*** Bug 211456 has been marked as a duplicate of this bug. ***
Comment 5 kde 2009-12-11 12:08:50 UTC
Ok, as this is now the main bug, have a look at Bug 211456 for a 1kb diff that reproduces the bug.
Comment 6 Dario Andres 2010-01-06 18:55:40 UTC
*** Bug 221544 has been marked as a duplicate of this bug. ***
Comment 7 Dario Andres 2010-02-05 14:52:25 UTC
*** Bug 225543 has been marked as a duplicate of this bug. ***
Comment 8 Kevin Kofler 2010-02-16 00:09:15 UTC
SVN commit 1090752 by kkofler:

Kompare: use QTreeWidget instead of K3ListView in komparenavtreepart.
Also fixes crashes with move diffs (kde#202457).
Patch by Jakub Wieczorek <faw217@gmail.com>.

BUG: 202457
CCMAIL: faw217@gmail.com
CCMAIL: kompare-devel@kde.org

 M  +88 -85    komparenavtreepart.cpp  
 M  +25 -25    komparenavtreepart.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1090752
Comment 9 Kevin Kofler 2010-02-16 00:53:42 UTC
To Jakub Wieczorek (I'll forward this as he doesn't seem to be registered in Bugzilla):

Uh, no, your change didn't fix this bug! I tried backporting this change to 4.4 (but didn't commit) and it still crashes (which is why I didn't commit it). KompareNavTreePart::setSelectedFile still calls currentFile->fillChangesList with a NULL currentFile. Needless to say, this crashes. And I don't think your previous patch (which I didn't backport) fixes that either, at least I don't see how it would.

In addition, your change makes the tree look worse for the testcase attached to this bug. The / and dev/ portion of the tree shows up twice.

Please test with:
kompare https://bugs.kde.org/attachment.cgi?id=35833
and move down to the next diff.
Comment 10 Kevin Kofler 2010-02-16 01:09:14 UTC
I tried again with both patches applied verbatim:
1. this bug is NOT fixed (and thus also NOT fixed in the current trunk), https://bugs.kde.org/attachment.cgi?id=35833 still crashes.
2. the directory tree regression is still there.

So:
1. This bug will have to stay in REOPENED state until it's actually fixed.
2. If Jakub Wieczorek doesn't come up soon with a fix for the regression he introduced, I will be forced to revert the change. (I'd rather have something working using Qt3Support/kde3support than something using the native Qt 4 APIs with regressions. That's why I was reluctant to do this kind of changes in the first place.)

Sorry.
Comment 11 Kevin Kofler 2010-02-16 01:40:21 UTC
So the regression is fixed in revision 1090782, but for this bug we're still back to square one.

I wonder if just checking for a NULL currentFile in setSelectedFile would be enough or if it'd just crash elsewhere then.
Comment 12 Christoph Feck 2010-10-19 14:24:04 UTC
*** Bug 254274 has been marked as a duplicate of this bug. ***
Comment 13 Raúl 2010-11-30 23:01:43 UTC
Created attachment 53925 [details]
New crash information added by DrKonqi

https://bugs.kde.org/show_bug.cgi?id=257835 is actually a dupe of this one. I'm having this following the procedure described above.
Regards,
Comment 14 Kevin Kofler 2011-11-08 17:52:07 UTC
*** Bug 257835 has been marked as a duplicate of this bug. ***
Comment 15 Kevin Kofler 2011-11-08 17:54:22 UTC
Konstantin Tokarev submitted a patch:
http://bugsfiles.kde.org/attachment.cgi?id=61596
which I'm going to apply to trunk and KDE/4.7 (see also my earlier comment #11), hoping it will be sufficient to fix this problem once and for all.
Comment 16 Kevin Kofler 2011-11-08 18:07:11 UTC
Konstantin, your proposed fix does not work:
1. The "Next" click which previously triggered the crash now causes the file list not to get updated even though the diff view moves on to a new file.
2. With a bit more moving back and forth, I still got it to crash! The backtrace is the same (except for the line offset from the added NULL check), I presume that currentFile is now invalid rather than NULL.

The problem is more complicated than just a missing NULL check.
Comment 17 Konstantin Tokarev 2011-11-08 18:09:12 UTC
Removing CC, I'm already on kompare-devel.
Comment 18 Kevin Kofler 2011-11-08 18:23:01 UTC
So there are several things going wrong:
* In order to prevent the crash, in addition to the NULL check you added, I guess KDirLVI::fillFileList should clear modelToFileItemDict. I guess adding that is a good idea for robustness in any case. Otherwise, we end up with nonsensical leaked hash table entries.
* However, that is not the root cause of the bug. The root cause is that we aren't getting the correct directory preselected, which is why the file cannot be found in the mapping (fixable by your NULL check), or a leaked old mapping can be found in some cases (fixable by additionally clearing the dict as I'm suggesting). The code which looks up the correct directory is apparently only looking in the destination and not in the source, which means it fails when the patch is a deletion. I'm still looking for the offending code.
Comment 19 Kevin Kofler 2011-11-08 18:41:02 UTC
Hmmm, the problem with clearing the modelToFileItemDict in KDirLVI::fillFileList is that the function is called twice, once for the source and once for the destination, and apparently the 2 are expected to get aggregated. Let's see, I guess this won't be needed anyway if we can fix the root cause.
Comment 20 Christoph Feck 2012-01-03 01:18:23 UTC
*** Bug 290454 has been marked as a duplicate of this bug. ***
Comment 21 Vitalie Lazu 2012-04-13 10:03:12 UTC
Created attachment 70359 [details]
Moving to next file and it crashes

After working for some files, it crashes
Comment 22 Christoph Feck 2012-06-20 23:50:13 UTC
*** Bug 302273 has been marked as a duplicate of this bug. ***
Comment 23 Christoph Feck 2013-01-25 09:46:34 UTC
Bug 313813 has steps to reproduce.
Comment 24 Christoph Feck 2013-01-25 09:47:08 UTC
*** Bug 313813 has been marked as a duplicate of this bug. ***
Comment 25 jckesser 2013-01-31 20:13:34 UTC
Created attachment 76845 [details]
New crash information added by DrKonqi

kompare (4.1.2) on KDE Platform 4.9.5 using Qt 4.8.4

- What I was doing when the application crashed:

clicking next file / difference while viewing a unified diff generated by hg diff

-- Backtrace (Reduced):
#5  0x00007f5a9261bab9 in KFileLVI::fillChangesList(QTreeWidget*, QHash<Diff2::Difference const*, KChangeLVI*>*) () from /usr/lib64/kde4/komparenavtreepart.so
#6  0x00007f5a9261bddd in KompareNavTreePart::setSelectedFile(Diff2::DiffModel const*) () from /usr/lib64/kde4/komparenavtreepart.so
#7  0x00007f5a9261bff2 in KompareNavTreePart::slotSetSelection(Diff2::DiffModel const*, Diff2::Difference const*) () from /usr/lib64/kde4/komparenavtreepart.so
[...]
#9  0x00007f5a9288de8f in KomparePart::setSelection(Diff2::DiffModel const*, Diff2::Difference const*) () from /usr/lib64/kde4/komparepart.so
#10 0x00007f5a9288f733 in KomparePart::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) () from /usr/lib64/kde4/komparepart.so
Comment 26 Christoph Feck 2013-04-25 18:58:01 UTC
*** Bug 318860 has been marked as a duplicate of this bug. ***
Comment 27 eamerritt 2014-02-11 04:54:34 UTC
Created attachment 85096 [details]
New crash information added by DrKonqi

kompare (4.1.2) on KDE Platform 4.11.4 using Qt 4.8.5

- What I was doing when the application crashed:

Kompare 4.1.2 KDE 4.11.4 Qt 4.8.5

Opened a unified patch containing a diff between two directories, from one of which a file had been deleted.

-- Backtrace (Reduced):
#5  0x00007f2c5fb0b860 in KFileLVI::fillChangesList(QTreeWidget*, QHash<Diff2::Difference const*, KChangeLVI*>*) () from /usr/lib64/kde4/komparenavtreepart.so
#6  0x00007f2c5fb0bb7e in KompareNavTreePart::setSelectedFile(Diff2::DiffModel const*) () from /usr/lib64/kde4/komparenavtreepart.so
#7  0x00007f2c5fb0c55a in KompareNavTreePart::slotSetSelection(Diff2::DiffModel const*, Diff2::Difference const*) () from /usr/lib64/kde4/komparenavtreepart.so
[...]
#10 0x00007f2c601ab34b in KomparePart::setSelection(Diff2::DiffModel const*, Diff2::Difference const*) () from /usr/lib64/kde4/komparepart.so
[...]
#13 0x00007f2c5ff7550b in Diff2::KompareModelList::setSelection(Diff2::DiffModel const*, Diff2::Difference const*) () from /lib64/libkomparediff2.so.4
Comment 28 Kevin Kofler 2015-05-13 15:58:09 UTC
*** Bug 343535 has been marked as a duplicate of this bug. ***
Comment 29 Kevin Kofler 2015-05-13 15:58:39 UTC
*** Bug 344532 has been marked as a duplicate of this bug. ***
Comment 30 Justin Zobel 2020-12-17 05:23:05 UTC
Thank you for the crash report.

As it has been a while since this was reported, can you please test and confirm if this issue is still occurring or if this bug report can be marked as resolved.

I have set the bug status to "needsinfo" pending your response, please change back to "reported" or "resolved/worksforme" when you respond, thank you.
Comment 31 Bug Janitor Service 2021-01-01 04:34:13 UTC
Dear Bug Submitter,

This bug has been in NEEDSINFO status with no change for at least
15 days. Please provide the requested information as soon as
possible and set the bug status as REPORTED. Due to regular bug
tracker maintenance, if the bug is still in NEEDSINFO status with
no change in 30 days the bug will be closed as RESOLVED > WORKSFORME
due to lack of needed information.

For more information about our bug triaging procedures please read the
wiki located here:
https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

If you have already provided the requested information, please
mark the bug as REPORTED so that the KDE team knows that the bug is
ready to be confirmed.

Thank you for helping us make KDE software even better for everyone!
Comment 32 eamerritt 2021-01-01 05:23:29 UTC
(In reply to Bug Janitor Service from comment #31)
> Dear Bug Submitter,
> 
> This bug has been in NEEDSINFO status with no change for at least
> 15 days. Please provide the requested information as soon as
> possible and set the bug status as REPORTED. Due to regular bug
> tracker maintenance, if the bug is still in NEEDSINFO status with
> no change in 30 days the bug will be closed as RESOLVED > WORKSFORME
> due to lack of needed information.
> 
> For more information about our bug triaging procedures please read the
> wiki located here:
> https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging
> 
> If you have already provided the requested information, please
> mark the bug as REPORTED so that the KDE team knows that the bug is
> ready to be confirmed.
> 
> Thank you for helping us make KDE software even better for everyone!


Sorry, no idea.  That was many years and several versions of KDE ago.
A quick try of the described recipe on my current desktop shows
no problem (kompare-19.04.0 kde 5.15
Comment 33 Raúl 2021-01-01 21:24:45 UTC
Created attachment 134446 [details]
New crash information added by DrKonqi

kompare (4.1.20120) using Qt 5.15.2

- What I was doing when the application crashed:
Debian sid. kompare 20.12.0, Qt 5.12.2
I reproduced like this: I used the most recent attached diff:
* curl https://bugsfiles.kde.org/attachment.cgi?id=70359 |gunzip > kompare_crash.diff
* kompare kompare_crash.diff
* Go to first file in diff
* Start browsing using ctrl-pgup, ctrl-pgdown

-- Backtrace (Reduced):
#4  QListData::begin (this=<error reading variable: Cannot access memory at address 0x40>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:118
#5  QList<Diff2::Difference*>::constBegin (this=<error reading variable: Cannot access memory at address 0x40>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:341
#6  KFileLVI::fillChangesList (this=0x0, changesList=0x55c63834cdb0, diffToChangeItemDict=0x55c638324590) at ./komparenavtreepart/komparenavtreepart.cpp:627
#7  0x00007fe7bcb1767d in KompareNavTreePart::setSelectedFile (this=0x55c638324560, model=0x55c638a5afd0) at ./komparenavtreepart/komparenavtreepart.cpp:339
#8  0x00007fe7bcb17a6a in KompareNavTreePart::slotSetSelection (this=0x55c638324560, model=0x55c638a5afd0, diff=0x55c638a66170) at ./komparenavtreepart/komparenavtreepart.cpp:299
Comment 34 Raúl 2021-01-01 21:32:47 UTC
Start browsing from a/Gemfile and press ctrl-PgUp until you get the crash.
Valgrind report (excerpt):

==25494== Invalid read of size 8
==25494==    at 0x118793AC: UnknownInlinedFun (qlist.h:118)
==25494==    by 0x118793AC: constBegin (qlist.h:341)
==25494==    by 0x118793AC: KFileLVI::fillChangesList(QTreeWidget*, QHash<Diff2::Difference const*, KChangeLVI*>*) (komparenavtreepart.cpp:627)
==25494==    by 0x1187967C: KompareNavTreePart::setSelectedFile(Diff2::DiffModel const*) (komparenavtreepart.cpp:339)
==25494==    by 0x11879A69: KompareNavTreePart::slotSetSelection(Diff2::DiffModel const*, Diff2::Difference const*) (komparenavtreepart.cpp:299)
==25494==    by 0x624653F: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.15.2)
==25494==    by 0x11848E0D: KomparePart::setSelection(Diff2::DiffModel const*, Diff2::Difference const*) (moc_kompare_part.cpp:400)
==25494==    by 0x6246505: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.15.2)
==25494==    by 0x489368D: Diff2::KompareModelList::setSelection(Diff2::DiffModel const*, Diff2::Difference const*) (in /usr/lib/x86_64-linux-gnu/libkomparediff2.so.5.2)
==25494==    by 0x489A442: Diff2::KompareModelList::slotNextModel() (in /usr/lib/x86_64-linux-gnu/libkomparediff2.so.5.2)
==25494==    by 0x6246505: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.15.2)
==25494==    by 0x5372B61: QAction::triggered(bool) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.15.2)
==25494==    by 0x53753A0: QAction::activate(QAction::ActionEvent) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.15.2)
==25494==    by 0x5375F66: QAction::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.15.2)
==25494==  Address 0x40 is not stack'd, malloc'd or (recently) free'd
==25494== 
KCrash: crashing... crashRecursionCounter = 2
KCrash: Application Name = kompare path = /usr/bin pid = 25494
KCrash: Arguments: /usr/bin/kompare kompare_crash.diff 
KCrash: Attempting to start /usr/lib/x86_64-linux-gnu/libexec/drkonqi