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

Thu Nov 28 01:05:28 GMT 2013

-----------------------------------------------------------
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

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