Differential D18562

Determine isDirectory by "Folder = " field in 7z plugin
ClosedPublic
Actions

Authored by gepardo on Jan 27 2019, 8:53 AM.

Details

Reviewers
elvisangelaccio
Commits
R36:ca4667adf1cd: Determine isDirectory by "Folder = " field in 7z plugin
Summary

After looking at 7zip sources, I've found that there are two types of file attributes:

  • FAT/Windows (e. g. "RA", "RDA")
  • Unix (e. g. "_ -rwxr-xr-x" or "D_ drwxr-xr-x")

Earlier, isDirectory property was determined by "Attributes = " field. But this is incorrect, when 7z returns strings like

"Attributes = RDA"

This is a directory, but the code searched for "D" in the beginning and failed. Due to that, the archive couldn't be displayed properly.

I used a more canonical way to determine isDirectory: parsing "Folder = " field in 7z's output. Also reworked handling "Attributes = " field to handle both cases correctly.

Test Plan

Rebuilt ark and tried on some zip archives (also on the archive that ark was unable to display properly). Everything works OK.

Diff Detail

Repository
R36 Ark
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gepardo created this revision.Jan 27 2019, 8:53 AM
Restricted Application added a project: Ark. · View Herald TranscriptJan 27 2019, 8:53 AM
Restricted Application added subscribers: Ark, kde-utils-devel. · View Herald Transcript
gepardo requested review of this revision.Jan 27 2019, 8:53 AM
elvisangelaccio added a subscriber: elvisangelaccio.Jan 28 2019, 4:27 PM

Thanks. Can you share a test archive that works with this patch but doesn't without?

gepardo added a comment.Jan 29 2019, 8:41 PM

Thanks. Can you share a test archive that works with this patch but doesn't without?

After my attempt to modify the original archive on which I got this bug, I created this one. Without the patch, the bug is reproducible on this archive.

Note that 7zip plugin must be used to reproduce, so p7zip must be installed.

ark-18.08-bug.zip730 BDownload

gepardo added a comment.Feb 3 2019, 7:46 PM

Any review on this patch? It's waiting the review for a week.

elvisangelaccio added a comment.Feb 3 2019, 8:32 PM

I'll review it soon. I want to add a test case with your test archive first; feel free to beat me to it :)

elvisangelaccio added a comment.EditedFeb 10 2019, 6:05 PM

Sorry but this patch breaks the following tests:

The following tests FAILED:
          4 - kerfuffle-deletetest (Child aborted)
          7 - kerfuffle-addtest (Child aborted)
          8 - kerfuffle-movetest (Child aborted)
          9 - kerfuffle-copytest (Child aborted)
         16 - plugins-cli7ztest (Child aborted)

Please run ctest after building ark.

elvisangelaccio added inline comments.Feb 10 2019, 6:08 PM
plugins/cli7zplugin/cliplugin.cpp
251–252

This will crash if line is equal to Folder = +.

elvisangelaccio added a comment.Feb 10 2019, 6:14 PM

Failing test case added with 95bf2244e5b4c9fe7901fd50dd662dd514439407 (thanks for the test archive!)

elvisangelaccio requested changes to this revision.Feb 10 2019, 6:15 PM
This revision now requires changes to proceed.Feb 10 2019, 6:15 PM
gepardo updated this revision to Diff 51338.Feb 10 2019, 6:36 PM

Remove the lines, they seem to be unnecessary

gepardo marked an inline comment as done.Feb 10 2019, 6:39 PM
elvisangelaccio requested changes to this revision.Feb 10 2019, 7:28 PM

Thanks, that fixed the tests.

However, I'm hitting the Q_ASSERT in Archive::Entry::appendEntry when opening any 7z archive:

