kioexecd: watch for creations or modifications of the temporary files
ClosedPublic

Authored by jtamate on Aug 31 2018, 8:36 AM.

Details

Summary

When a non KIO friendly program opens a non local file, the file is copied to an user temporary folder by kioexec.
Watch any creation or modification or deletion of that temporary file, because some programs delete the old file and create a new one when saving the file. When the file is recreated or modified, the user is asked if he/she wants to upload the modified file.

If the temporary file keeps deleted more than 30s then forget about that file and try to delete the temporary directory.

BUG: 397742

Test Plan

Tested logging out and in several times and opening two or three remote files with xed and libreoffice.
After removing manually some of the temporary files and wait around ~31s the empty temporary directories are deleted.

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 31 2018, 8:36 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jtamate requested review of this revision.Aug 31 2018, 8:36 AM
broulik added inline comments.Aug 31 2018, 8:39 AM
src/kioexec/kioexecd.cpp
58

I'm not sure that's a good idea

73

You never unwatch the dir

jtamate marked 2 inline comments as done.Aug 31 2018, 8:59 AM
jtamate added inline comments.
src/kioexec/kioexecd.cpp
58

Why not?
It should be just removing the temporary file copy and it's backups.

73

No, only at the destructor, unless there is a way to know when the program that uses the file has been closed, I guess keeping the file during all the session is the same behavior as before.

broulik added inline comments.Aug 31 2018, 9:01 AM
src/kioexec/kioexecd.cpp
58

If you can be sure kioexec creates a folder per temp file (it might does), then this is probably fine.
Also check kioexec what it does when the process closes, if it also recursively removes the temp dir

73

I see. Previously it would just remove the watcher when the temp file is removed. Not sure how to fix it now, maybe we need an unwatch dbus call.
Imho we shouldn't change the behavior here too much, ie. "randomly" watching the parent dir instead of the actual file passed in as argument but instead introduce a watchDirectory dbus call that exhibits the new behavior? Not sure.

jtamate updated this revision to Diff 40779.Aug 31 2018, 3:14 PM
jtamate marked 3 inline comments as done.
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)

I'm not sure if I have to keep compatibility in the dbus calls, therefore the old one is still there.

src/kioexec/kioexecd.cpp
58

kioexec creates a folder per temp file.
kioexec doesn't remove recursively, therefore the temporary directory will not be deleted if the program creates backup files, like a.txt~

73

The watch will be left as it was, introducing the create case and removing the deleted case.

I've implemented an unwatch dbus call.

I don't understand what's the counter used for. Can you explain why we need it and can you update the Test plan accordingly?

jtamate edited the summary of this revision. (Show Details)Sep 2 2018, 11:17 AM
jtamate edited the test plan for this revision. (Show Details)
jtamate updated this revision to Diff 40888.Sep 3 2018, 7:00 AM
jtamate added a reviewer: elvisangelaccio.

Updated with the code that handles m_openedBy right.

I'm unhappy with that stop watching is on exit == 0, so when it's not, somehow, containers will continue to grow, it'll result in higher memory usage and slower performance. So stop watching should not depend on process return code, also same command should not stop container to shrink, that's my opinion, but i can miss something.

src/kioexec/kioexecd.cpp
114

Indentation

