Differential D845

TopContextDynamicData: Fix bug in loadPartialData
ClosedPublic
Actions

Authored by kfunk on Jan 20 2016, 8:38 AM.

Details

Reviewers
mwolff
Commits
R33:c8fa1a6ba1f2: TopContextDynamicData: Fix bug in loadPartialData
R33:14df9ba1a88e: TopContextDynamicData: Fix bug in loadPartialData
R32:14df9ba1a88e: TopContextDynamicData: Fix bug in loadPartialData
R32:c8fa1a6ba1f2: TopContextDynamicData: Fix bug in loadPartialData
Summary
Detected using ASAN

Regression introduced by 49e4b656f0e54bff882f03efe04feedff5994ed1
==31328==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x60b0007dc6e4 at pc 0x7f8c81dd210f bp 0x7ffc0dba13f0 sp 0x7ffc0dba13e8
READ of size 8 at 0x60b0007dc6e4 thread T0
    #0 0x7f8c81dd210e in
QList<KDevelop::IndexedDUContext>::node_construct(QList<KDevelop::IndexedDUContext>::Node*,
KDevelop::IndexedDUContext const&)
/usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:405:39
    #1 0x7f8c81dd210e in
QList<KDevelop::IndexedDUContext>::append(KDevelop::IndexedDUContext
const&) /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:569
    #2 0x7f8c81dd210e in
QList<KDevelop::IndexedDUContext>::operator<<(KDevelop::IndexedDUContext
const&) /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:355
    #3 0x7f8c81dd210e in
KDevelop::TopDUContextDynamicData::loadImporters(unsigned
int)::$_0::operator()(KDevelop::TopDUContextData const*) const
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/topducontextdynamicdata.cpp:505
    #4 0x7f8c81dd210e in void (anonymous
namespace)::loadPartialData<KDevelop::TopDUContextDynamicData::loadImporters(unsigned
int)::$_0>(unsigned int,
KDevelop::TopDUContextDynamicData::loadImporters(unsigned int)::$_0)
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/topducontextdynamicdata.cpp:175
    #5 0x7f8c81dd210e in
KDevelop::TopDUContextDynamicData::loadImporters(unsigned int)
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/topducontextdynamicdata.cpp:502
    #6 0x7f8c81e7bc8d in KDevelop::ParsingEnvironmentFile::importers()
const
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/parsingenvironment.cpp:205:11
    #7 0x7f8c81d42a12 in
KDevelop::DUChainPrivate::addContextsForRemoval(QSet<unsigned int>&,
KDevelop::IndexedTopDUContext)
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/duchain.cpp:1015:81
    #8 0x7f8c81d12dd6 in KDevelop::DUChainPrivate::cleanupTopContexts()
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/duchain.cpp:985:9
    #9 0x7f8c81d08f64 in KDevelop::DUChain::shutdown()
/home/kfunk/devel/src/kf5/kdevplatform-stable/language/duchain/duchain.cpp:1572:5
    #10 0x7f8c8a4e6c07 in KDevelop::Core::cleanup()
/home/kfunk/devel/src/kf5/kdevplatform-stable/shell/core.cpp:454:9
    #11 0x7f8c8a4e5add in KDevelop::Core::shutdown()
/home/kfunk/devel/src/kf5/kdevplatform-stable/shell/core.cpp:409:9
    #12 0x7f8c8a487499 in KDevelop::MainWindow::~MainWindow()
/home/kfunk/devel/src/kf5/kdevplatform-stable/shell/mainwindow.cpp:147:9
    #13 0x7f8c8a487818 in KDevelop::MainWindow::~MainWindow()
/home/kfunk/devel/src/kf5/kdevplatform-stable/shell/mainwindow.cpp:144:1
    #14 0x7f8c8a487818 in KDevelop::MainWindow::~MainWindow()
/home/kfunk/devel/src/kf5/kdevplatform-stable/shell/mainwindow.cpp:144
    #15 0x7f8c851797cf in QObject::event(QEvent*)
(/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2b67cf)
    #16 0x7f8c85bda74a in QWidget::event(QEvent*)
(/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x19e74a)
    #17 0x7f8c85cf0a4a in QMainWindow::event(QEvent*)
(/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x2b4a4a)
    #18 0x7f8c8893e2a6 in KMainWindow::event(QEvent*)
(/usr/lib/x86_64-linux-gnu/libKF5XmlGui.so.5+0x7a2a6)
    #19 0x7f8c88976754 in KXmlGuiWindow::event(QEvent*)
(/usr/lib/x86_64-linux-gnu/libKF5XmlGui.so.5+0xb2754)
    #20 0x7f8c85b979db in QApplicationPrivate::notify_helper(QObject*,
QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15b9db)
    #21 0x7f8c85b9cea5 in QApplication::notify(QObject*, QEvent*)
(/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x160ea5)
    #22 0x7f8c85149d7a in QCoreApplication::notifyInternal(QObject*,
QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x286d7a)
    #23 0x7f8c8514c175 in
QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15b9db)
    #21 0x7f8c85b9cea5 in QApplication::notify(QObject*, QEvent*)
(/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x160ea5)
    #22 0x7f8c85149d7a in QCoreApplication::notifyInternal(QObject*,
QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x286d7a)
    #23 0x7f8c8514c175 in
QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*)
(/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x289175)
    #24 0x7f8c851a00e2
(/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2dd0e2)
    #25 0x7f8c7c6c4ff6 in g_main_context_dispatch
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x49ff6)
    #26 0x7f8c7c6c524f  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a24f)
    #27 0x7f8c7c6c52fb in g_main_context_iteration
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a2fb)
    #28 0x7f8c851a04ee in
QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>)
(/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2dd4ee)
    #29 0x7f8c85147509 in
QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>)
(/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x284509)
    #30 0x7f8c8514f5eb in QCoreApplication::exec()
(/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28c5eb)
    #31 0x502967 in main
/home/kfunk/devel/src/kf5/kdevelop-stable/app/main.cpp:652:12
    #32 0x7f8c83c4ea3f in __libc_start_main
/build/buildd/glibc-2.21/csu/libc-start.c:289
    #33 0x43d948 in _start
(/home/kfunk/devel/install/kf5-stable/bin/kdevelop+0x43d948)
AddressSanitizer can not describe address in more detail (wild memory
access suspected).
SUMMARY: AddressSanitizer: heap-buffer-overflow
/usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:405
QList<KDevelop::IndexedDUContext>::node_construct(QList<KDevelop::IndexedDUContext>::Node*,
KDevelop::IndexedDUContext const&)
Test Plan

No longer crashes on exit.

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kfunk updated this revision to Diff 2033.Jan 20 2016, 8:38 AM
kfunk retitled this revision from to TopContextDynamicData: Fix bug in loadPartialData Detected using ASAN. Fixes frequent crash-on-exit. Regression introduced by 49e4b656f0e54bff882f03efe04feedff5994ed1 ==31328==ERROR: AddressSanitizer: heap-buffer-overflow on address....
kfunk updated this object.
kfunk edited the test plan for this revision. (Show Details)
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 20 2016, 8:38 AM
kfunk updated this object.Jan 20 2016, 8:40 AM
kfunk edited the test plan for this revision. (Show Details)
kfunk updated this revision to Diff 2034.Jan 20 2016, 8:54 AM

Load partial data in loadUrl again.

All other users want to load all of TopDUContextData (including the appended lists)

Updating D845: TopContextDynamicData: Fix bug in loadPartialData

Detected using ASAN. Fixes frequent crash-on-exit.

Regression introduced by 49e4b656f0e54bff882f03efe04feedff5994ed1

31328==ERROR: AddressSanitizer: heap-buffer-overflow on address...

kfunk retitled this revision from TopContextDynamicData: Fix bug in loadPartialData Detected using ASAN. Fixes frequent crash-on-exit. Regression introduced by 49e4b656f0e54bff882f03efe04feedff5994ed1 ==31328==ERROR: AddressSanitizer: heap-buffer-overflow on address... to TopContextDynamicData: Fix bug in loadPartialData.Jan 20 2016, 8:54 AM
kfunk added a reviewer: mwolff.
mwolff edited edge metadata.Jan 20 2016, 12:00 PM

Ouch! Sorry for that mess... Any idea how we could unit test this?

language/duchain/topducontextdynamicdata.cpp
174 ↗(On Diff #2034)

Shouldn't we always use readValue?

Also good gotcha! No compiler warns us in this case that readValue is "set but not used" :(

kfunk added inline comments.Jan 20 2016, 12:08 PM
language/duchain/topducontextdynamicdata.cpp
174 ↗(On Diff #2034)

ASAN is just awesome, I encourage everyone to re-build KDevelop with it :)

Re. "always use readValue". Well the partial load is clearly an optimization attempt, I'm fine with leaving it as-is. I've noticed we're loading quite a few top context during shutdown; any performance boost there is likely useful.

mwolff accepted this revision.Jan 20 2016, 2:37 PM
mwolff edited edge metadata.
mwolff added inline comments.
language/duchain/topducontextdynamicdata.cpp
174 ↗(On Diff #2034)

Ah, now I understand the code! I assumed readValue <= sizeof(TopDUContextData) but of course that is not the case, but rather the opposite. So +1 from my side and thanks for this patch.

This revision is now accepted and ready to land.Jan 20 2016, 2:37 PM
mwolff added inline comments.Jan 20 2016, 2:37 PM
language/duchain/topducontextdynamicdata.cpp
174 ↗(On Diff #2034)

Actually, could you add an assert that verifies readValue >= sizeof(TopDUContextData)?

Closed by commit R33:14df9ba1a88e: TopContextDynamicData: Fix bug in loadPartialData (authored by kfunk). · Explain WhyJan 20 2016, 2:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 2038

View Options

language/duchain/topducontextdynamicdata.cpp

Loading...