<table><tr><td style="">kossebau added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12743">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12743#inline-67955">View Inline</a><span style="color: #4b4d51; font-weight: bold;">arrowdodger</span> wrote in <span style="color: #4b4d51; font-weight: bold;">test_pluginenabling.cpp:138</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I managed to come up with minimal repro testcase:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">#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;
}</pre></div>
<p style="padding: 0; margin: 8px;">I debugged this code instruction-by-instruction and there is indeed use after free there. <tt style="background: #ebebeb; font-size: 13px;">enabledByDefaultValue</tt> is a <tt style="background: #ebebeb; font-size: 13px;">QJsonValueRef</tt> returned from the last <tt style="background: #ebebeb; font-size: 13px;">operator[]</tt>. It holds a reference to QJsonObject's internal data in its <tt style="background: #ebebeb; font-size: 13px;">.d</tt> and <tt style="background: #ebebeb; font-size: 13px;">.o</tt> fields. After the last <tt style="background: #ebebeb; font-size: 13px;">operator[]</tt> call, the compiler emits destructors for two created <tt style="background: #ebebeb; font-size: 13px;">QString</tt>s as well as two created <tt style="background: #ebebeb; font-size: 13px;">QJsonObject</tt>s. After the last <tt style="background: #ebebeb; font-size: 13px;">~QJsonObject</tt> finishes, the reference count drop to 0, what makes internal data to be freed and <tt style="background: #ebebeb; font-size: 13px;">QJsonValueRef enabledByDefaultValue</tt> to reference a freed memory. Exactly as I concluded after looking on IDA output.</p>
<p style="padding: 0; margin: 8px;">I catched this because I was running development branch of FreeBSD, in which <tt style="background: #ebebeb; font-size: 13px;">free</tt> doesn't only release the memory, but also fill it with the garbage.</p>
<p style="padding: 0; margin: 8px;">As you requested, here is a Valgrind output for this testcase:</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">[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<br />
[root@localhost qttst]# valgrind --track-origins=yes ./fail <br />
==26895== Memcheck, a memory error detector<br />
==26895== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.<br />
==26895== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info<br />
==26895== Command: ./fail<br />
==26895== <br />
==26895== Invalid read of size 4<br />
==26895== at 0x506C031: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)<br />
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)<br />
==26895== by 0x400F23: main (test.cpp:18)<br />
==26895== Address 0xa7cd3d8 is 56 bytes inside a block of size 168 free'd<br />
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)<br />
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400EFF: main (test.cpp:15)<br />
==26895== Block was alloc'd at<br />
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)<br />
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400DB0: makeObject() (test.cpp:8)<br />
==26895== by 0x400E9E: main (test.cpp:15)<br />
==26895== <br />
==26895== Invalid read of size 4<br />
==26895== at 0x506C050: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)<br />
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)<br />
==26895== by 0x400F23: main (test.cpp:18)<br />
==26895== Address 0xa7cd3dc is 60 bytes inside a block of size 168 free'd<br />
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)<br />
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400EFF: main (test.cpp:15)<br />
==26895== Block was alloc'd at<br />
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)<br />
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400DB0: makeObject() (test.cpp:8)<br />
==26895== by 0x400E9E: main (test.cpp:15)<br />
==26895== <br />
==26895== Invalid read of size 4<br />
==26895== at 0x506C05D: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)<br />
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)<br />
==26895== by 0x400F23: main (test.cpp:18)<br />
==26895== Address 0xa7cd3f8 is 88 bytes inside a block of size 168 free'd<br />
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)<br />
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400EFF: main (test.cpp:15)<br />
==26895== Block was alloc'd at<br />
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)<br />
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400DB0: makeObject() (test.cpp:8)<br />
==26895== by 0x400E9E: main (test.cpp:15)<br />
==26895== <br />
==26895== Invalid read of size 4<br />
==26895== at 0x506ED88: QJsonValue::QJsonValue(QJsonPrivate::Data*, QJsonPrivate::Base*, QJsonPrivate::Value const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506C06A: QJsonObject::valueAt(int) const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8C6: QJsonValueRef::toValue() const (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x40106F: QJsonValueRef::type() const (qjsonvalue.h:157)<br />
==26895== by 0x4010C7: QJsonValueRef::isNull() const (qjsonvalue.h:158)<br />
==26895== by 0x400F23: main (test.cpp:18)<br />
==26895== Address 0xa7cd3e0 is 64 bytes inside a block of size 168 free'd<br />
==26895== at 0x4C29D1D: free (vg_replace_malloc.c:530)<br />
==26895== by 0x506AD20: QJsonObject::~QJsonObject() (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400EFF: main (test.cpp:15)<br />
==26895== Block was alloc'd at<br />
==26895== at 0x4C28C23: malloc (vg_replace_malloc.c:299)<br />
==26895== by 0x506B359: QJsonObject::detach2(unsigned int) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506B84B: QJsonObject::insert(QString const&, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506C0C6: QJsonObject::setValueAt(int, QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x506F8A9: QJsonValueRef::operator=(QJsonValue const&) (in /usr/lib64/libQt5Core.so.5.9.2)<br />
==26895== by 0x400DB0: makeObject() (test.cpp:8)<br />
==26895== by 0x400E9E: main (test.cpp:15)<br />
==26895== <br />
==26895== <br />
==26895== HEAP SUMMARY:<br />
==26895== in use at exit: 18,604 bytes in 6 blocks<br />
==26895== total heap usage: 28 allocs, 22 frees, 19,735 bytes allocated<br />
==26895== <br />
==26895== LEAK SUMMARY:<br />
==26895== definitely lost: 0 bytes in 0 blocks<br />
==26895== indirectly lost: 0 bytes in 0 blocks<br />
==26895== possibly lost: 0 bytes in 0 blocks<br />
==26895== still reachable: 18,604 bytes in 6 blocks<br />
==26895== suppressed: 0 bytes in 0 blocks<br />
==26895== Rerun with --leak-check=full to see details of leaked memory<br />
==26895== <br />
==26895== For counts of detected and suppressed errors, rerun with: -v<br />
==26895== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)</p></blockquote>
<p style="padding: 0; margin: 8px;">The main question is whose the bug is. Is it not allowed to chain <tt style="background: #ebebeb; font-size: 13px;">QJsonObject::operator[]</tt> and we should fix it or it is a bug in Qt itself?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">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?</p></blockquote>
<p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">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).</p>
<p style="padding: 0; margin: 8px;">((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 ;) ))</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12743">https://phabricator.kde.org/D12743</a></div></div><br /><div><strong>To: </strong>arrowdodger, KDevelop, mwolff<br /><strong>Cc: </strong>kossebau, mwolff, kdevelop-devel<br /></div>