D10543: fix crashing with duplicate entries
This revision was automatically updated to reflect the committed changes. Closed by commit R243:6736aca49c2d: karchive, kzip: try to handle duplicate files in a bit nicer way (authored by sandsmark). REPOSITORY R243 KArchive CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27668&id=29009 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp src/kzip.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
dfaure accepted this revision. This revision is now accepted and ready to land. REVISION DETAIL https://phabricator.kde.org/D10543 To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark updated this revision to Diff 27668. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27664&id=27668 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp src/kzip.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark updated this revision to Diff 27664. sandsmark marked 7 inline comments as done. sandsmark added a comment. fixes to issues raised REPOSITORY R243 KArchive CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27286&id=27664 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp src/kzip.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks for the fix!
INLINE COMMENTS
> karchive.cpp:471
> } else {
> -//qCWarning(KArchiveLog) << "Found" << path << "but it's not a
> directory";
> +qCWarning(KArchiveLog) << "Found" << path << "but it's not a
> directory";
> +if (ent->isFile()) {
Please remove this warning, it's ambiguous at this point what the problem is;
better have only one clear warning once we know which case we're in.
> karchive.cpp:472
> +qCWarning(KArchiveLog) << "Found" << path << "but it's not a
> directory";
> +if (ent->isFile()) {
> +const KArchiveFile *file = static_cast *>(ent);
You can remove this if(), every entry is either a file or a directory, by
design.
And the if() confused me, e.g. if it was false, the code below would still talk
about an empty file ;). But it can't ever be false, so let's just remove that
if().
> karchive.cpp:475
> +if (file->size() > 0) {
> +qCWarning(KArchiveLog) << "It's a normal file, bailing
> out";
> +return nullptr;
Obviously that means mentioning "path" in this warning, something about that
file being a duplicate.
> karchive.cpp:480
> +
> +qCWarning(KArchiveLog) << "It's an empty file, assuming it is
> actually a directory and replacing";
> +KArchiveEntry *myEntry = const_cast(ent);
If this can happen, since there's no data loss, maybe a qCDebug is enough?
(this also needs to mention "path" of course)
> kzip.cpp:756
> +} else {
> +setErrorString(tr("File %1 is in folder %2, but %3
> is actually a file.").arg(entryName).arg(path).arg(path));
> +delete entry;
Use multi-arg to avoid issues in case one of these strings contains %1.
.arg(entryName, path, path)
REPOSITORY
R243 KArchive
REVISION DETAIL
https://phabricator.kde.org/D10543
To: sandsmark, dfaure, #frameworks
Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark updated this revision to Diff 27286. sandsmark added a comment. an actual fix, which passes all unit tests. REPOSITORY R243 KArchive CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27281&id=27286 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp src/kzip.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark added a dependency: D10551: autotest for crashing in KArchiveDirectory::addEntry. REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D10543 To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark updated this revision to Diff 27281. sandsmark added a comment. prettify REPOSITORY R243 KArchive CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27276&id=27281 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
apol added inline comments.
INLINE COMMENTS
> karchive.cpp:829
> {
> -if (d->entries.value(entry->name())) {
> -/*qCWarning(KArchiveLog) << "directory " << name()
> -<< "has entry" << entry->name() << "already";*/
> -delete entry;
> +if (d->entries.value(entry->name()) == entry) {
> +qCWarning(KArchiveLog) << "directory " << name()
Cache the value not to query twice?
REPOSITORY
R243 KArchive
REVISION DETAIL
https://phabricator.kde.org/D10543
To: sandsmark, dfaure, #frameworks
Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark updated this revision to Diff 27276. sandsmark added a comment. handle stuff a bit prettier. I don't think there's a "right" way to handle this, so parsing is a bit best-effort, but at least it doesn't crash anymore. REPOSITORY R243 KArchive CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27275&id=27276 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark updated this revision to Diff 27275. sandsmark added a comment. actually do the correct thing REPOSITORY R243 KArchive CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10543?vs=27254&id=27275 REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
apol added inline comments. INLINE COMMENTS > karchive.cpp:832-833 > +<< "has entry" << entry->name() << "already"; > +delete d->entries.take(entry->name()); > return; > } Haven't looked at the code yet, but this doesn't make much sense. Since it's already in the map, we remove it from the map? Maybe this should remove the `return` then. REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D10543 To: sandsmark, dfaure, #frameworks Cc: apol, michaelh
D10543: fix crashing with duplicate entries
sandsmark created this revision. sandsmark added reviewers: dfaure, Frameworks. Restricted Application added a project: Frameworks. sandsmark requested review of this revision. REVISION SUMMARY some zip files apparently can have duplicate entry names, which weren't handled correctly. TEST PLAN created a unit test in a separate commit. REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D10543 AFFECTED FILES src/karchive.cpp To: sandsmark, dfaure, #frameworks Cc: michaelh
