DevHeads.net

Review Request 125613: Race condition and error notification loss in ListJob

Review request for kdelibs.

Repository: kdelibs

Description
Race condition and error notification loss in ListJob

Diffs
kio/kio/job.cpp 91712e3
kio/kio/jobclasses.h d771cfe

Diff: <a href="https://git.reviewboard.kde.org/r/125613/diff/" title="https://git.reviewboard.kde.org/r/125613/diff/">https://git.reviewboard.kde.org/r/125613/diff/</a>

Testing
Tests done on Kubuntu 15.10:

With this folder structure in ~:
src
######c (Folder)
##############b (Folder chmod'ed to 700 and owned by root)
#######################d (10MB file made of /dev/urandom data)
##############a (10MB file made of /dev/urandom data)

Then, (as a normal user execute: "kioclient ~/src ~/dst")

Expected result: Notification that some files were not able to be copied (Cannot access "b" folder)
Result with latest kdelibs: Silent copying where b folder is owned by user but nothing inside.
---When all kdebugdialog flags are set, a backtrace is also seen.

subError notifications for listjob subjobs are lost, Copyjob uses this signal to skip data.

Also, this fix prevents a race condition.

ListJob has another ListJob as a subjob.

Chaild fails for whatever reason and calls error, which calls slotError, which calls slotsFinished and emitResult.
The result of the child gets collected by parent. slotResult of parent gets called, removes subjob and emitsresult because there are no subjobs left.
---That's fine as long as the slave associated with the parent emits finished before the child fails. If it doesn't, parent calls emitsResult twice.

Result after patch: kioclient shows a MessageBox telling the copy operation could not be completed.

Thanks,

Alberto Jiménez Ruiz

Comments

Re: Review Request 125613: Race condition and error notification

By =?utf-8?q?Alber... at 10/12/2015 - 15:11

(Updated Oct. 12, 2015, 7:11 p.m.)

Review request for kdelibs.

Bugs: 333436
<a href="http://bugs.kde.org/show_bug.cgi?id=333436" title="http://bugs.kde.org/show_bug.cgi?id=333436">http://bugs.kde.org/show_bug.cgi?id=333436</a>

Repository: kdelibs

Description
Race condition and error notification loss in ListJob

Diffs
kio/kio/job.cpp 91712e3
kio/kio/jobclasses.h d771cfe

Diff: <a href="https://git.reviewboard.kde.org/r/125613/diff/" title="https://git.reviewboard.kde.org/r/125613/diff/">https://git.reviewboard.kde.org/r/125613/diff/</a>

Testing
Tests done on Kubuntu 15.10:

With this folder structure in ~:
src
######c (Folder)
##############b (Folder chmod'ed to 700 and owned by root)
#######################d (10MB file made of /dev/urandom data)
##############a (10MB file made of /dev/urandom data)

Then, (as a normal user execute: "kioclient ~/src ~/dst")

Expected result: Notification that some files were not able to be copied (Cannot access "b" folder)
Result with latest kdelibs: Silent copying where b folder is owned by user but nothing inside.
---When all kdebugdialog flags are set, a backtrace is also seen.

subError notifications for listjob subjobs are lost, Copyjob uses this signal to skip data.

Also, this fix prevents a race condition.

ListJob has another ListJob as a subjob.

Chaild fails for whatever reason and calls error, which calls slotError, which calls slotsFinished and emitResult.
The result of the child gets collected by parent. slotResult of parent gets called, removes subjob and emitsresult because there are no subjobs left.
---That's fine as long as the slave associated with the parent emits finished before the child fails. If it doesn't, parent calls emitsResult twice.

Result after patch: kioclient shows a MessageBox telling the copy operation could not be completed.

Thanks,

Alberto Jiménez Ruiz

Re: Review Request 125613: Race condition and error notification

By =?utf-8?q?Alber... at 10/13/2015 - 06:48

(Updated Oct. 13, 2015, 10:48 a.m.)

Review request for kdelibs.

