Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds

Fri Nov 15 10:34:52 UTC 2013

> On Nov. 15, 2013, 11:26 a.m., Frank Reininghaus wrote:
> > I think the root cause of the corruption is that there is a "++it" in line 981, and then "remove_startup_info_internal( key )" in the following line, which calls QMap::remove(key). The latter invalidates all existing iterators for the corresponding map, so any later use of "it" is unsafe.
> > 
> > An alternative would be to use "it = startups.erase(it)" instead. QMap::erase(it) always returns a valid iterator pointing to the next item (which may be end()). That approach could also be combined with your refactored function (which looks much nicer than the threefold repetition of mostly the same code, nice!), but I guess it's a matter of taste if you prefer the Java-style iterators, or you use the STL-style ones in the correct and safe way. I prefer the STL ones though, because you can still use "*it" and "it->foo()" to dereference them.
> > 
> > Most (or all?) Qt and STL containers support "it = container.erase(it)" for the removal of an item that an iterator points to, and having "it" point to the next item (or end()) afterwards.

ah I didn't see erase() at all. It was not mentioned in the doc: "Items can be removed from the map in several ways. One way is to call remove(); this will remove any item with the given key. Another way is to use QMutableMapIterator::remove(). In addition, you can clear the entire map using clear()."

That's why I went for QMutableMapIterator. It's not like I prefer the Java-style iterators.

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113877/#review43717
-----------------------------------------------------------

