Unit tests fixes
ClosedPublic

Authored by arrowd on May 7 2018, 3:50 PM.

Details

Summary
  • Fix segmentation fault when running test_pluginenabling on FreeBSD. No idea what caused it, but now test passes.
  • Fix some tests from gdb that were using line numbers to set breakpoints.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arrowd created this revision.May 7 2018, 3:50 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMay 7 2018, 3:50 PM
arrowd requested review of this revision.May 7 2018, 3:50 PM
arrowd updated this revision to Diff 33787.May 7 2018, 6:58 PM
  • Fix some tests from gdb that were using line numbers to set breakpoints.
arrowd retitled this revision from Fix segmentation fault when running test_pluginenabling on FreeBSD. to Unit tests fixes.May 7 2018, 6:59 PM
arrowd edited the summary of this revision. (Show Details)
mwolff requested changes to this revision.May 7 2018, 7:17 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
kdevplatform/shell/tests/test_pluginenabling.cpp
138

this doesn't change anything from a functionality point of view. Please use valgrind or similar to inspect the crash

plugins/gdb/unittests/test_gdb.cpp
679 ↗(On Diff #33787)

these all sound fine, but please make this a separate commit

This revision now requires changes to proceed.May 7 2018, 7:17 PM
arrowd added inline comments.May 8 2018, 3:39 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
138

I'm far from being a C++ expert, maybe there is some subtle UB?

I've fired up IDA Pro and here is decompiled code of patched version:

KPluginMetaData::rawData((KPluginMetaData *)&pluginInfoThis);
QString::QString(&string_KPlugin, "KPlugin");
LODWORD(v10) = QJsonObject::operator[](&pluginInfoThis, &string_KPlugin);
v25 = v10;
v26 = v11;
QJsonValueRef::toObject((QJsonValueRef *)&v27);
QString::~QString((QString *)&string_KPlugin);
QJsonObject::~QJsonObject((QJsonObject *)&pluginInfoThis);
QString::QString(&string_EnabledByDef, "EnabledByDefault");
LODWORD(v12) = QJsonObject::operator[](&v27, &string_EnabledByDef);
v21 = v12;
v22 = v13;
QString::~QString((QString *)&string_EnabledByDef);
v16 = 1;
if ( !(QJsonValueRef::isNull((QJsonValueRef *)&v21) & 1) )

Note that before QJsonValueRef::isNull is called, 3 destructors are run:

QString::~QString((QString *)&string_KPlugin);
QJsonObject::~QJsonObject((QJsonObject *)&pluginInfoThis);
QString::~QString((QString *)&string_EnabledByDef);

And here is decompiled code of the current version:

KPluginMetaData::rawData((KPluginMetaData *)&pluginInfoThis);
QString::QString(&string_KPlugin, "KPlugin");
LODWORD(v10) = QJsonObject::operator[](&pluginInfoThis, &string_KPlugin);
v23 = v10;
v24 = v11;
QJsonValueRef::toObject((QJsonValueRef *)&v25);
QString::QString(&string_EnabledByDef, "EnabledByDefault");
LODWORD(v12) = QJsonObject::operator[](&v25, &string_EnabledByDef);
v26 = v12;
v27 = v13;
QString::~QString((QString *)&string_EnabledByDef);
QJsonObject::~QJsonObject((QJsonObject *)&v25);
QString::~QString((QString *)&string_KPlugin);
QJsonObject::~QJsonObject((QJsonObject *)&pluginInfoThis);
v16 = 1;
if ( !(QJsonValueRef::isNull((QJsonValueRef *)&v26) & 1) )

There 4 destructors are run - 3 from above and additional QJsonObject::~QJsonObject((QJsonObject *)&v25);, which is an object that QJsonObject::operator[](&v25, &string_EnabledByDef); operates on. I suspect this is what causes the problem.

There also might be miscompilation on the clang side. I'll try to use 5.0 instead of 6.0.

arrowd added inline comments.May 12 2018, 8:13 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
138

Valgrind on FreeBSD is unstable and didn't yield useful results. Compiling this code with GCC is also not an easy task, because of libc++/libstdc++ conflicts. I basically stuck there and don't know where to move on.

I may try compiling with GCC or ask Clang guys if it is a miscompilation. What'd you suggest?

arrowd added inline comments.May 26 2018, 11:56 AM
kdevplatform/shell/tests/test_pluginenabling.cpp
138

I managed to come up with minimal repro testcase:

#include <QJsonObject>
 
QJsonObject makeObject()
{
    QJsonObject ret, kplugin;
 
    kplugin["EnabledByDefault"] = false;
    ret["KPlugin"] = kplugin;
 
    return ret;
}
 
int main()
{
    // works
//     auto p = makeObject()["KPlugin"].toObject();
//     const auto enabledByDefaultValue = p["EnabledByDefault"];
 
    // segfaults
    const auto enabledByDefaultValue = makeObject()["KPlugin"].toObject()["EnabledByDefault"];
 
 
    const bool enabledByDefault = enabledByDefaultValue.isNull();
    return 0;
}

I debugged this code instruction-by-instruction and there is indeed use after free there. enabledByDefaultValue is a QJsonValueRef returned from the last operator[]. It holds a reference to QJsonObject's internal data in its .d and .o fields. After the last operator[] call, the compiler emits destructors for two created QStrings as well as two created QJsonObjects. After the last ~QJsonObject finishes, the reference count drop to 0, what makes internal data to be freed and QJsonValueRef enabledByDefaultValue to reference a freed memory. Exactly as I concluded after looking on IDA output.

I catched this because I was running development branch of FreeBSD, in which free doesn't only release the memory, but also fill it with the garbage.

As you requested, here is a Valgrind output for this testcase:

[root@localhost qttst]# c++ -std=c++11 -g -I/usr/include/qt5/QtCore -I /usr/include/qt5 -fPIC -L /usr/lib64/qt5 -lQt5Core test.cpp -o fail
[root@localhost qttst]# valgrind --track-origins=yes ./fail
==26895== Memcheck, a memory error detector
==26895== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==26895== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==26895== Command: ./fail
==26895==
==26895== Invalid read of size 4
==26895== at 0x506C031: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)
==26895== by 0x400F23: main (test.cpp:18)
==26895== Address 0xa7cd3d8 is 56 bytes inside a block of size 168 free'd
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400EFF: main (test.cpp:15)
==26895== Block was alloc'd at
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400DB0: makeObject() (test.cpp:8)
==26895== by 0x400E9E: main (test.cpp:15)
==26895==
==26895== Invalid read of size 4
==26895== at 0x506C050: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)
==26895== by 0x400F23: main (test.cpp:18)
==26895== Address 0xa7cd3dc is 60 bytes inside a block of size 168 free'd
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400EFF: main (test.cpp:15)
==26895== Block was alloc'd at
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400DB0: makeObject() (test.cpp:8)
==26895== by 0x400E9E: main (test.cpp:15)
==26895==
==26895== Invalid read of size 4
==26895== at 0x506C05D: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)
==26895== by 0x400F23: main (test.cpp:18)
==26895== Address 0xa7cd3f8 is 88 bytes inside a block of size 168 free'd
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400EFF: main (test.cpp:15)
==26895== Block was alloc'd at
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400DB0: makeObject() (test.cpp:8)
==26895== by 0x400E9E: main (test.cpp:15)
==26895==
==26895== Invalid read of size 4
==26895== at 0x506ED88: QJsonValue::QJsonValue(QJsonPrivate::Data*, QJsonPrivate::Base*, QJsonPrivate::Value const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506C06A: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)
==26895== by 0x400F23: main (test.cpp:18)
==26895== Address 0xa7cd3e0 is 64 bytes inside a block of size 168 free'd
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400EFF: main (test.cpp:15)
==26895== Block was alloc'd at
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)
==26895== by 0x400DB0: makeObject() (test.cpp:8)
==26895== by 0x400E9E: main (test.cpp:15)
==26895==
==26895==
==26895== HEAP SUMMARY:
==26895== in use at exit: 18,604 bytes in 6 blocks
==26895== total heap usage: 28 allocs, 22 frees, 19,735 bytes allocated
==26895==
==26895== LEAK SUMMARY:
==26895== definitely lost: 0 bytes in 0 blocks
==26895== indirectly lost: 0 bytes in 0 blocks
==26895== possibly lost: 0 bytes in 0 blocks
==26895== still reachable: 18,604 bytes in 6 blocks
==26895== suppressed: 0 bytes in 0 blocks
==26895== Rerun with --leak-check=full to see details of leaked memory
==26895==
==26895== For counts of detected and suppressed errors, rerun with: -v
==26895== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)

