Review Request 114048: fix crash in KZip on overwriting existing entries (+ unit test)

[Albert Astals Cid]

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

Ship it!


Your explanation makes sense, code is clear and valgrind complains in the test without the patch and doesn't complain with the patch.

So looks pretty solid to me. Of course if dfaure reviews it'd be better, but i don't see how we can regress with this patch.

Please apply to 4.11 and then merge to 4.12 and then merge 4.12 to master.

- Albert Astals Cid


On Nov. 22, 2013, 11:02 p.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114048/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2013, 11:02 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Bugs: 321100
>     http://bugs.kde.org/show_bug.cgi?id=321100
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> CRASH REASON
> ------------
> In case of writing a file to a KZip object with a name for which there already exists an entry with the same name, the following will happen in KZip::doPrepareWriting(...):
> when the existing entry in d->m_fileList is found, it is removed from d->m_fileList and deleted.
> 
> The problem is that each entry is also listed in the KArchiveDirectory it is contained in by the path derived from its name. And KArchiveDirectory is the one which does the lifetime handling of the entries, by a qDeleteAll(entries) in the private data destructor.
> But in the above point it is forgotten to remove the entry from the KArchiveDirectory it was listed in. So KArchiveDirectory keeps a pointer to a no-longer existing entry. Which should result in a crash on that qDeleteAll. But in many situations something accidentally prevents that, see soon.
> 
> So after the old entry was removed and deleted, a new KZipFileEntry is created, then added both to d->m_fileList and the parentDir, by calling addEntry(...) on that. Now KArchiveDirectory::addEntry(...) checks if there is already an entry with such a name listed, and if so, simply emits a warning and returns without doing anything. In our case it will do so, because the old entry, with the same name, was not removed, so the new entry will not be added.
> 
> Means, we have a pointer to a non-existing entry and an entry which will not be cleaned up. Strange enough for users of KZip like Krita, which happened to accidentally write the same entry two times, a crash was not always experienced. Adding some debug output shows why: the new KZipFileEntry often gets exactly the memory assigned which was before assigned to the old entry that was just deleted.
>   // first write of file
>   KZip::doPrepareWriting:
>   KZip::doPrepareWriting: created 0x1cfbc20
>   KArchiveDirectory::addEntry: entry= 0x1cfbc20 name= "samefile"
>   // second write of file
>   KZip::doPrepareWriting:
>   KZip::doPrepareWriting: deleting 0x1cfbc20
>   KZip::doPrepareWriting: created 0x1cfbc20
>   KArchiveDirectory::addEntry: entry= 0x1cfbc20 name= "samefile"
>   KArchiveDirectory::addEntry: directory  "/" has entry "samefile" already 
>   KArchiveDirectory::~KArchiveDirectory: "/"
> 
> So the old entry in KArchiveDirectory was pointing to the new entry again, thus not resulting in any problem.
> 
> But often enough that does not happen, so is assumed to lead to https://bugs.kde.org/show_bug.cgi?id=321100 because, as said before, Krita happened to accidentally write the same entry two times into the zip on saving .kra files.
> 
> PATCH
> -----
> Attached patch fixes that by removing the old entry also properly from the KArchiveDirectory it is registered with before deleting it. The retrieval of the parent dir has been removed before the duplication check for that reason, so that the parent directory object is available when needed.
> 
> There is also a small unit test which tests behaviour on writing the same file two times into a zip.
> 
> 
> DESTINATION?
> ------------
> If okayed, to which branches should that be applied? master, KDE/4.11, KDE/4.12?
> Who could care to get this into KF5?
> 
> 
> Diffs
> -----
> 
>   kdecore/io/karchive.h 7cd7c0c 
>   kdecore/io/karchive.cpp 88e1de0 
>   kdecore/io/kzip.cpp d5b0146 
>   kdecore/tests/karchivetest.h 29dc791 
>   kdecore/tests/karchivetest.cpp 8a5b9f3 
> 
> Diff: http://git.reviewboard.kde.org/r/114048/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

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