Delete IdleSlave having temporary authorization
ClosedPublic

Authored by chinmoyr on Feb 25 2018, 10:55 AM.

Details

Summary

IdleSlave with temporary authorization can be easily misused. So delete any such IdleSlave.

Depends on D10822

Diff Detail

Repository
R303 KInit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Feb 25 2018, 10:55 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 25 2018, 10:55 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr requested review of this revision.Feb 25 2018, 10:55 AM
dfaure accepted this revision.Mar 1 2018, 6:52 AM
This revision is now accepted and ready to land.Mar 1 2018, 6:52 AM
dfaure requested changes to this revision.Mar 1 2018, 6:52 AM

Wait, no, this is in a slot connected to the slave that we're deleting ==> you have to use deleteLater.

This revision now requires changes to proceed.Mar 1 2018, 6:52 AM
chinmoyr updated this revision to Diff 28536.Mar 4 2018, 3:40 AM

Used deleteLater()

dfaure requested changes to this revision.Mar 4 2018, 10:45 AM
dfaure added inline comments.
src/klauncher/klauncher.cpp
1114

This deletes the Slave C++ class, but it doesn't actually kill the ioslave.
So why do it?

I'm confused now. Do you want to kill the ioslave (then call slave->kill())
or do you want to reuse that ioslave, just without any of the previous authorizations (which is what I thought you were doing in slaveStatus()) ?
If the latter, then why the delete here?

This revision now requires changes to proceed.Mar 4 2018, 10:45 AM
chinmoyr added inline comments.Mar 4 2018, 11:09 AM
src/klauncher/klauncher.cpp
1114

I meant to kill the slave and not reuse it.
And IdleSlave subclasses QObject and doesn't have kill() which is why I used deleteLater which actually kills the IdleSlave.

No, that deletes the IdleSlave QObject, but the slave process is still running then.

IdleSlave::~IdleSlave()
{
}

Not much happening there ;)

Yes, that makes me wondering if the SLAVE_MAX_IDLE thing actually works.

Hmm, and KLauncher::requestHoldSlave looks like it's leaking the IdleSlave (well, child of "this", but the object won't be deleted until logging out).
Unless I'm missing something, that's a bug from 2001 :-)

Anyhow, one thing at a time, can you test if the kioslave process really goes away?
My bet is that it doesn't, in which case you need a call to KIOPrivate::sendTerminateSignal(slave->pid());

(if that's not available from this repository, add a method IdleSlave::kill())

[The alternative solution is to call SlaveBase::exit() from the slave process itself, but there's no good place for doing that I think, so better kill it when klauncher decides it's idle, it's simpler than sending it a command to ask it to exit.]

Actually deleteLater() does kill the slave. I placed a qDebug statement in there and immediately after the message I am getting

kdeinit5: PID $slave_pid terminated.

Interesting. But are you sure that neither Slave::kill() nor SlaveBase::exit() is called? It can't just be the deleteLater that does this, unless I'm missing something (e.g. closing the pipe makes the slave die with SIGPIPE maybe)

Neither SlaveBase::exit nor Slave::kill is called. I also commented the line in ConnectionBackend which removes the socket file but even then the slave process terminated.
At this point I have no idea why is this working.

dfaure added a comment.Mar 4 2018, 4:45 PM

I tested what happens in the current code when an idle job is killed. My testcase was configuring to use KIO in componentchooser ("open http urls in an application based on the contents on the URL") and then kioclient5 exec www.kde.org. This puts the slave on hold while it emits the mimetype, while KIO runs the associated application for that mimetype.
The idea was that the app can then use that job to resume the same transfer, if the app uses KIO. In my testcase it's launching konqueror+webenginepart, which doesn't use KIO, so the slave remains idle and gets killed later by klauncher.

What happens just after KLauncher::idleTimeout deletes the slave is that the slave goes to this code path inside mimeType() :

if (ret == -1) {
    qDebug() << "read error";
    exit();
}

So in my testcase at least, it goes to SlaveBase::exit(), which is how the slave exits :)
You could try the same set of debugging statements in your testcase... http://www.davidfaure.fr/2018/exit_debug.diff for kio and http://www.davidfaure.fr/2018/kinit.diff for kinit.
Remember to restart kdeinit5 afterwards by typing kdeinit5 in a terminal - but I guess you know this already ;-)

I haven't yet tried your patch (which btw involves the http ioslave right?) but I am quite sure that exit() is not called for file ioslave after interrupting the application. I have placed the debug statements inside exit() as well as before every exit() call and I am not seeing any output.

That aside here's what I have observed till now,
After IdleSlave is created, inside dispatchLoop() SlaveBase waits in this line

if (d->appConnection.hasTaskAvailable() || d->appConnection.waitForIncomingTask(ms)) {

with appConnection having no tasks available.
After deleting the IdleSlave waitForIncomingTask returns false because the Connection object in IdleSlave was destroyed which interrupted the connection and changed the socket state from QAbstractSocket::ConnectedState (It's merely a speculation. I haven't verified it yet). Then the variable ret is assigned -1. Since the connection was interrupted, SlaveBase returns from dispatchLoop and in FileProtocol kdemain returns thus terminating the process.

Please fill me in if you think I have missed something.

David, what about this patch? It seems to me deleting the IdleSlave object is the only way to kill the ioslave from here.

dfaure accepted this revision.Mar 23 2018, 5:12 PM

OK. If the slave indeed gets killed then this is what we want, indeed.

This revision is now accepted and ready to land.Mar 23 2018, 5:12 PM
This revision was automatically updated to reflect the committed changes.