The main question is whose the bug is. Is it not allowed to chain QJsonObject::operator[] and we should fix it or it is a bug in Qt itself?

kossebau added inline comments.
kdevplatform/shell/tests/test_pluginenabling.cpp
138

Thanks for investigations, @arrowdodger . Being the one who is to blame for writing this chained code and possibly having copied the same approach elsewhere, I feel bad now.

IIRC I wrote this code without having deeply looked at the API docs, at least I do not remember to have spotted that overload of operator[] for non-const calls :/

Being unsure about C++ object scope rules in such call chains, while you have your setup around, could you do me a quick favour and inspect if changing the auto to an explicit QJsonValue might save us here, or using value(QString) instead of operator[QString]?

kossebau added inline comments.May 26 2018, 12:44 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
138

The main question is whose the bug is. Is it not allowed to chain QJsonObject::operator[] and we should fix it or it is a bug in Qt itself?

IMHO it is at least a trap in the Qt API to run into which should get some respective warning in the API docs. Similar to like QStringRef has a prominent note "Warning: A QStringRef is only valid as long as the referenced string exists. If the original string is deleted, the string reference points to an invalid memory location." the same thing should be added to QJsonValueRef docs as well as possibly to the methods returning a QJsonValueRef, given with QString API one has to explicitly request such a ref, while with QJson* the Ref variant can sneek in unnoticed due to const-ness (which itself is not straight visible in the code doing related calls.

So IMHO worth an issue filing asking for such a note, and perhaps dping a blog post as PSA to the coding fellows (and padding yourself a little in public for the good detection work).

((someone should add to KDevelop the option to style const variables & methods:) E.g. making them use bold font, to reflect solidness? and non-const italic, to show the fragile nature ;) ))

