Differential D10045

remote: don't create entries with empty names
ClosedPublic
Actions

Authored by mwolff on Jan 23 2018, 1:16 PM.

Details

Reviewers
dfaure
Commits
R241:4d153df7359c: remote: don't create entries with empty names
Summary

Directly after adding a new remote:/ entry via knetattach, we
may read the .desktop file in a partial state without a valid
name. Or maybe the user even hand-edits the file and removes the
name. Filter these entries out. Additionally, don't ever try to
read anything but *.desktop files, most notably we do not want
to parse *.desktop.lock files produced by knetattach.

This removes an assertion that I hit reproducibly when adding
a new remote:/ link via knetattach:

Thread 1 (Thread 0x7f194a2fb4c0 (LWP 10953)):
[KCrash Handler]
#6 0x00007f195a064860 in raise () from /usr/lib/libc.so.6
#7 0x00007f195a065ec9 in abort () from /usr/lib/libc.so.6
#8 0x00007f19570678c8 in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/libQt5Core.so.5
#9 0x00007f19570624a7 in qt_assert(char const*, char const*, int) () from /usr/lib/libQt5Core.so.5
#10 0x00007f19598dc55f in KCoreDirListerCache::slotUpdateResult (this=0x7f1959998500 <(anonymous namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, j=0x56549d5cfc60) at /home/milian/projects/kf5/src/frameworks/kio/src/core/kcoredirlister.cpp:1799
#11 0x00007f19598e49b5 in KCoreDirListerCache::qt_static_metacall (_o=0x7f1959998500 <(anonymous namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, _c=QMetaObject::InvokeMetaMethod, _id=11, _a=0x7fffa67266f0) at /home/milian/projects/kf5/build-dbg/frameworks/kio/src/core/KF5KIOCore_autogen/include/moc_kcoredirlister_p.cpp:139
#12 0x00007f195729aee6 in QMetaObject::activate(QObject*, int, int, void) () from /usr/lib/libQt5Core.so.5
#13 0x00007f1958c56ada in KJob::result (this=0x56549d5cfc60, _t1=0x56549d5cfc60, _t2=...) at /ssd/milian/projects/kf5/build-dbg/frameworks/kcoreaddons/src/lib/KF5CoreAddons_autogen/include/moc_kjob.cpp:569
#14 0x00007f1958c549df in KJob::finishJob (this=0x56549d5cfc60, emitResult=true) at /home/milian/projects/kf5/src/frameworks/kcoreaddons/src/lib/jobs/kjob.cpp:109
#15 0x00007f1958c55141 in KJob::emitResult (this=0x56549d5cfc60) at /home/milian/projects/kf5/src/frameworks/kcoreaddons/src/lib/jobs/kjob.cpp:293
#16 0x00007f195988c826 in KIO::SimpleJob::slotFinished (this=0x56549d5cfc60) at /home/milian/projects/kf5/src/frameworks/kio/src/core/simplejob.cpp:236
#17 0x00007f19598869b4 in KIO::ListJob::slotFinished (this=0x56549d5cfc60) at /home/milian/projects/kf5/src/frameworks/kio/src/core/listjob.cpp:246
#18 0x00007f1959886f97 in KIO::ListJob::qt_static_metacall (_o=0x56549d5cfc60, _c=QMetaObject::InvokeMetaMethod, _id=4, _a=0x7fffa6726a10) at /home/milian/projects/kf5/build-dbg/frameworks/kio/src/core/KF5KIOCore_autogen/include/moc_listjob.cpp:129
#19 0x00007f195729aee6 in QMetaObject::activate(QObject*, int, int, void) () from /usr/lib/libQt5Core.so.5
#20 0x00007f195987231f in KIO::SlaveInterface::finished (this=0x56549d3c24b0) at /home/milian/projects/kf5/build-dbg/frameworks/kio/src/core/KF5KIOCore_autogen/include/moc_slaveinterface.cpp:437
#21 0x00007f195986fe3d in KIO::SlaveInterface::dispatch (this=0x56549d3c24b0, _cmd=104, rawdata=...) at /home/milian/projects/kf5/src/frameworks/kio/src/core/slaveinterface.cpp:160
#22 0x00007f195986fa6a in KIO::SlaveInterface::dispatch (this=0x56549d3c24b0) at /home/milian/projects/kf5/src/frameworks/kio/src/core/slaveinterface.cpp:89
#23 0x00007f19598746eb in KIO::Slave::gotInput (this=0x56549d3c24b0) at /home/milian/projects/kf5/src/frameworks/kio/src/core/slave.cpp:406
#24 0x00007f195991088d in KIO::Slave::qt_static_metacall (_o=0x56549d3c24b0, _c=QMetaObject::InvokeMetaMethod, _id=2, _a=0x7fffa6726d10) at /home/milian/projects/kf5/build-dbg/frameworks/kio/src/core/KF5KIOCore_autogen/EWIEGA46WW/moc_slave.cpp:89
#25 0x00007f195729aee6 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#26 0x00007f19598135e3 in KIO::Connection::readyRead (this=0x56549d4620b0) at /home/milian/projects/kf5/build-dbg/frameworks/kio/src/core/KF5KIOCore_autogen/include/moc_connection_p.cpp:143
#27 0x00007f19598122e5 in KIO::ConnectionPrivate::dequeue (this=0x56549d63f1d0) at /home/milian/projects/kf5/src/frameworks/kio/src/core/connection.cpp:46
#28 0x00007f19598133f4 in KIO::Connection::qt_static_metacall (_o=0x56549d4620b0, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x56549d4e6fa0) at /home/milian/projects/kf5/build-dbg/frameworks/kio/src/core/KF5KIOCore_autogen/include/moc_connection_p.cpp:87
#29 0x00007f195729b932 in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
#30 0x00007f195826ee3c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#31 0x00007f1958276816 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#32 0x00007f195726a6c0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/libQt5Core.so.5
#33 0x00007f195726d326 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib/libQt5Core.so.5
#34 0x00007f19572c7584 in ?? () from /usr/lib/libQt5Core.so.5
#35 0x00007f19502eee38 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#36 0x00007f19502ef081 in ?? () from /usr/lib/libglib-2.0.so.0
#37 0x00007f19502ef10e in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#38 0x00007f19572c6b71 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#39 0x00007f1949cb72f2 in ?? () from /usr/lib/libQt5XcbQpa.so.5
#40 0x00007f1957268d0b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#41 0x00007f1957271ff8 in QCoreApplication::exec() () from /usr/lib/libQt5Core.so.5
#42 0x00007f195a42609e in kdemain () from /usr/lib/libkdeinit5_dolphin.so
#43 0x00007f195a050f4a in __libc_start_main () from /usr/lib/libc.so.6
#44 0x000056549c3e677a in _start ()

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Jan 23 2018, 1:16 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 23 2018, 1:16 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
mwolff requested review of this revision.Jan 23 2018, 1:16 PM
dfaure added a comment.Jan 23 2018, 5:33 PM