Bugs: 333436
<a href="http://bugs.kde.org/show_bug.cgi?id=333436" title="http://bugs.kde.org/show_bug.cgi?id=333436">http://bugs.kde.org/show_bug.cgi?id=333436</a>

Repository: kdelibs

Description
Race condition and error notification loss in ListJob

Diffs (updated)
kio/kio/copyjob.cpp e6c3817
kio/kio/job.cpp 91712e3
kio/kio/jobclasses.h d771cfe
kio/tests/jobtest.h d3c552e
kio/tests/jobtest.cpp ee2677a

Diff: <a href="https://git.reviewboard.kde.org/r/125613/diff/" title="https://git.reviewboard.kde.org/r/125613/diff/">https://git.reviewboard.kde.org/r/125613/diff/</a>

Testing (updated)
Update 1, added unittest
Changed condition of listjob slotresult finishing (When executed as a synchronous job, gets stuck in a eventloop)
Added setError to copyjob.

Tests done on Kubuntu 15.10:

With this folder structure in ~:
src
######c (Folder)
##############b (Folder chmod'ed to 700 and owned by root)
#######################d (10MB file made of /dev/urandom data)
##############a (10MB file made of /dev/urandom data)

Then, (as a normal user execute: "kioclient ~/src ~/dst")

Expected result: Notification that some files were not able to be copied (Cannot access "b" folder)
Result with latest kdelibs: Silent copying where b folder is owned by user but nothing inside.
---When all kdebugdialog flags are set, a backtrace is also seen.

subError notifications for listjob subjobs are lost, Copyjob uses this signal to skip data.

Also, this fix prevents a race condition.

ListJob has another ListJob as a subjob.

Chaild fails for whatever reason and calls error, which calls slotError, which calls slotsFinished and emitResult.
The result of the child gets collected by parent. slotResult of parent gets called, removes subjob and emitsresult because there are no subjobs left.
---That's fine as long as the slave associated with the parent emits finished before the child fails. If it doesn't, parent calls emitsResult twice.

Result after patch: kioclient shows a MessageBox telling the copy operation could not be completed.

Thanks,

Alberto Jiménez Ruiz

Re: Review Request 125613: Race condition and error notification

By Albert Astals Cid at 10/12/2015 - 15:29

Do you think you can add an unittest for this or is it hard to recreate the conditionts?

- Albert Astals Cid

On oct. 12, 2015, 7:11 p.m., Alberto Jiménez Ruiz wrote:

Re: Review Request 125613: Race condition and error notification

By =?utf-8?q?Alber... at 10/12/2015 - 16:09

chmodding to 000 works. I'm writing a unittest.

For completion, here's how I emulated it in KDE Plasma 5 (Kubuntu 15.10 beta) and KDE 4.X (It also happens)

#Perform on empty folder
#Activate in kdebugdialog kio (7007)

mkdir src
mkdir src/c
mkdir src/c/b
dd if=/dev/urandom of=src/c/a bs=1M count=10
dd if=/dev/urandom of=src/c/b/d bs=1M count=10
chmod 000 src/c/b

kioclient copy src dst

