From 70a3c9ce3c40ae2fb288b6dfc9d6fd66d623c1ba Mon Sep 17 00:00:00 2001 From: "Anthony J. Thibault" Date: Wed, 27 Jun 2018 16:06:23 -0700 Subject: [PATCH] Fix for heap-corruption in Settings::saveAll due to implicitly shared QStrings Using gflags on windows and enabling full page heap verification, I was able to detect this as a source of memory corruption. https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/enable-page-heap Running interface with pageheap before and after, this change leads me to believe that this is a source of some of our heap-corruption crashes on backtrace. Specifically these 19 crashes in the last month, and possibly more. https://highfidelity.sp.backtrace.io/dashboard/highfidelity/project/Interface/query-builder?qb=%7B%22mode%22%3A%22aggregate%22%2C%22sorting%22%3A%7B%22aggFactor%22%3A%22count%22%2C%22aggSortOrder%22%3A%22descending%22%2C%22selectFactor%22%3A%22timestamp%22%2C%22selectSortOrder%22%3A%22descending%22%2C%22sortedColumnNames%22%3A%5B%5D%7D%2C%22dateTimePicker%22%3A%7B%22granularity%22%3A%221M%22%2C%22start%22%3A%222018-05-27T07%3A00%3A00.000Z%22%2C%22end%22%3A%222018-06-27T23%3A05%3A29.399Z%22%7D%7D&qn=&query=%7B%22filter%22%3A%5B%7B%22callstack%22%3A%5B%5B%22contains%22%2C%22saveAll%22%5D%5D%2C%22timestamp%22%3A%5B%5B%22at-least%22%2C1527404400%5D%2C%5B%22at-most%22%2C1530140729%5D%5D%7D%5D%2C%22fold%22%3A%7B%22timestamp%22%3A%5B%5B%22range%22%5D%2C%5B%22bin%22%2C32%2C1527404400%2C1530140729%5D%5D%7D%2C%22group%22%3A%5B%22classifiers%22%5D%2C%22select%22%3A%5B%22timestamp%22%2C%22fingerprint%22%2C%22_deleted%22%2C%22callstack%22%2C%22object%22%5D%7D --- interface/src/scripting/SettingsScriptingInterface.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/interface/src/scripting/SettingsScriptingInterface.cpp b/interface/src/scripting/SettingsScriptingInterface.cpp index 2f14c33dc7..afafe1a350 100644 --- a/interface/src/scripting/SettingsScriptingInterface.cpp +++ b/interface/src/scripting/SettingsScriptingInterface.cpp @@ -35,5 +35,8 @@ QVariant SettingsScriptingInterface::getValue(const QString& setting, const QVar } void SettingsScriptingInterface::setValue(const QString& setting, const QVariant& value) { - Setting::Handle(setting).set(value); + // Make a deep-copy of the string. + // Dangling pointers can occur with QStrings that are implicitly shared from a QScriptEngine. + QString deepCopy = QString::fromUtf16(setting.utf16()); + Setting::Handle(deepCopy).set(value); }