D12743: Unit tests fixes
Friedrich W. H. Kossebau
noreply at phabricator.kde.org
Sat May 26 12:29:36 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?
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]`?
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/7a14e21e/attachment.html>
More information about the KDevelop-devel
mailing list