<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;"><p style="padding: 0; margin: 8px;">Thanks for investigations, <a href="https://phabricator.kde.org/p/arrowdodger/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@arrowdodger</a> . Being the one who is to blame for writing this chained code and possibly having copied the same approach elsewhere, I feel bad now.</p>
<p style="padding: 0; margin: 8px;">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 :/</p>
<p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">auto</tt> to an explicit <tt style="background: #ebebeb; font-size: 13px;">QJsonValue</tt> might save us here, or using <tt style="background: #ebebeb; font-size: 13px;">value(QString)</tt> instead of <tt style="background: #ebebeb; font-size: 13px;">operator[QString]</tt>?</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>