Differential D20519

Fix double delete on broken files
ClosedPublic
Actions

Authored by aacid on Apr 13 2019, 10:21 PM.

Details

Reviewers

dfaure

Commits

R243:b401e24093d8: Fix double delete on broken files

Summary

findOrCreate has some code that tries to recover broken files.

That code will delete an empty file if later it looks like it should be a directory.

The problem is that that code, was doing

entry = rootDir->get(path)
if (entry is not dir and it's 0 size) {
rootDir->remove(entry)
delete entry;
}

But unfortunately get() and remove() are not equally stubborn.

get will try to see if the path lives in a subdir while remove only will remove entries in the exact dir.

So it might happen that the old code did call remove() on the root dir but that did nothing since the entry didn't belong to the root dir.

This failure would result in a double delete when the dir that actually contains the entry would be deleted.

This patch introduces a private get that will also return which directory entry exactly lives on so remove succeeds and no double delete happens.

Diff Detail

Repository

R243 KArchive

Branch

arcpatch-D20519

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 10854
Build 10872: arc lint + arc unit

aacid created this revision.Apr 13 2019, 10:21 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptApr 13 2019, 10:21 PM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

aacid requested review of this revision.Apr 13 2019, 10:21 PM

Harbormaster completed remote builds in B10815: Diff 56161.Apr 13 2019, 10:21 PM

aacid added a subscriber: dfaure.Apr 13 2019, 10:22 PM

Good catch.
I'd still prefer if such changes would come with a unittest, because it will be hard otherwise to immediately detect (while developing) that changes don't introduce any regressions.
I managed to write a unittest for this one, please integrate it into your commit:

http://www.davidfaure.fr/2019/ktar_unittest.diff
http://www.davidfaure.fr/2019/tar_emptyfile_missingdir.tar.gz

The tar file was created like this (this info should go into the commit log)

mkdir dir
touch dir/foo
tar cf tar_emptyfile_missingdir.tar dir/foo
rm -f dir/foo
mkdir dir/foo
touch dir/foo/file
tar rf tar_emptyfile_missingdir.tar dir/foo/file
gzip tar_emptyfile_missingdir.tar

src/karchive.cpp
77 ↗	(On Diff #56161)	A cleaner solution is to return a small struct with the two things we want to return. This would also remove the need for a "dummy" variable.

This revision now requires changes to proceed.Apr 14 2019, 9:48 AM

In D20519#449595, @dfaure wrote:

Good catch.
I'd still prefer if such changes would come with a unittest, because it will be hard otherwise to immediately detect (while developing) that changes don't introduce any regressions.
I managed to write a unittest for this one, please integrate it into your commit:

http://www.davidfaure.fr/2019/ktar_unittest.diff

QCOMPARE(listing[0], QString("mode=40777 path=dir type=dir"));
QCOMPARE(listing[1], QString("mode=40777 path=dir type=dir"));
QCOMPARE(listing.count(), 1);

seems wrong, it should either be count = 2 or the second qcompare should go away, no?

also, the listing variable it's empty when i run that test here.

update with test from dfaure + local tweak

Harbormaster completed remote builds in B10854: Diff 56218.Apr 14 2019, 4:20 PM

Oops indeed, I stopped working on the unittest when I managed to make it reproduce the crash. Thanks for integrating it and fixing it.

Any input on my "cleaner solution: return a two-field structs" suggestion?

In D20519#450066, @dfaure wrote:

Any input on my "cleaner solution: return a two-field structs" suggestion?

I can do it if you want, but i feel that for a function that is private and called in one just place the "new" syntax is not that terrible.

The double star looks very C-ish to me, but OK, I just realized that the recursive call itself would probably look more cumbersome, so I won't insist.

src/karchive.cpp
871	Two places -- this is the second one, the one that looks not so great.

This revision is now accepted and ready to land.Apr 14 2019, 6:05 PM

aacid closed this revision.Apr 14 2019, 7:14 PM

Revision Contents
Changeset List

			Path
M			autotests/karchivetest.h (1 line)
M			autotests/karchivetest.cpp (18 lines)
A	M	(bin)	autotests/tar_emptyfile_missingdir.tar.gz
M			src/karchive.cpp (131 lines)
M			src/karchivedirectory.h (1 line)