- 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.
Details
Details
Diff Detail
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.
| 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. | |
| 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? | |
| 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:
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? | |
| 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]? | |
| kdevplatform/shell/tests/test_pluginenabling.cpp | ||
|---|---|---|
| 138 |
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 ;) )) | |
| 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? | |
| 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. | |
Comment Actions
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"]; | |