Review Request 113965: Possible fix for bug 321100

[Friedrich W. H. Kossebau]

> On Nov. 20, 2013, 6:02 p.m., Albert Astals Cid wrote:
> > I don't see why this should fix anything. Do you think anyone in the bug can provide a valgrind trace to better understand why it's crashing?
> 
> Christoph Feck wrote:
>     See also https://git.reviewboard.kde.org/r/102981/ which has some discussion and links to related bugs.
> 
> Albert Astals Cid wrote:
>     Why is it related? Who is modifying the entries variable? It's used in 4 places in the file, and actually there's no way to remove stuff from it, so I don't see how it is related to the bug you point.
> 
> Christoph Feck wrote:
>     They are only related because replacing qDeleteAll() with manual deletion fixed the crash for Boudewijn. Since my understanding ended there, I suggested to post a review request.
> 
> Thomas Lübking wrote:
>     See http://lists.kde.org/?t=132194778400005&r=1&w=2
>     Qt 4.8 changed qDeleteAll to rely on the container being immutable.
>     
>     The patch detaches the container, what allows safe operation despite - what's likely happening as it seemed the core issue back than - the container is altered by the deletion of one or more of its items (eg. the items deconstructor delists itself)
>     
>     An alternative solution would be to pass the to-be-deleted objects class a static member flag to skip self delisting and set that around qDeleteAll() (which would be followed by an erase())
> 
> Albert Astals Cid wrote:
>     Please look at the code and tell us where the item deconstructor delists itself from the list.
> 
> Thomas Lübking wrote:
>     I said that it seemed the core issue back then, not that it happens here (for sure) or would be the only trigger.
>     
>     Briefly looking at KArchive, i'd rather bet on a recursion (entries containing a KArchiveEntry being or ultimately pointing down to the same  KArchiveDirectory)
>     Just a wild guess, though - but testable if one can reproduce the bug (unless you can assure that cannot be the case)
> 
> Boudewijn Rempt wrote:
>     I haven't been able to reproduce it myself on Linux. On Windows it was a lot easier, but there I use an old and completely hacked-up version of kdelibs. However, when I was giving a workshop at LGM, pretty much half of the Linux users present (most of them *buntu) users had to disable autosave to prevent this crash from happening.
>     
>     I'm puzzled... And I would love a better fix than mine, but that will have to come from someone who understands karchive -- which I don't, not really.
> 
> Friedrich W. H. Kossebau wrote:
>     I do not have a better fix yet, but I think I found the root of the problem:
>     in case of a duplicated name for an entry in KZip::doPrepareWriting() the old entry is removed from d->m_fileList, but not from the parentDir directory holding it:
>     https://projects.kde.org/projects/kde/kdelibs/repository/revisions/ee5dea835039c8a8f765321378dbed398826f368/entry/kdecore/io/kzip.cpp#L1026
>     
>     Thus on destruction of that dir the qDeleteAll will try to delete an entry that is already deleted. Seems that triggers not always a crash, perhaps because the memory might still be available?
>     
>     Sadly I do not have a kdelibs development environment setup currently and also short of disk space, so cannot come up with a patch/unittest. Anyone? But so far I already see the problem that KArchiveDirectory has a method "addEntry( KArchiveEntry* )" (which currently in case of adding an entry with the same name qwarns about that, and ignores the new entry), but not some "removeEntry( KArchiveEntry* )". This needs some more thinking what the proper behaviour should be and how to solve that. Perhaps "addEntry( KArchiveEntry* )" should just overwrite the old entry, that would at least match the behaviour in KZip::doPrepareWriting()...
>     
>     Any takers?

Took it myself ;)

Please see & review https://git.reviewboard.kde.org/r/114048/ for a proposal how to fix that problem in KZip::doPrepareWriting().


- Friedrich W. H.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113965/#review44060
-----------------------------------------------------------


On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113965/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2013, 9:40 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Bugs: 321100
>     http://bugs.kde.org/show_bug.cgi?id=321100
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> While I haven't been able to reproduce the issue on Linux, many Krita users encounter a crash in the destructor of  KArchiveDirectoryPrivate, where all entries are removed with qDeleteAll.
> 
> This patch removes the use of qDeleteAll.
> 
> I'm not 100% sure whether this is correct, but it works for me with the Windows build of kdelibs I use.
> 
> 
> Diffs
> -----
> 
>   kdecore/io/karchive.cpp 88e1de0 
> 
> Diff: http://git.reviewboard.kde.org/r/113965/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Boudewijn Rempt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20131122/e00450b7/attachment.htm>