On Nov. 15, 2013, 10:48 a.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113877/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2013, 10:48 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> If there were multiple KStartupInfoIds which were removed at the same time, it was possible that the map got changed while being iterated, thus the data corrupted and we hit a crash later on. This changes to a QMutableMapIterator for checking the cleanup and allowing to safely remove ids during the iteration.
> 
> 
> Diffs
> -----
> 
>   tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp 17890ff 
>   tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 
> 
> Diff: http://git.reviewboard.kde.org/r/113877/diff/
> 
> 
> Testing
> -------
> 
> When running the unit tests on the old code base it isn't guaranteed that it crashes as it's a race and also depends on X. Just run a few times and it will certainly hit the abort condition.
> 
> For reference the backtrace of the crash before fix:
> #0  0x00007ffff50131e5 in __GI_raise (sig=sig at entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff5016398 in __GI_abort () at abort.c:90
> #2  0x00007ffff5c3d402 in qt_message_fatal (context=..., message=...) at global/qlogging.cpp:979
> #3  0x00007ffff5c3afce in QMessageLogger::fatal (this=0x7fffffffbbc0, msg=0x7ffff5f8d9f8 "ASSERT: \"%s\" in file %s, line %d") at global/qlogging.cpp:384
> #4  0x00007ffff5c369e6 in qt_assert (assertion=0x7ffff5f8b7f0 "size == 0 || offset < 0 || size_t(offset) >= sizeof(QArrayData)", file=0x7ffff5f8b7b0 "../../include/QtCore/../../src/corelib/tools/qarraydata.h", line=62)
>     at global/qglobal.cpp:2088
> #5  0x00007ffff5c29bf8 in QArrayData::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
> #6  0x00007ffff5c3015e in QTypedArrayData<char>::data (this=0x6cb830) at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207
> #7  0x00007ffff5c2fde7 in QByteArray::constData (this=0x6f4760) at ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433
> #8  0x00007ffff5c53333 in qstrcmp (str1=..., str2=...) at tools/qbytearray.cpp:347
> #9  0x00007ffff7ba4625 in operator< (a1=..., a2=...) at /opt/qt5/include/QtCore/qbytearray.h:550
> #10 0x00007ffff7ba0eb9 in KStartupInfoId::operator< (this=0x693858, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175
> #11 0x00007ffff7ba67b5 in qMapLessThanKey<KStartupInfoId> (key1=..., key2=...) at /opt/qt5/include/QtCore/qmap.h:75
> #12 0x00007ffff7ba6ef3 in QMapNode<KStartupInfoId, KStartupInfo::Data>::lowerBound (this=0x693840, akey=...) at /opt/qt5/include/QtCore/qmap.h:145
> #13 0x00007ffff7ba66b4 in QMapData<KStartupInfoId, KStartupInfo::Data>::findNode (this=0x700870, akey=...) at /opt/qt5/include/QtCore/qmap.h:292
> #14 0x00007ffff7ba5a73 in QMap<KStartupInfoId, KStartupInfo::Data>::remove (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897
> #15 0x00007ffff7b9dd42 in KStartupInfo::Private::remove_startup_info_internal (this=0x65acb0, id_P=...) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:399
> #16 0x00007ffff7ba039b in KStartupInfo::Private::startups_cleanup_internal (this=0x65acb0, age_P=true) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:984
> #17 0x00007ffff7ba02bf in KStartupInfo::Private::startups_cleanup (this=0x65acb0) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:965
> #18 0x00007ffff7ba3d1c in KStartupInfo::qt_static_metacall (_o=0x7fffffffdc60, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffffffbf70) at /opt/build/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/moc_kstartupinfo.cpp:106
> #19 0x00007ffff5ef8c9f in QMetaObject::activate (sender=0x684d50, signalOffset=3, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3530
> #20 0x00007ffff5ef84f0 in QMetaObject::activate (sender=0x684d50, m=0x7ffff63ab980 <QTimer::staticMetaObject>, local_signal_index=0, argv=0x0) at kernel/qobject.cpp:3405
> #21 0x00007ffff5f88fc3 in QTimer::timeout (this=0x684d50) at .moc/moc_qtimer.cpp:189
> #22 0x00007ffff5f03bc5 in QTimer::timerEvent (this=0x684d50, e=0x7fffffffca30) at kernel/qtimer.cpp:255
> #23 0x00007ffff5ef2314 in QObject::event (this=0x684d50, e=0x7fffffffca30) at kernel/qobject.cpp:1101
> #24 0x00007ffff710e24e in QApplicationPrivate::notify_helper (this=0x60f010, receiver=0x684d50, e=0x7fffffffca30) at kernel/qapplication.cpp:3467
> #25 0x00007ffff710b805 in QApplication::notify (this=0x7fffffffdc90, receiver=0x684d50, e=0x7fffffffca30) at kernel/qapplication.cpp:2888
> #26 0x00007ffff5eb6908 in QCoreApplication::notifyInternal (this=0x7fffffffdc90, receiver=0x684d50, event=0x7fffffffca30) at kernel/qcoreapplication.cpp:878
> #27 0x00007ffff5eba4f1 in QCoreApplication::sendEvent (receiver=0x684d50, event=0x7fffffffca30) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:232
> #28 0x00007ffff5f2c450 in QTimerInfoList::activateTimers (this=0x654ec0) at kernel/qtimerinfo_unix.cpp:643
> #29 0x00007ffff5f2d764 in timerSourceDispatch (source=0x654e60) at kernel/qeventdispatcher_glib.cpp:185
> #30 0x00007ffff25a8ea6 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #31 0x00007ffff25a91f8 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #32 0x00007ffff25a929c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #33 0x00007ffff5f2e0f9 in QEventDispatcherGlib::processEvents (this=0x64ed40, flags=...) at kernel/qeventdispatcher_glib.cpp:426
> #34 0x00007fffefcca744 in QPAEventDispatcherGlib::processEvents (this=0x64ed40, flags=...) at eventdispatchers/qeventdispatcher_glib.cpp:123
> #35 0x00007ffff5eb6e46 in QCoreApplication::processEvents (flags=..., maxtime=11000) at kernel/qcoreapplication.cpp:1076
> #36 0x0000000000403ecc in QTest::qWait (ms=11000) at /opt/qt5/include/QtTest/qtestsystem.h:66
> #37 0x000000000040690c in KStartupInfo_UnitTest::dontCrashCleanup (this=0x7fffffffdc50) at /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp:128
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20131115/cefc57ab/attachment-0001.html>