arrowd added inline comments.May 26 2018, 4:52 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
138

Both using QJsonValue instead of auto or value() instead of operator[] also make the crash go away. How would you prefer me fixing this code?

kossebau added inline comments.May 28 2018, 4:07 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
138

My personal preference would be to go for explicit QJsonValue as result type then. This would be most consistent with other code I have found elsewhere in KDE spheres (incl. normal kdevelop code reading that very data property).

IIUC any temporary objects in the chained call are only destroyed once the complete call has been evaluated, so using the non-const operator[] with the QJsonValueRef should be fine inside the chained calls, just the final lvalue type should be not a reference type, but a "normal" value type.

arrowd updated this revision to Diff 35058.May 28 2018, 8:55 PM

Proper fix for test_pluginenabling.

mwolff accepted this revision.May 29 2018, 9:09 AM

ouch, thanks a lot for the investigation. This is similar to the QStringBuilder crashes one can get... Can you simplify the line based on my suggestion? then it's a clear +2 from me, please also include the analysis and valgrind output in your commit message.

kdevplatform/shell/tests/test_pluginenabling.cpp
138

this should work too:

const QJsonValue enabledByDefaultValue = pluginInfo.rawData()["KPlugin"].toObject()["EnabledByDefault"];

This revision is now accepted and ready to land.May 29 2018, 9:09 AM
This revision was automatically updated to reflect the committed changes.