Fix crash due to concurrent access in MidiController

src/controllers/bulk/bulkcontroller.cpp

@@ -283,8 +283,8 @@ void BulkController::send(const QList<int>& data, unsigned int length) {
 
 void BulkController::sendBytes(const QByteArray& data) {
     VERIFY_OR_DEBUG_ASSERT(!m_pMapping ||
-            m_pMapping->getDeviceDirection() &
-                    LegacyControllerMapping::DeviceDirection::Outgoing) {
+            (m_pMapping->getDeviceDirection() &
+                    LegacyControllerMapping::DeviceDirection::Outgoing)) {
         qDebug() << "The mapping for the bulk device" << getName()
                  << "doesn't require sending data. Ignoring sending request.";
         return;

src/controllers/controllermanager.cpp

@@ -186,6 +186,12 @@ void ControllerManager::slotShutdown() {
 }
 
 void ControllerManager::updateControllerList() {
+    static bool already_called = false;
+    VERIFY_OR_DEBUG_ASSERT(!already_called) {
+        qWarning() << "skipping ControllerManager::updateControllerList() because is already called";
+        return;
+    }
+    already_called = true;
     // NOTE: Currently this function is only called on startup. If hotplug is added, changes to the
     // controller list must be synchronized with dlgprefcontrollers to avoid dangling connections
     // and possible crashes.

src/controllers/midi/midicontroller.cpp

@@ -2,6 +2,7 @@
 
 #include <QJSValue>
 #include <algorithm>
+#include <atomic>
 
 #include "control/controlobject.h"
 #include "controllers/defs_controllers.h"
@@ -37,7 +38,11 @@ MidiController::MidiController(const QString& deviceName)
 
 void MidiController::slotBeforeEngineShutdown() {
     Controller::slotBeforeEngineShutdown();
-    m_pMapping->removeInputHandlerMappings();
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping) {
+        return;
+    }
+    pMapping->removeInputHandlerMappings();
 }
 
 MidiController::~MidiController() {
@@ -55,14 +60,25 @@ QString MidiController::mappingExtension() {
 }
 
 void MidiController::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
-    m_pMapping = downcastAndTakeOwnership<LegacyMidiControllerMapping>(std::move(pMapping));
+    std::shared_ptr<LegacyMidiControllerMapping> pMidiMapping =
+            downcastAndTakeOwnership<LegacyMidiControllerMapping>(
+                    std::move(pMapping));
+#ifdef __cpp_lib_atomic_shared_ptr
+    // Used Spinlock
+    m_pMapping.store(pMidiMapping, std::memory_order_relaxed);
+#else
+    // Uses Mutex
+    std::atomic_store_explicit(&m_pMapping, pMidiMapping, std::memory_order_relaxed);
+#endif
 }
 
 std::shared_ptr<LegacyControllerMapping> MidiController::cloneMapping() {
-    if (!m_pMapping) {
+    // Function becomes temporary shared owner
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping) {
         return nullptr;
     }
-    return m_pMapping->clone();
+    return pMapping->clone();
 }
 
 int MidiController::close() {
@@ -92,16 +108,14 @@ bool MidiController::applyMapping() {
 }
 
 void MidiController::createOutputHandlers() {
-    if (!m_pMapping) {
-        return;
-    }
-
-    if (m_pMapping->getOutputMappings().isEmpty()) {
+    // Function becomes temporary shared owner
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping || pMapping->getOutputMappings().isEmpty()) {
         return;
     }
 
     QStringList failures;
-    for (const auto& mapping : m_pMapping->getOutputMappings()) {
+    for (const auto& mapping : pMapping->getOutputMappings()) {
         QString group = mapping.controlKey.group;
         QString key = mapping.controlKey.item;
 
@@ -223,7 +237,9 @@ void MidiController::clearTemporaryInputMappings() {
 }
 
 void MidiController::commitTemporaryInputMappings() {
-    if (!m_pMapping) {
+    // Function becomes temporary shared owner
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping) {
         return;
     }
 
@@ -232,15 +248,15 @@ void MidiController::commitTemporaryInputMappings() {
     // m_temporaryInputMappings from m_mapping's input mappings.
     for (auto it = m_temporaryInputMappings.constBegin();
          it != m_temporaryInputMappings.constEnd(); ++it) {
-        m_pMapping->removeInputMapping(it.key());
+        pMapping->removeInputMapping(it.key());
     }
 
     // Now, we can just use add all mappings from m_temporaryInputMappings
     // since we removed the duplicates in the original set.
     for (auto it = m_temporaryInputMappings.constBegin();
             it != m_temporaryInputMappings.constEnd();
             ++it) {
-        m_pMapping->addInputMapping(it.key(), it.value());
+        pMapping->addInputMapping(it.key(), it.value());
     }
     m_temporaryInputMappings.clear();
 }
@@ -286,8 +302,13 @@ void MidiController::receivedShortMessage(unsigned char status,
         }
     }
 
-    auto it = m_pMapping->getInputMappings().constFind(mappingKey.key);
-    for (; it != m_pMapping->getInputMappings().constEnd() && it.key() == mappingKey.key; ++it) {
+    // Function becomes temporary shared owner
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping) {
+        return;
+    }
+    auto it = pMapping->getInputMappings().constFind(mappingKey.key);
+    for (; it != pMapping->getInputMappings().constEnd() && it.key() == mappingKey.key; ++it) {
         processInputMapping(it.value(), status, control, value, timestamp);
     }
 }
@@ -584,8 +605,13 @@ void MidiController::receive(const QByteArray& data, mixxx::Duration timestamp)
         }
     }
 
+    // Function becomes temporary shared owner
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping) {
+        return;
+    }
     const auto [inputMappingsBegin, inputMappingsEnd] =
-            m_pMapping->getInputMappings().equal_range(mappingKey.key);
+            pMapping->getInputMappings().equal_range(mappingKey.key);
     std::for_each(inputMappingsBegin, inputMappingsEnd, [&](const auto& inputMapping) {
         processInputMapping(inputMapping, data, timestamp);
     });
@@ -612,6 +638,11 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
     VERIFY_OR_DEBUG_ASSERT(pJsEngine) {
         return QJSValue();
     }
+    // Function becomes temporary shared owner
+    std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+    if (!pMapping) {
+        return QJSValue();
+    }
 
     if (!scriptCode.isCallable()) {
         auto error = kMakeInputHandlerError;
@@ -635,8 +666,8 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
 
     const auto midiKey = MidiKey(status, midino);
 
-    auto it = m_pMapping->getInputMappings().constFind(midiKey.key);
-    if (it != m_pMapping->getInputMappings().constEnd() &&
+    auto it = pMapping->getInputMappings().constFind(midiKey.key);
+    if (it != pMapping->getInputMappings().constEnd() &&
             it.value().options.testFlag(MidiOption::Script) &&
             std::holds_alternative<ConfigKey>(it.value().control)) {
         qCWarning(m_logBase) << QStringLiteral(
@@ -652,6 +683,16 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
             MidiOption::Script,
             std::make_shared<QJSValue>(scriptCode));
 
-    m_pMapping->addInputMapping(inputMapping.key.key, inputMapping);
-    return pJsEngine->newQObject(new MidiInputHandleJSProxy(m_pMapping, inputMapping));
+    pMapping->addInputMapping(inputMapping.key.key, inputMapping);
+    return pJsEngine->newQObject(new MidiInputHandleJSProxy(pMapping, inputMapping));
+}
+
+std::shared_ptr<LegacyMidiControllerMapping> MidiController::getSharedMapping() const {
+#ifdef __cpp_lib_atomic_shared_ptr
+    // Used Spinlock
+    return m_pMapping.load(std::memory_order_relaxed);
+#else
+    // Uses Mutex
+    return std::atomic_load_explicit(&m_pMapping, std::memory_order_relaxed);
+#endif
 }

src/controllers/midi/midicontroller.h

@@ -43,10 +43,11 @@ class MidiController : public Controller {
     virtual std::shared_ptr<LegacyControllerMapping> cloneMapping() override;
 
     bool isMappable() const override {
-        if (!m_pMapping) {
+        std::shared_ptr<LegacyMidiControllerMapping> pMapping = getSharedMapping();
+        if (!pMapping) {
             return false;
         }
-        return m_pMapping->isMappable();
+        return pMapping->isMappable();
     }
 
     bool matchMapping(const MappingInfo& mapping) override;
@@ -103,10 +104,17 @@ class MidiController : public Controller {
     void createOutputHandlers();
     void updateAllOutputs();
     void destroyOutputHandlers();
+    std::shared_ptr<LegacyMidiControllerMapping> getSharedMapping() const;
 
     QHash<uint16_t, MidiInputMapping> m_temporaryInputMappings;
     QList<MidiOutputHandler*> m_outputs;
+
+#ifdef __cpp_lib_atomic_shared_ptr
+    std::atomic<std::shared_ptr<LegacyMidiControllerMapping>> m_pMapping;
+#else
     std::shared_ptr<LegacyMidiControllerMapping> m_pMapping;
+#endif
+
     SoftTakeoverCtrl m_st;
     QList<QPair<MidiInputMapping, unsigned char>> m_fourteen_bit_queued_mappings;