That assert is there to catch a horrible bug in the kioslave. UDS_NAME is NOT supposed to be empty, ever. Which kioslave was this about?

dhaumann added a subscriber: dhaumann.Jan 23 2018, 5:42 PM

@dfaure It seems to me there is some know-how in your head that you should add as comment to the ASSERT then ;)

dfaure added a comment.Jan 23 2018, 6:02 PM

Good point, done.

mwolff added a comment.Jan 24 2018, 10:58 AM
In D10045#195097, @dfaure wrote:

Good point, done.

I did the following:

  • in Dolphin, go to remote:/
  • add network folder
  • allow execution of this script (separate usability issue)
  • webdav, then fill out the form for our KDAB owncloud instance, i.e. with host = swanson and folder == /owncloud/remote.php/webdav
  • then save and connect, you'll see a strange warning dialog with this info:

    "You are about to log in to the site "swanson.kdab.com" with the username "milian", but the website does not require authentication. This may be an attempt to trick you. Is "swanson.kdab.com" the site you want to visit?"

    imo this is again a usability issue, but unrelated I guess? Anyhow, accept it and dolphin crashes with the assertion
dfaure added a comment.Jan 29 2018, 8:46 AM

See https://phabricator.kde.org/D10172 for the fix for the first usability issue.

However I can't reproduce the assert...
Can you add this to your kio_remote to find out more?
There must be one strange .desktop file around somewhere.

