-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114048/
-----------------------------------------------------------
(Updated Nov. 28, 2013, 1:05 a.m.)
Status
------
This change has been marked as submitted.
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
