From 74029c94a63a2040ccf9c8ad08abc938dd43b8f5 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Thu, 9 Dec 2021 13:28:14 +0100 Subject: [PATCH] Fix a double-free when shutting down the python engine. Python_AddObject() will steal the reference and delete it. Since we deleted m_logger ourselves too, a double free would corrupt memory on nymea shutdown. This would cause tests to crash when restarting the core within a single process by restartServer() --- .../integrations/pythonintegrationplugin.cpp | 20 +++++++++++-------- .../integrations/pythonintegrationplugin.h | 1 - .../auto/pythonplugins/testpythonplugins.cpp | 9 +++++++-- tests/testlib/nymeatestbase.cpp | 8 ++++++-- tests/testlib/nymeatestbase.h | 2 +- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/libnymea-core/integrations/pythonintegrationplugin.cpp b/libnymea-core/integrations/pythonintegrationplugin.cpp index bd25058f..306f5f8d 100644 --- a/libnymea-core/integrations/pythonintegrationplugin.cpp +++ b/libnymea-core/integrations/pythonintegrationplugin.cpp @@ -242,7 +242,6 @@ PythonIntegrationPlugin::~PythonIntegrationPlugin() s_plugins.take(this); Py_XDECREF(m_pluginModule); Py_DECREF(m_nymeaModule); - Py_XDECREF(m_logger); Py_XDECREF(m_stdOutHandler); Py_XDECREF(m_stdErrHandler); @@ -259,11 +258,13 @@ void PythonIntegrationPlugin::initPython() // Only modify the init tab once (initPython() might be called again after calling deinitPython()) static bool initTabPrepared = false; if (!initTabPrepared) { + qCDebug(dcPythonIntegrations()) << "Adding nymea module to init tab"; PyImport_AppendInittab("nymea", PyInit_nymea); initTabPrepared = true; } // Initialize the python engine and fire up threading support + qCDebug(dcPythonIntegrations()) << "Initializing Python engine"; Py_InitializeEx(0); PyEval_InitThreads(); @@ -277,6 +278,7 @@ void PythonIntegrationPlugin::deinitPython() PyEval_RestoreThread(s_mainThreadState); // Tear down the python engine + qCDebug(dcPythonIntegrations()) << "Finalizing Python engine"; Py_Finalize(); // Our main thread state is destroyed now @@ -289,6 +291,7 @@ bool PythonIntegrationPlugin::loadScript(const QString &scriptFile) PyEval_RestoreThread(s_mainThreadState); // Create a new interpreter + qCDebug(dcPythonIntegrations()) << "Creatig new Python interpreter for script:" << scriptFile; m_threadState = Py_NewInterpreter(); // Switch to the new interpreter thread state @@ -361,12 +364,19 @@ bool PythonIntegrationPlugin::loadScript(const QString &scriptFile) s_plugins.insert(this, m_pluginModule); // Set up logger with appropriate logging category + // Note: AddModule steals the reference and will delete it on Interpreter shutdown, so not keeping a reference to the logger. QString category = metadata.pluginName(); category.replace(0, 1, category[0].toUpper()); PyObject *args = Py_BuildValue("(s)", category.toUtf8().data()); - m_logger = PyObject_CallObject((PyObject*)&PyNymeaLoggingHandlerType, args); + PyObject *logger = PyObject_CallObject((PyObject*)&PyNymeaLoggingHandlerType, args); Py_DECREF(args); + int loggerAdded = PyModule_AddObject(m_pluginModule, "logger", logger); + if (loggerAdded != 0) { + qCWarning(dcPythonIntegrations()) << "Failed to add the logger object"; + Py_DECREF(logger); + } + // Override stdout and stderr args = Py_BuildValue("(si)", category.toUtf8().data(), QtMsgType::QtDebugMsg); m_stdOutHandler = PyObject_CallObject((PyObject*)&PyStdOutHandlerType, args); @@ -377,12 +387,6 @@ bool PythonIntegrationPlugin::loadScript(const QString &scriptFile) PySys_SetObject("stderr", m_stdErrHandler); Py_DECREF(args); - int loggerAdded = PyModule_AddObject(m_pluginModule, "logger", m_logger); - if (loggerAdded != 0) { - qCWarning(dcPythonIntegrations()) << "Failed to add the logger object"; - Py_DECREF(m_logger); - m_logger = nullptr; - } // Export metadata ids into module exportIds(); diff --git a/libnymea-core/integrations/pythonintegrationplugin.h b/libnymea-core/integrations/pythonintegrationplugin.h index 89e555aa..8f3c4a92 100644 --- a/libnymea-core/integrations/pythonintegrationplugin.h +++ b/libnymea-core/integrations/pythonintegrationplugin.h @@ -81,7 +81,6 @@ private: PyObject *m_nymeaModule = nullptr; // The imported plugin module (the plugin.py) PyObject *m_pluginModule = nullptr; - PyObject *m_logger = nullptr; PyObject *m_stdOutHandler = nullptr; PyObject *m_stdErrHandler = nullptr; diff --git a/tests/auto/pythonplugins/testpythonplugins.cpp b/tests/auto/pythonplugins/testpythonplugins.cpp index c15dd764..e2b01471 100644 --- a/tests/auto/pythonplugins/testpythonplugins.cpp +++ b/tests/auto/pythonplugins/testpythonplugins.cpp @@ -51,17 +51,22 @@ private slots: void initTestCase(); + void testRestartServer(); + void setupAndRemoveThing(); void testDiscoverPairAndRemoveThing(); }; +void TestPythonPlugins::testRestartServer() +{ + NymeaTestBase::restartServer(); +} void TestPythonPlugins::initTestCase() { - NymeaTestBase::initTestCase(); - QLoggingCategory::setFilterRules("*.debug=false\n" + NymeaTestBase::initTestCase("*.debug=false\n*.info=false\n*.warning=false\n" "Tests.debug=true\n" "PyMock.debug=true\n" "PythonIntegrations.debug=true\n" diff --git a/tests/testlib/nymeatestbase.cpp b/tests/testlib/nymeatestbase.cpp index cdc687f6..32ea0847 100644 --- a/tests/testlib/nymeatestbase.cpp +++ b/tests/testlib/nymeatestbase.cpp @@ -53,7 +53,7 @@ NymeaTestBase::NymeaTestBase(QObject *parent) : QCoreApplication::instance()->setOrganizationName("nymea-test"); } -void NymeaTestBase::initTestCase() +void NymeaTestBase::initTestCase(const QString &loggingRules) { qCDebug(dcTests) << "NymeaTestBase starting."; @@ -71,7 +71,11 @@ void NymeaTestBase::initTestCase() NymeaSettings nymeadSettings(NymeaSettings::SettingsRoleGlobal); nymeadSettings.clear(); - QLoggingCategory::setFilterRules("*.debug=false\nApplication.debug=true\nTests.debug=true\nMock.debug=true"); + if (loggingRules.isEmpty()) { + QLoggingCategory::setFilterRules("*.debug=false\nApplication.debug=true\nTests.debug=true\nMock.debug=true"); + } else { + QLoggingCategory::setFilterRules(loggingRules); + } // Start the server qCDebug(dcTests()) << "Setting up nymea core instance"; diff --git a/tests/testlib/nymeatestbase.h b/tests/testlib/nymeatestbase.h index dc84f738..3c2bc1a5 100644 --- a/tests/testlib/nymeatestbase.h +++ b/tests/testlib/nymeatestbase.h @@ -50,7 +50,7 @@ public: explicit NymeaTestBase(QObject *parent = nullptr); protected slots: - void initTestCase(); + void initTestCase(const QString &loggingRules = QString()); void cleanupTestCase(); void cleanup();