src/kioexec/main.cpp
261 ↗(On Diff #40888)

Indentation

263 ↗(On Diff #40888)

Extra space after &&

I'm unhappy with that stop watching is on exit == 0, so when it's not, somehow, containers will continue to grow, it'll result in higher memory usage and slower performance. So stop watching should not depend on process return code, also same command should not stop container to shrink, that's my opinion, but i can miss something.

I desist from this path, it has a big flaw: What if the tabbed application is already running? All the temporary files will be deleted immediately. :-(
I'll try this other path: allowing the application a generous amount of time (30s) to recreate the deleted file, otherwise deleting the temporary directory. Unfortunately, the temporary directories will not shrink unless the app deletes the temporary file.
I guess the only complete solution is to implement use fuse (where available).

jtamate updated this revision to Diff 40908.Sep 3 2018, 12:02 PM
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)

As I'm never sure if a timed execution can happen in the middle of other execution, I've added a mutex for m_deleted.

anthonyfieroni added inline comments.Sep 3 2018, 1:05 PM
src/kioexec/kioexecd.cpp
102

Better to me, make a class variable single shot timer, then when you add in deleted start it if not, when it expires delete what you do, at last check if deleted is not empty just restart it. Make 30s predefined.

anthonyfieroni added inline comments.Sep 3 2018, 1:14 PM
src/kioexec/kioexecd.cpp
114

Also for loop should looks like:

for (it = begin(); it != end();) {
    if () {
        it = erase(it);
    } else {
        ++it;
    }
}
anthonyfieroni added inline comments.Sep 3 2018, 1:19 PM
src/kioexec/kioexecd.cpp
79

Contains should be also in the guard.

src/kioexec/kioexecd.h
39

Unused?

jtamate updated this revision to Diff 40952.Sep 4 2018, 6:32 AM
jtamate marked 4 inline comments as done.

Implemented Anthony comments/suggestions.

anthonyfieroni added inline comments.Sep 4 2018, 7:05 AM
src/kioexec/kioexecd.cpp
50

Also add interval here, setInterval, in other place just start()

80–83

Now it's not needed 'remove' will do the work

118

lvalue = rvalue (i mean space between =)

jtamate updated this revision to Diff 40960.Sep 4 2018, 7:35 AM
jtamate marked 3 inline comments as done.

Done.

m_timer.start(predefinedTimeout) should be m_timer.start() in 2 places, but let's see @elvisangelaccio and @dfaure comments.

jtamate updated this revision to Diff 40961.Sep 4 2018, 8:01 AM

I missed that part, sorry.
It is safer that way, the start(time) method was added in Qt 5.8.

elvisangelaccio requested changes to this revision.Sep 4 2018, 7:58 PM
elvisangelaccio added inline comments.
src/kioexec/kioexecd.cpp
58

The problem with using QDir::removeRecursively() is that the folder we are going to delete recursively is an input from dbus. What happens if some malicious software calls watch("~/dummy.txt") ?

At the very least we need to check whether this folder starts with QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + QStringLiteral("/krun") (the path used by kioexec).

This revision now requires changes to proceed.Sep 4 2018, 7:58 PM
jtamate added inline comments.Sep 5 2018, 7:09 AM
src/kioexec/kioexecd.cpp
58

There is a slightly problem: QStandardPaths::CacheLocation is application dependent, and their values doesn't match here:
kioexec: /home/jtorres/.cache/kioexec/
kioexecd: /home/jtorres/.cache/kiod5/
Can we assume that replacing kiod5 by kioexec will always work?

We could use QStandardPaths::GenericCacheLocation instead, but this is not guaranteed to be non empty.

Or another solution: keep it as it was (delete only the file and the directory if it is possible).

80–83

You're right. In this case it isn't possible to be notified of a file creation unless it has been deleted first.

anthonyfieroni added inline comments.Sep 5 2018, 7:27 AM
src/kioexec/kioexecd.cpp
58

You can remove only file and then if dir is empty to delete it. Same in slotCheckDeletedFiles

jtamate updated this revision to Diff 41026.Sep 5 2018, 7:56 AM
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)

Do not delete recursively.
Do not delete the file after 30s (we know it is already deleted).

dfaure requested changes to this revision.Sep 5 2018, 8:24 AM
dfaure added inline comments.
src/kioexec/kioexecd.cpp
36

static const int...

79

Why is there a mutex at all, in this single-threaded code? This doesn't make sense to me.

110

Move the call to currentDateTime() outside of the loop, so it happens only once.
It's much more costly than one might think (because of timezone conversion, which uses tzset() etc.)

This revision now requires changes to proceed.Sep 5 2018, 8:24 AM
jtamate updated this revision to Diff 41030.Sep 5 2018, 8:41 AM

Why the mutex? I interpreted that QTimer work as an interrumpt (wrong) instead of generating events in the event queue (right).
So I've gone through all the possible mistakes one can do here:

  • Design mistakes
  • Security mistakes
  • Misunderstanding the API.

Definitely, I need more vacation.

dfaure requested changes to this revision.Sep 5 2018, 9:16 AM

No worries, that's what code review is for :-)

Now this is starting to look good ;)

src/kioexec/kioexecd.cpp
81

Doesn't kdirwatch emit dirty when emitting created anyway?
(because historically, there was only "dirty" initially)

But I could be confusing watching files and watching directories, so better test rather than trusting me ;)

src/kioexec/kioexecd.h
28

remove

This revision now requires changes to proceed.Sep 5 2018, 9:16 AM
jtamate updated this revision to Diff 41035.Sep 5 2018, 10:02 AM

Tested, dirty is not signaled when created (at least I didn't saw the dialog for uploading the modified file).
Removed the header.

dfaure accepted this revision.Sep 5 2018, 12:27 PM

Thanks!

bruns added a subscriber: bruns.Sep 5 2018, 2:35 PM
bruns added inline comments.
src/kioexec/kioexecd.cpp
114

Even better use the std::remove_if/std::erase pattern, which avoids O(n^2) complexity during erase.
Or in this specific case, std::partition/{iterate over removed}/std::erase

May I commit or should I wait for @elvisangelaccio to accept the changes? This time I have read the arc message, that is usually something about non tracked files:
Revision 'D15180: kioexecd: watch for creations or modifications of the temporary files' has not been accepted. Continue anyway? [y/N]

Even better use the std::remove_if/std::erase pattern, which avoids O(n^2) complexity during erase.
Or in this specific case, std::partition/{iterate over removed}/std::erase

I assume QMap is like a std::map. I've seen in this cppreference page that these algorithms cannot be used with associative containers such as std::set and std::map.

bruns added inline comments.Sep 5 2018, 4:29 PM
src/kioexec/kioexecd.cpp
110

makes me wonder, wouldn't it be better to use currentDateTimeUtc() throughout? We are not interested in absolute time here anyway.

jtamate updated this revision to Diff 41066.Sep 5 2018, 5:02 PM

Use QDateTime::currentDateTimeUtc() instead of QDateTime::currentDateTime().

elvisangelaccio accepted this revision.Sep 5 2018, 7:23 PM
elvisangelaccio added inline comments.
src/kioexec/kioexecd.cpp
107

Maybe constBegin()/constEnd() here?

This revision is now accepted and ready to land.Sep 5 2018, 7:23 PM
jtamate marked an inline comment as done.Sep 6 2018, 6:15 AM
jtamate added inline comments.
src/kioexec/kioexecd.cpp
107

I always thought const iterators were meant to be used when the whole container is considered ReadOnly, not only its elements, looks like the compiler agrees:
kioexecd.cpp:138:36: error: no matching function for call to ‘QMap<QString, QDateTime>::erase(QMap<QString, QDateTime>::const_iterator&)’

This revision was automatically updated to reflect the committed changes.
jtamate marked an inline comment as done.