diff --git a/src/ioslaves/remote/remoteimpl.cpp b/src/ioslaves/remote/remoteimpl.cpp
index 6d01cc8cf7..3376d619b9 100644
--- a/src/ioslaves/remote/remoteimpl.cpp
+++ b/src/ioslaves/remote/remoteimpl.cpp
@@ -184,6 +184,8 @@ void RemoteImpl::createEntry(KIO::UDSEntry &entry, const QString &directory,
     QString new_filename = file;
     new_filename.truncate(file.length()-8);
 
+    qDebug() << "UDS_NAME" << desktop.readName() << "from" << (dir+file);
+    Q_ASSERT(!desktop.readName().isEmpty());
     entry.insert(KIO::UDSEntry::UDS_NAME, desktop.readName());
     entry.insert(KIO::UDSEntry::UDS_URL, "remote:/"+new_filename);
mwolff added a comment.Jan 30 2018, 9:12 AM
41.919 warning: KConfigIniBackend::parseConfig[/home/milian/projects/kf5/src/frameworks/kconfig/src/core/kconfigini.cpp:253]: "KConfigIni: In file /home/milian/.local/share/remoteview/yxcvyxcv.desktop.lock, line 1: " Invalid entry (missing '=')
41.919 warning: KConfigIniBackend::parseConfig[/home/milian/projects/kf5/src/frameworks/kconfig/src/core/kconfigini.cpp:253]: "KConfigIni: In file /home/milian/.local/share/remoteview/yxcvyxcv.desktop.lock, line 2: " Invalid entry (missing '=')
41.919 warning: KConfigIniBackend::parseConfig[/home/milian/projects/kf5/src/frameworks/kconfig/src/core/kconfigini.cpp:253]: "KConfigIni: In file /home/milian/.local/share/remoteview/yxcvyxcv.desktop.lock, line 3: " Invalid entry (missing '=')
41.919 debug: RemoteImpl::createEntry[/home/milian/projects/kf5/src/frameworks/kio/src/ioslaves/remote/remoteimpl.cpp:187]: UDS_NAME "" from "/home/milian/.local/share/remoteview/yxcvyxcv.desktop.lock"
41.919 fatal: unknown[/home/milian/projects/kf5/src/frameworks/kio/src/ioslaves/remote/remoteimpl.cpp:188]: ASSERT: "!desktop.readName().isEmpty()" in file /home/milian/projects/kf5/src/frameworks/kio/src/ioslaves/remote/remoteimpl.cpp, line 188

ahum... ;-) I'll see to find a way to skip these files

mwolff added a comment.Jan 30 2018, 9:18 AM

OK, that's not enough. It seems like the file can sometimes be read while it's still empty:

7.098 debug: RemoteImpl::createEntry[/home/milian/projects/kf5/src/frameworks/kio/src/ioslaves/remote/remoteimpl.cpp:188]: UDS_NAME "" from "/home/milian/.local/share/remoteview/asdfasdf.desktop"
7.098 fatal: unknown[/home/milian/projects/kf5/src/frameworks/kio/src/ioslaves/remote/remoteimpl.cpp:189]: ASSERT: "!desktop.readName().isEmpty()" in file /home/milian/projects/kf5/src/frameworks/kio/src/ioslaves/remote/remoteimpl.cpp, line 189
mwolff updated this revision to Diff 26203.Jan 30 2018, 9:28 AM

filter out empty entries from remote:/ KIO slave

src/ioslaves/remote/remoteimpl.cpp
56–57

is this OK? or are there other entries that should be shown under remote:/ ?

mwolff retitled this revision from Don't assert on empty names to remote: don't create entries with empty names.Jan 30 2018, 1:17 PM
mwolff edited the summary of this revision. (Show Details)
dfaure accepted this revision.Feb 3 2018, 9:07 AM

Minor: now readName() is called 3 times, I think this should go into a local variable.

I don't understand why the name can be empty in one of the desktop files, that seems quite strange. But oh well.

src/ioslaves/remote/remoteimpl.cpp
56–57

I'm not 100% sure, but given the code in statNetworkFolder and below, I think every interesting file always ends with .desktop.

This revision is now accepted and ready to land.Feb 3 2018, 9:07 AM
Closed by commit R241:4d153df7359c: remote: don't create entries with empty names (authored by mwolff). · Explain WhyFeb 6 2018, 9:50 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 26633

View Options

src/ioslaves/remote/remoteimpl.h

Loading...
View Options

src/ioslaves/remote/remoteimpl.cpp

Loading...