#3  0x00007f30665c8c28 in qt_assert(char const*, char const*, int) () from /usr/lib/libQt5Core.so.5
#4  0x00007f306893cb07 in Kerfuffle::Archive::Entry::appendEntry (this=0x55d5c11f01d0, entry=0x55d5c139b3f0) at ../kerfuffle/archiveentry.cpp:85
#5  0x00007f30524f97ea in ArchiveModel::insertEntry (this=0x55d5c11654f0, entry=0x55d5c139b3f0, behaviour=ArchiveModel::DoNotNotifyViews) at ../part/archivemodel.cpp:559
#6  0x00007f30524fac2d in ArchiveModel::newEntry (this=0x55d5c11654f0, receivedEntry=0x55d5c139b3f0, behaviour=ArchiveModel::DoNotNotifyViews) at ../part/archivemodel.cpp:534
#7  0x00007f30524facb2 in ArchiveModel::slotListEntry (this=0x55d5c11654f0, entry=0x55d5c139b3f0) at ../part/archivemodel.cpp:456
#8  0x00007f3052502536 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<Kerfuffle::Archive::Entry*>, void, void (ArchiveModel::*)(Kerfuffle::Archive::Entry*)>::call (f=
    (void (ArchiveModel::*)(ArchiveModel * const, Kerfuffle::Archive::Entry *)) 0x7f30524fac90 <ArchiveModel::slotListEntry(Kerfuffle::Archive::Entry*)>, o=0x55d5c11654f0, arg=0x7ffc799e5090) at /usr/include/qt/QtCore/qobjectdefs_impl.h:152
#9  0x00007f3052502493 in QtPrivate::FunctionPointer<void (ArchiveModel::*)(Kerfuffle::Archive::Entry*)>::call<QtPrivate::List<Kerfuffle::Archive::Entry*>, void> (f=
    (void (ArchiveModel::*)(ArchiveModel * const, Kerfuffle::Archive::Entry *)) 0x7f30524fac90 <ArchiveModel::slotListEntry(Kerfuffle::Archive::Entry*)>, o=0x55d5c11654f0, arg=0x7ffc799e5090) at /usr/include/qt/QtCore/qobjectdefs_impl.h:185
#10 0x00007f30525023b6 in QtPrivate::QSlotObject<void (ArchiveModel::*)(Kerfuffle::Archive::Entry*), QtPrivate::List<Kerfuffle::Archive::Entry*>, void>::impl (which=1, this_=0x55d5c17f2e10, r=0x55d5c11654f0, a=0x7ffc799e5090, ret=0x0)
    at /usr/include/qt/QtCore/qobjectdefs_impl.h:414
#11 0x00007f30667ea9d0 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#12 0x00007f3068949736 in Kerfuffle::Job::newEntry (this=0x55d5c12de240, _t1=0x55d5c139b3f0) at kerfuffle/kerfuffle_autogen/EWIEGA46WW/moc_jobs.cpp:216
#13 0x00007f30688f2bcd in Kerfuffle::Job::onEntry (this=0x55d5c12de240, entry=0x55d5c139b3f0) at ../kerfuffle/jobs.cpp:184
#14 0x00007f30688fad86 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<Kerfuffle::Archive::Entry*>, void, void (Kerfuffle::Job::*)(Kerfuffle::Archive::Entry*)>::call (f=&virtual table offset 168, o=0x55d5c12de240, arg=0x7ffc799e5320)
    at /usr/include/qt/QtCore/qobjectdefs_impl.h:152
#15 0x00007f30688face3 in QtPrivate::FunctionPointer<void (Kerfuffle::Job::*)(Kerfuffle::Archive::Entry*)>::call<QtPrivate::List<Kerfuffle::Archive::Entry*>, void> (f=&virtual table offset 168, o=0x55d5c12de240, arg=0x7ffc799e5320)
    at /usr/include/qt/QtCore/qobjectdefs_impl.h:185
