Review Request 119110: Release Blocker - KProtocolManager: Fix double mutex locking on a non recursive mutex

[David Faure]

> On July 4, 2014, 7:22 p.m., David Faure wrote:
> > recursive mutexes are more expensive. Can you try this patch instead?
> > http://www.davidfaure.fr/2014/kprotocolmanager.cpp.diff
> 
> David Faure wrote:
>     Actually, nothing beats a unittest :) Added, fix pushed.
>     
>     http://commits.kde.org/kio/842bc8008f5eed03698e8ec67351e791b65ad2b1
>     
>     I'll repack kio after dinner.

Done (v4.100.0-rc2). You can discard this RR.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119110/#review61614
-----------------------------------------------------------


On July 4, 2014, 9:40 a.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119110/
> -----------------------------------------------------------
> 
> (Updated July 4, 2014, 9:40 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
>     KProtocolManager: Fix double mutex locking on a non recursive mutex
> 
>     Currently called reparseConfiguration results in an infinite block
>     because the code tries to lock the mutex twice even though it is not a
>     recursive mutex.
> 
>     Stack trace -
>      KProtocolManager::entryMap -> Tries to lock it
>      KIO::SlaveConfigPrivate::readGlobalConfig
>      KIO::SlaveConfig::reset
>      KProtocolManager::reparseConfiguration -> Holds the lock
> 
>     This can easily be solved by making the mutex recursive, but that breaks
>     the asserts as they are using QMutex::tryLock to check if the mutex is
>     locked. With the mutex being recursive tryLock just results in them
>     getting locked again.
> 
>     I'm not sure if this is the correct way of fixing it
> 
> Here is the full backtrace if anyone is interested -
> 
> #0  0x00007ffff328a359 in syscall () from /usr/lib/libc.so.6
> #1  0x00007ffff3e21830 in _q_futex (addr=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>, op=0, val=3, timeout=0x0)
>     at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:154
> #2  0x00007ffff3e21a04 in lockInternal_helper<false> (d_ptr=..., timeout=-1, elapsedTimer=0x0) at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:195
> #3  0x00007ffff3e21891 in QBasicMutex::lockInternal (this=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>)
>     at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:211
> #4  0x00007ffff3e2164c in QMutex::lock (this=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>)
>     at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex.cpp:225
> #5  0x00007ffff2eed989 in QMutexLocker::QMutexLocker (this=0x7fffffffc6f0, m=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>)
>     at /opt/qt5/include/QtCore/qmutex.h:136        
> #6  0x00007ffff2ee7f27 in KProtocolManager::entryMap (group=...) at /home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:294
> #7  0x00007ffff2ee5748 in KIO::SlaveConfigPrivate::readGlobalConfig (this=0x7b5d10) at /home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:73
> #8  0x00007ffff2ee5fa6 in KIO::SlaveConfig::reset (this=0x7b6b20) at /home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:227
> #9  0x00007ffff2ee7dfc in KProtocolManager::reparseConfiguration () at /home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:278
> #10 0x00007ffff2ea37ee in KIO::SessionData::reset (this=0x7b52f0) at /home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:136
> #11 0x00007ffff2ea3289 in KIO::SessionData::configDataFor (this=0x7b52f0, configData=..., proto=...) at /home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:102
> #12 0x00007ffff2edb7f9 in KIO::SchedulerPrivate::metaDataFor (this=0x7b52d0, protocol=..., proxyList=..., url=...)
>     at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1049
> #13 0x00007ffff2edc18b in KIO::SchedulerPrivate::setupSlave (this=0x7b52d0, slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, config=0x0)
>     at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1094
> #14 0x00007ffff2edb737 in setupSlave (slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, config=0x0)
>     at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1042
> #15 0x00007ffff2eda759 in KIO::ProtoQueue::startAJob (this=0x7c11b0) at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:621
> #16 0x00007ffff2edd795 in KIO::ProtoQueue::qt_static_metacall (_o=0x7c11b0, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffffcfa0)
>     at /home/vishesh/kde5/build/frameworks/kio/src/core/moc_scheduler_p.cpp:244
> #17 0x00007ffff40879bf in QMetaObject::activate (sender=0x7c1208, signalOffset=3, local_signal_index=0, argv=0x0)
>     at /home/vishesh/kde5/qt5/qtbase/src/corelib/kernel/qobject.cpp:3680
> 
> 
> Diffs
> -----
> 
>   src/core/kprotocolmanager.cpp 63baa6d 
> 
> Diff: https://git.reviewboard.kde.org/r/119110/diff/
> 
> 
> Testing
> -------
> 
> This can be tested by running `khotnewstuff "plasmoids.knsrc"` in knewstuff/tests/. Without this patch it just blocks.
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140704/2f097d75/attachment.html>