----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114048/#review44645 -----------------------------------------------------------
This review has been submitted with commit fa0be8a39623b3bc18454895b4008440f6e4ed0d by Friedrich W. H. Kossebau to branch KDE/4.11. - Commit Hook 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 > >