#16 0x00007f30688fac06 in QtPrivate::QSlotObject<void (Kerfuffle::Job::*)(Kerfuffle::Archive::Entry*), QtPrivate::List<Kerfuffle::Archive::Entry*>, void>::impl (which=1, this_=0x55d5c16d3250, r=0x55d5c12de240, a=0x7ffc799e5320, ret=0x0)
    at /usr/include/qt/QtCore/qobjectdefs_impl.h:414
#17 0x00007f30667ea9d0 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#18 0x00007f3068946cc6 in Kerfuffle::ReadOnlyArchiveInterface::entry (this=0x55d5c1bfe1f0, _t1=0x55d5c139b3f0) at kerfuffle/kerfuffle_autogen/EWIEGA46WW/moc_archiveinterface.cpp:267
#19 0x00007f3050e71e97 in CliPlugin::readListLine (this=0x55d5c1bfe1f0, line=...) at ../plugins/cli7zplugin/cliplugin.cpp:271
#20 0x00007f306892470b in Kerfuffle::CliInterface::handleLine (this=0x55d5c1bfe1f0, line=...) at ../kerfuffle/cliinterface.cpp:931
#21 0x00007f3068923a40 in Kerfuffle::CliInterface::readStdout (this=0x55d5c1bfe1f0, handleAll=false) at ../kerfuffle/cliinterface.cpp:816
#22 0x00007f3068925dae in Kerfuffle::CliInterface::runProcess(QString const&, QStringList const&)::$_1::operator()() const (this=0x55d5c20ef790) at ../kerfuffle/cliinterface.cpp:291

I think I was getting the same crash with the first version of the patch, so there is probably something else still going on.

This revision now requires changes to proceed.Feb 10 2019, 7:28 PM
gepardo added a comment.Feb 10 2019, 7:53 PM

However, I'm hitting the Q_ASSERT in Archive::Entry::appendEntry when opening any 7z archive:

Got it. It seems that for 7zip archives Folder field is not printed. So, I still need to use the attributes also.

gepardo updated this revision to Diff 51349.Feb 10 2019, 8:27 PM

Add checking for directory in Attributes

elvisangelaccio requested changes to this revision.Feb 10 2019, 8:35 PM

Thanks, looks good to me now.

Please remove the QEXPECT_FAIL at line 498 of cli7ztest.cpp, then this is good to go.

This revision now requires changes to proceed.Feb 10 2019, 8:35 PM
gepardo added a comment.Feb 10 2019, 8:37 PM

Please remove the QEXPECT_FAIL at line 498 of cli7ztest.cpp, then this is good to go.

I don't have such line. Should I merge my branch with master?

elvisangelaccio added a comment.Feb 10 2019, 8:43 PM
In D18562#409491, @gepardo wrote:

Please remove the QEXPECT_FAIL at line 498 of cli7ztest.cpp, then this is good to go.

I don't have such line. Should I merge my branch with master?

git rebase master from your branch should be enough.

gepardo updated this revision to Diff 51350.Feb 10 2019, 8:45 PM

Remove Q_EXPECT_FAIL

elvisangelaccio accepted this revision.Feb 10 2019, 8:46 PM

Thanks. Do you have commit access?

This revision is now accepted and ready to land.Feb 10 2019, 8:46 PM
gepardo added a comment.Feb 10 2019, 8:47 PM

Thanks. Do you have commit access?

No, please merge it into master.

Closed by commit R36:ca4667adf1cd: Determine isDirectory by "Folder = " field in 7z plugin (authored by gepardo, committed by elvisangelaccio). · Explain WhyFeb 10 2019, 8:53 PM
This revision was automatically updated to reflect the committed changes.
elvisangelaccio added a comment.Feb 10 2019, 8:54 PM

You're welcome :)

Revision Contents
Changeset List

Diff 51354

View Options

autotests/plugins/cli7zplugin/cli7ztest.cpp

Loading...
View Options

plugins/cli7zplugin/cliplugin.h

Loading...
View Options

plugins/cli7zplugin/cliplugin.cpp

Loading...