D12743: Unit tests fixes
Friedrich W. H. Kossebau
noreply at phabricator.kde.org
Sat May 26 12:45:00 UTC 2018
kossebau added inline comments.
INLINE COMMENTS
> arrowdodger wrote in 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 `QString`s as well as two created `QJsonObject`s. 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 at 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 at 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?
> 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 ;) ))
REPOSITORY
R32 KDevelop
REVISION DETAIL
https://phabricator.kde.org/D12743
To: arrowdodger, #kdevelop, mwolff
Cc: kossebau, mwolff, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20180526/4cc2d939/attachment-0001.html>
More information about the KDevelop-devel
mailing list