You should see:
klauncher(1553)/kio (KLauncher) KLauncher::requestSlave: KLauncher: launching new slave "kio_file" with protocol= "file" args= ("file", "local:/run/user/1000/ksocket-alberto/klauncherhX1553.slave-socket", "local:/run/user/1000/ksocket-alberto/kioclientZT1614.slave-socket")
klauncher(1553)/kio (KLauncher) KLauncher::processRequestReturn: "kio_file" (pid 1615) up and running.
kioclient(1614)/kio (KIOJob) KIO::SlaveInterface::dispatch: error 111 "/home/alberto/prueba2/dst"
klauncher(1553)/kio (KLauncher) KLauncher::requestSlave: KLauncher: launching new slave "kio_file" with protocol= "file" args= ("file", "local:/run/user/1000/ksocket-alberto/klauncherhX1553.slave-socket", "local:/run/user/1000/ksocket-alberto/kioclientCj1614.slave-socket")
klauncher(1553)/kio (KLauncher) KLauncher::processRequestReturn: "kio_file" (pid 1616) up and running.
kioclient(1614)/kio (KIOJob) KIO::SlaveInterface::dispatch: error 117 "/home/alberto/prueba2/src/c/b"
kioclient(1614)/kio (KIOJob) KIO::CopyJobPrivate::statCurrentSrc: Stating finished. To copy: 10506240 , available: 21012074496
kioclient(1614)/kio (KIOJob) KIO::SimpleJob::~SimpleJob: Killing job KIO::SimpleJob(0xf98e00) in destructor! "[
0: /usr/lib/libkdecore.so.5(kRealBacktrace(int)+0x50) [0x7f6948647c20]
1: /usr/lib/libkio.so.5(KIO::SimpleJob::~SimpleJob()+0xb7) [0x7f69491984f7]
2: /usr/lib/libkio.so.5(KIO::ListJob::~ListJob()+0x9) [0x7f6949198809]
3: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QObject::event(QEvent*)+0x1f8) [0x7f6947f76d28]
4: /usr/lib/x86_64-linux-gnu/libQtGui.so.4(QApplicationPrivate::notify_helper(QObject*, QEvent*)+0x8c) [0x7f69472a5cdc]
5: /usr/lib/x86_64-linux-gnu/libQtGui.so.4(QApplication::notify(QObject*, QEvent*)+0x2b6) [0x7f69472acc16]
6: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QCoreApplication::notifyInternal(QObject*, QEvent*)+0x8d) [0x7f6947f5c85d]
7: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*)+0x376) [0x7f6947f60316]
8: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(+0x1bb07e) [0x7f6947f8d07e]
9: /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_dispatch+0x2a7) [0x7f6943111ff7]
10: /lib/x86_64-linux-gnu/libglib-2.0.so.0(+0x4a250) [0x7f6943112250]
11: /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_iteration+0x2c) [0x7f69431122fc]
12: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>)+0x7e) [0x7f6947f8d1ee]
13: /usr/lib/x86_64-linux-gnu/libQtGui.so.4(+0x26fc26) [0x7f6947350c26]
14: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>)+0x41) [0x7f6947f5b0d1]
15: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>)+0x1b5) [0x7f6947f5b445]
16: /usr/lib/x86_64-linux-gnu/libQtCore.so.4(QCoreApplication::exec()+0x99) [0x7f6947f61429]
17: kioclient() [0x404d64]
18: kioclient() [0x405faf]
19: kioclient() [0x403b5e]
20: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7f69469b5a40]
21: kioclient(_start+0x29) [0x404599]
]
"

- Alberto

On Oct. 12, 2015, 7:11 p.m., Alberto Jiménez Ruiz wrote:

Re: Review Request 125613: Race condition and error notification

By =?utf-8?Q?Thoma... at 10/12/2015 - 15:53

"Depends" - your file, your control.
You can remove and re-add permissions at will; kio might be too "smart to" be challenged by permissions, but chattr might come to the rescue (ie. turn the inaccessible file immutable - kio not smart enough for /that/ ;-)

- Thomas

On Okt. 12, 2015, 7:11 nachm., Alberto Jiménez Ruiz wrote:

Re: Review Request 125613: Race condition and error notification

By Albert Astals Cid at 10/12/2015 - 15:48

Why would you need test privileges to create a folder you can't access, that's doable without being root, no?

- Albert

On oct. 12, 2015, 7:11 p.m., Alberto Jiménez Ruiz wrote:

Re: Review Request 125613: Race condition and error notification

By =?utf-8?q?Alber... at 10/12/2015 - 15:43

It happens to very few people. Most developers tried to reproduce bug 333436 and bug 162211(I believe they are closely related) with no luck. The heart of this bug is that sometimes a child ListJob fails silently and I emulated an "error" making an unaccessable folder. A unit test for this would need root privileges.

When I ran across this bug copying stuff with dolphin, I tried to recreate it copying folders thousands of times. I couldn't.

- Alberto

On Oct. 12, 2015, 7:11 p.m., Alberto Jiménez Ruiz wrote: