Fix ASan error in test_cmakemanager by catching all signals before objects they are delivered to are deleted.
Needs RevisionPublic

Authored by arrowd on Feb 5 2019, 12:43 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

In TestCore::shutdown() there is a qWait(1) line that causes signals from queued connections to arrive. At this point, however, the reciever object is deleted already.
Fix testReload testcase by waiting for these signals inside the test function.

Full ASan report:

PASS   : TestCMakeManager::testReload()
=================================================================
==8081==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040006073e0 at pc 0x00080085b9f8 bp 0x7fffffffa8f0 sp 0x7fffffffa8e8
READ of size 8 at 0x6040006073e0 thread T0
    #0 0x80085b9f7 in KDevelop::StatusBar::showProgress(KDevelop::IStatus*, int, int, int) /home/arr/projects/kdevelop/kdevplatform/shell/statusbar.cpp:243:56
    #1 0x800b1683c in KDevelop::StatusBar::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/kdevplatform/shell/KDevPlatformShell_autogen/EWIEGA46WW/moc_statusbar.cpp:115:21
    #2 0x804e51650 in QObject::event(QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x451650)
    #3 0x8040b755e in QWidget::event(QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x2b755e)
    #4 0x804213241 in QStatusBar::event(QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x413241)
    #5 0x80407dee0 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27dee0)
    #6 0x80407f27c in QApplication::notify(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27f27c)
    #7 0x804e26ef0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x426ef0)
    #8 0x804e27db8 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/usr/local/lib/qt5/libQt5Core.so.5+0x427db8)
    #9 0x804e7c347  (/usr/local/lib/qt5/libQt5Core.so.5+0x47c347)
    #10 0x8069f0b76 in g_main_context_dispatch (/usr/local/lib/libglib-2.0.so.0+0xc9b76)
    #11 0x8069f0f02  (/usr/local/lib/libglib-2.0.so.0+0xc9f02)
    #12 0x8069f0fb3 in g_main_context_iteration (/usr/local/lib/libglib-2.0.so.0+0xc9fb3)
    #13 0x804e7bda5 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/local/lib/qt5/libQt5Core.so.5+0x47bda5)
    #14 0x804e22a3d in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/local/lib/qt5/libQt5Core.so.5+0x422a3d)
    #15 0x803c9fbfd in KJob::exec() (/usr/local/lib/libKF5CoreAddons.so.5+0x5cbfd)
    #16 0x8007dbdc2 in KDevelop::ProjectPrivate::initProjectFiles() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:229:24
    #17 0x8007d222f in KDevelop::Project::open(KDevelop::Path const&) /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:494:13
    #18 0x800798ae2 in KDevelop::ProjectControllerPrivate::importProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:353:24
    #19 0x800786613 in KDevelop::ProjectController::openProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:884:12
    #20 0x2d9a0e in loadProject(QString const&, QString const&) /home/arr/projects/kdevelop/plugins/cmake/tests/testhelpers.h:138:51
    #21 0x2edfbf in TestCMakeManager::testExecutableOutputPath() /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:381:25
    #22 0x30d3aa in TestCMakeManager::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/plugins/cmake/tests/test_cmakemanager_autogen/EWIEGA46WW/moc_test_cmakemanager.cpp:153:22
    #23 0x804e320ce in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/local/lib/qt5/libQt5Core.so.5+0x4320ce)
    #24 0x8004611f8  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e1f8)
    #25 0x800461bee  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ebee)
    #26 0x800462988  (/usr/local/lib/qt5/libQt5Test.so.5+0x1f988)
    #27 0x80046317b in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x2017b)
    #28 0x800462ef3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1fef3)
    #29 0x2da091 in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1

0x6040006073e0 is located 16 bytes inside of 40-byte region [0x6040006073d0,0x6040006073f8)
freed by thread T0 here:
    #0 0x2d31f2 in operator delete(void*) /usr/src/contrib/compiler-rt/lib/asan/asan_new_delete.cc:149:3
    #1 0x8007cff31 in KDevelop::ProjectProgress::~ProjectProgress() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:105:1
    #2 0x8007d0d42 in KDevelop::Project::~Project() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:427:5
    #3 0x8007d0d88 in KDevelop::Project::~Project() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:426:1
    #4 0x804e51461 in QObject::event(QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x451461)
    #5 0x80407dee0 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27dee0)
    #6 0x80407f27c in QApplication::notify(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27f27c)
    #7 0x804e26ef0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x426ef0)
    #8 0x804e27db8 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/usr/local/lib/qt5/libQt5Core.so.5+0x427db8)
    #9 0x800461331  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e331)
    #10 0x800461bee  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ebee)
    #11 0x800462988  (/usr/local/lib/qt5/libQt5Test.so.5+0x1f988)
    #12 0x80046317b in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x2017b)
    #13 0x800462ef3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1fef3)
    #14 0x2da091 in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1

previously allocated by thread T0 here:
    #0 0x2d2612 in operator new(unsigned long) /usr/src/contrib/compiler-rt/lib/asan/asan_new_delete.cc:92:3
    #1 0x8007d0a57 in KDevelop::Project::Project(QObject*) /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:421:19
    #2 0x800798908 in KDevelop::ProjectControllerPrivate::importProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:350:29
    #3 0x800786613 in KDevelop::ProjectController::openProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:884:12
    #4 0x2d9a0e in loadProject(QString const&, QString const&) /home/arr/projects/kdevelop/plugins/cmake/tests/testhelpers.h:138:51
    #5 0x2ec539 in TestCMakeManager::testReload() /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:338:25
    #6 0x30d39c in TestCMakeManager::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/plugins/cmake/tests/test_cmakemanager_autogen/EWIEGA46WW/moc_test_cmakemanager.cpp:152:22
    #7 0x804e320ce in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/local/lib/qt5/libQt5Core.so.5+0x4320ce)
    #8 0x8004611f8  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e1f8)
    #9 0x800461bee  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ebee)
    #10 0x800462988  (/usr/local/lib/qt5/libQt5Test.so.5+0x1f988)
    #11 0x80046317b in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x2017b)
    #12 0x800462ef3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1fef3)
    #13 0x2da091 in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1

Diff Detail

Repository
R32 KDevelop
Branch
asan_fix
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7909
Build 7927: arc lint + arc unit
arrowd created this revision.Feb 5 2019, 12:43 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptFeb 5 2019, 12:43 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
arrowd requested review of this revision.Feb 5 2019, 12:43 PM
mwolff requested changes to this revision.Feb 10 2019, 7:04 PM
mwolff added a subscriber: mwolff.

I'm afraid to say that this is just a workaround, not a proper fix... I ran into this issue recently too - the real problem - imo - is that we synchronously execute the stat jobs in KDevelop::ProjectPrivate::initProjectFiles... if you look at the ASAN report you'll see that TestCore::shutdown isn't referenced there at all!

it's not really an option to add random waits to the tests in the hope to workaround the issue everywhere. Rather, we should try to remove such nested eventloops somehow, even if that means refactoring the codebase...

This revision now requires changes to proceed.Feb 10 2019, 7:04 PM

I have little Qt experience, why nested event loops are bad? What are options to avoid them?

I'm happy to invest some time into it, but need a bit of guidance.

the proper way would be to have Project::open return void, and instead report failure via a new 'failedToOpen' signal. And then internally handle the async code without a nested event loop. We'll probably need this in more places, and I wonder what the current best-practice is for that. Eventually async/await can be used, but we want to have something that works with the compilers we support today. Hm. I'd like to prevent us from trying to handle async code in an adhoc fashion. Rather, we should try to adopt a proper framework like e.g. KAsync? QtPromise? Something similar?

The problem with nested event loops are that they introduce execution flows that are very hard to reason about and can easily cause issues like the one you see here. Quite often, you can run into issues where an object spawns a nested eventloop, which then somehow handles a deleteLater event for the object itself, thereby destroying the object. When the nested eventloop exits, this was destroyed and anything can happen. I believe something like that happens here too.

KAsync looks a bit unmaintained. On the other hand, QtPromise seems to be an active project. Should I start looking into the latter?

yes, QtPromise or AsyncFuture (https://github.com/benlau/asyncfuture) could be used - I wouldn't be opposed to introducing it as a thirdparty dependency (or git submodule)

@brauch, @kfunk, @apol what do you think? we could then leverage that new utility in quite a few places where we currently deal with async code in an ad-hoc fashion

kfunk added a comment.Mon, Apr 1, 5:44 PM

As long as it integrates well with the rest of the KDevelop build I'm fine. Would probably just copy the QtPromise source tree into maybe kdevelop.git:3rdparty/ and create a CMakeLists.txt which creates a STATIC or OBJECT library for it? QtPromise looks easy enough to build.

arrowd added a comment.Thu, Apr 4, 6:36 PM

After finishing initial implementation I found out that QtPromise requires exceptions enabled. KDevelop seem to have exceptions disabled right now.

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

Yep, this is what I'm using to make a QPromise "fail".

apol added a comment.Sun, Apr 14, 6:14 PM

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

That would mean adding a bunch of noexcept all over the place or risk quite some performance penalty. I'd prefer keeping it localised.

In D18758#450081, @apol wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

That would mean adding a bunch of noexcept all over the place or risk quite some performance penalty. I'd prefer keeping it localised.

We only get bigger binaries - and the impact is left to be seen. That said, I also would prefer to keep it localized. We could probably get away with only enabling exceptions wherever QtPromise is being used, so make it an interface property of that target in cmake?