From d3603b8f9e781b59e966d5f2e6c2b948b8a26bc3 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Fri, 11 Feb 2022 18:52:38 -0500 Subject: [PATCH 1/2] Store channelhandles in messages by value instead of pointers. This fixes issues with pointers to deleted channelhandles being accessed, causing crashes, mostly with QT6. Converting to values means we can no longer trivially initialize the union/struct by memsetting all of the subtypes, because two of the struts are no longer trivial. Instead, we memset what we think is the largest datatype. SetEffectChainParameters includes a bool, an enum, and a double, so it's the largest one. --- src/effects/backends/effectprocessor.h | 16 ++++++++-------- src/effects/effectchain.cpp | 4 ++-- src/effects/effectsmessenger.cpp | 4 ++-- src/engine/channelhandle.h | 2 +- src/engine/channels/enginechannel.h | 2 +- src/engine/effects/engineeffect.cpp | 6 +++--- src/engine/effects/engineeffect.h | 4 ++-- src/engine/effects/engineeffectchain.cpp | 20 ++++++++++---------- src/engine/effects/engineeffectchain.h | 6 +++--- src/engine/effects/message.h | 18 +++++------------- 10 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/effects/backends/effectprocessor.h b/src/effects/backends/effectprocessor.h index fbee277f2a17..837da2b91043 100644 --- a/src/effects/backends/effectprocessor.h +++ b/src/effects/backends/effectprocessor.h @@ -74,10 +74,10 @@ class EffectProcessor { virtual void loadEngineEffectParameters( const QMap& parameters) = 0; virtual EffectState* createState(const mixxx::EngineParameters& engineParameters) = 0; - virtual void deleteStatesForInputChannel(const ChannelHandle* inputChannel) = 0; + virtual void deleteStatesForInputChannel(ChannelHandle inputChannel) = 0; // Called from the audio thread - virtual bool loadStatesForInputChannel(const ChannelHandle* inputChannel, + virtual bool loadStatesForInputChannel(ChannelHandle inputChannel, const EffectStatesMap* pStatesMap) = 0; /// Called from the audio thread @@ -202,11 +202,11 @@ class EffectProcessorImpl : public EffectProcessor { return createSpecificState(engineParameters); }; - bool loadStatesForInputChannel(const ChannelHandle* inputChannel, + bool loadStatesForInputChannel(ChannelHandle inputChannel, const EffectStatesMap* pStatesMap) final { if (kEffectDebugOutput) { qDebug() << "EffectProcessorImpl::loadStatesForInputChannel" << this - << "input" << *inputChannel; + << "input" << inputChannel; } // NOTE: ChannelHandleMap is like a map in that it associates an @@ -220,7 +220,7 @@ class EffectProcessorImpl : public EffectProcessor { // pStatesMap to build a new ChannelHandleMap with // dynamic_cast'ed states. ChannelHandleMap& effectSpecificStatesMap = - m_channelStateMatrix[*inputChannel]; + m_channelStateMatrix[inputChannel]; // deleteStatesForInputChannel should have been called before a new // map of EffectStates was sent to this function, or this is the first @@ -256,10 +256,10 @@ class EffectProcessorImpl : public EffectProcessor { }; /// Called from main thread for garbage collection after an input channel is disabled - void deleteStatesForInputChannel(const ChannelHandle* inputChannel) final { + void deleteStatesForInputChannel(ChannelHandle inputChannel) final { if (kEffectDebugOutput) { qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel" - << this << *inputChannel; + << this << inputChannel; } // NOTE: ChannelHandleMap is like a map in that it associates an @@ -269,7 +269,7 @@ class EffectProcessorImpl : public EffectProcessor { // engine thread in loadStatesForInputChannel. ChannelHandleMap& stateMap = - m_channelStateMatrix[*inputChannel]; + m_channelStateMatrix[inputChannel]; for (EffectSpecificState* pState : stateMap) { VERIFY_OR_DEBUG_ASSERT(pState != nullptr) { continue; diff --git a/src/effects/effectchain.cpp b/src/effects/effectchain.cpp index 6c7fd8d13a5a..14e052d1ab54 100644 --- a/src/effects/effectchain.cpp +++ b/src/effects/effectchain.cpp @@ -394,7 +394,7 @@ void EffectChain::enableForInputChannel(const ChannelHandleAndGroup& handleGroup EffectsRequest* request = new EffectsRequest(); request->type = EffectsRequest::ENABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL; request->pTargetChain = m_pEngineEffectChain; - request->EnableInputChannelForChain.pChannelHandle = &handleGroup.handle(); + request->EnableInputChannelForChain.channelHandle = handleGroup.handle(); // Allocate EffectStates here in the main thread to avoid allocating // memory in the realtime audio callback thread. Pointers to the @@ -428,7 +428,7 @@ void EffectChain::disableForInputChannel(const ChannelHandleAndGroup& handleGrou EffectsRequest* request = new EffectsRequest(); request->type = EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL; request->pTargetChain = m_pEngineEffectChain; - request->DisableInputChannelForChain.pChannelHandle = &handleGroup.handle(); + request->DisableInputChannelForChain.channelHandle = handleGroup.handle(); m_pMessenger->writeRequest(request); } diff --git a/src/effects/effectsmessenger.cpp b/src/effects/effectsmessenger.cpp index 9a3cb00cec1c..2eec71ef6151 100644 --- a/src/effects/effectsmessenger.cpp +++ b/src/effects/effectsmessenger.cpp @@ -93,10 +93,10 @@ void EffectsMessenger::collectGarbage(const EffectsRequest* pRequest) { } else if (pRequest->type == EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL) { if (kEffectDebugOutput) { qDebug() << debugString() << "deleting states for input channel" - << pRequest->DisableInputChannelForChain.pChannelHandle + << pRequest->DisableInputChannelForChain.channelHandle << "for EngineEffectChain" << pRequest->pTargetChain; } pRequest->pTargetChain->deleteStatesForInputChannel( - pRequest->DisableInputChannelForChain.pChannelHandle); + pRequest->DisableInputChannelForChain.channelHandle); } } diff --git a/src/engine/channelhandle.h b/src/engine/channelhandle.h index 1fc1d4abcf18..e20a9e1583e2 100644 --- a/src/engine/channelhandle.h +++ b/src/engine/channelhandle.h @@ -95,7 +95,7 @@ class ChannelHandleAndGroup { return m_name; } - inline const ChannelHandle& handle() const { + inline ChannelHandle handle() const { return m_handle; } diff --git a/src/engine/channels/enginechannel.h b/src/engine/channels/enginechannel.h index 749e43ccdccf..6771447c1581 100644 --- a/src/engine/channels/enginechannel.h +++ b/src/engine/channels/enginechannel.h @@ -30,7 +30,7 @@ class EngineChannel : public EngineObject { virtual ChannelOrientation getOrientation() const; - inline const ChannelHandle& getHandle() const { + inline ChannelHandle getHandle() const { return m_group.handle(); } diff --git a/src/engine/effects/engineeffect.cpp b/src/engine/effects/engineeffect.cpp index b5b543c74e89..20e9b30e3709 100644 --- a/src/engine/effects/engineeffect.cpp +++ b/src/engine/effects/engineeffect.cpp @@ -53,16 +53,16 @@ EffectState* EngineEffect::createState(const mixxx::EngineParameters& enginePara return m_pProcessor->createState(engineParameters); } -void EngineEffect::loadStatesForInputChannel(const ChannelHandle* inputChannel, +void EngineEffect::loadStatesForInputChannel(ChannelHandle inputChannel, EffectStatesMap* pStatesMap) { if (kEffectDebugOutput) { qDebug() << "EngineEffect::loadStatesForInputChannel" << this - << "loading states for input" << *inputChannel; + << "loading states for input" << inputChannel; } m_pProcessor->loadStatesForInputChannel(inputChannel, pStatesMap); } -void EngineEffect::deleteStatesForInputChannel(const ChannelHandle* inputChannel) { +void EngineEffect::deleteStatesForInputChannel(ChannelHandle inputChannel) { m_pProcessor->deleteStatesForInputChannel(inputChannel); } diff --git a/src/engine/effects/engineeffect.h b/src/engine/effects/engineeffect.h index 854d1e6e325b..ea83a6fbc158 100644 --- a/src/engine/effects/engineeffect.h +++ b/src/engine/effects/engineeffect.h @@ -35,10 +35,10 @@ class EngineEffect final : public EffectsRequestHandler { EffectState* createState(const mixxx::EngineParameters& engineParameters); /// Called in audio thread to load EffectStates received from the main thread - void loadStatesForInputChannel(const ChannelHandle* inputChannel, + void loadStatesForInputChannel(ChannelHandle inputChannel, EffectStatesMap* pStatesMap); /// Called from the main thread for garbage collection after an input channel is disabled - void deleteStatesForInputChannel(const ChannelHandle* inputChannel); + void deleteStatesForInputChannel(ChannelHandle inputChannel); /// Called in audio thread bool processEffectsRequest( diff --git a/src/engine/effects/engineeffectchain.cpp b/src/engine/effects/engineeffectchain.cpp index 621299fd4b3b..c3101ecb5ab3 100644 --- a/src/engine/effects/engineeffectchain.cpp +++ b/src/engine/effects/engineeffectchain.cpp @@ -123,10 +123,10 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message, qDebug() << debugString() << this << "ENABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL" << message.pTargetChain - << *message.EnableInputChannelForChain.pChannelHandle; + << message.EnableInputChannelForChain.channelHandle; } response.success = enableForInputChannel( - message.EnableInputChannelForChain.pChannelHandle, + message.EnableInputChannelForChain.channelHandle, message.EnableInputChannelForChain.pEffectStatesMapArray); break; case EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL: @@ -134,10 +134,10 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message, qDebug() << debugString() << this << "DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL" << message.pTargetChain - << *message.DisableInputChannelForChain.pChannelHandle; + << message.DisableInputChannelForChain.channelHandle; } response.success = disableForInputChannel( - message.DisableInputChannelForChain.pChannelHandle); + message.DisableInputChannelForChain.channelHandle); break; default: return false; @@ -146,12 +146,12 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message, return true; } -bool EngineEffectChain::enableForInputChannel(const ChannelHandle* inputHandle, +bool EngineEffectChain::enableForInputChannel(ChannelHandle inputHandle, EffectStatesMapArray* statesForEffectsInChain) { if (kEffectDebugOutput) { qDebug() << "EngineEffectChain::enableForInputChannel" << this << inputHandle; } - auto& outputMap = m_chainStatusForChannelMatrix[*inputHandle]; + auto& outputMap = m_chainStatusForChannelMatrix[inputHandle]; for (auto&& outputChannelStatus : outputMap) { VERIFY_OR_DEBUG_ASSERT(outputChannelStatus.enableState != EffectEnableState::Enabled) { @@ -180,8 +180,8 @@ bool EngineEffectChain::enableForInputChannel(const ChannelHandle* inputHandle, return true; } -bool EngineEffectChain::disableForInputChannel(const ChannelHandle* inputHandle) { - auto& outputMap = m_chainStatusForChannelMatrix[*inputHandle]; +bool EngineEffectChain::disableForInputChannel(ChannelHandle inputHandle) { + auto& outputMap = m_chainStatusForChannelMatrix[inputHandle]; for (auto&& outputChannelStatus : outputMap) { if (outputChannelStatus.enableState != EffectEnableState::Disabled) { outputChannelStatus.enableState = EffectEnableState::Disabling; @@ -195,7 +195,7 @@ bool EngineEffectChain::disableForInputChannel(const ChannelHandle* inputHandle) } // Called from the main thread for garbage collection after an input channel is disabled -void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle* inputChannel) { +void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle inputChannel) { // If an output channel is not presently being processed, for example when // PFL is not active, then process() cannot be relied upon to set this // chain's EffectEnableState from Disabling to Disabled. This must be done @@ -209,7 +209,7 @@ void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle* inputCh // QMap. So it is okay that m_chainStatusForChannelMatrix may be // accessed concurrently in the audio engine thread in process(), // enableForInputChannel(), or disableForInputChannel(). - auto& outputMap = m_chainStatusForChannelMatrix[*inputChannel]; + auto& outputMap = m_chainStatusForChannelMatrix[inputChannel]; for (auto&& outputChannelStatus : outputMap) { outputChannelStatus.enableState = EffectEnableState::Disabled; } diff --git a/src/engine/effects/engineeffectchain.h b/src/engine/effects/engineeffectchain.h index e9cef53baadc..9bb5aa6ac355 100644 --- a/src/engine/effects/engineeffectchain.h +++ b/src/engine/effects/engineeffectchain.h @@ -45,7 +45,7 @@ class EngineEffectChain final : public EffectsRequestHandler { const GroupFeatureState& groupFeatures); /// called from main thread - void deleteStatesForInputChannel(const ChannelHandle* channel); + void deleteStatesForInputChannel(const ChannelHandle channel); private: struct ChannelStatus { @@ -64,9 +64,9 @@ class EngineEffectChain final : public EffectsRequestHandler { bool updateParameters(const EffectsRequest& message); bool addEffect(EngineEffect* pEffect, int iIndex); bool removeEffect(EngineEffect* pEffect, int iIndex); - bool enableForInputChannel(const ChannelHandle* inputHandle, + bool enableForInputChannel(ChannelHandle inputHandle, EffectStatesMapArray* statesForEffectsInChain); - bool disableForInputChannel(const ChannelHandle* inputHandle); + bool disableForInputChannel(ChannelHandle inputHandle); // Gets or creates a ChannelStatus entry in m_channelStatus for the provided // handle. diff --git a/src/engine/effects/message.h b/src/engine/effects/message.h index c6504ab173f4..f5dce7ea916f 100644 --- a/src/engine/effects/message.h +++ b/src/engine/effects/message.h @@ -40,17 +40,9 @@ struct EffectsRequest { value(0.0) { pTargetChain = nullptr; pTargetEffect = nullptr; -#define CLEAR_STRUCT(x) memset(&x, 0, sizeof(x)); - CLEAR_STRUCT(AddEffectChain); - CLEAR_STRUCT(RemoveEffectChain); - CLEAR_STRUCT(EnableInputChannelForChain); - CLEAR_STRUCT(DisableInputChannelForChain); - CLEAR_STRUCT(AddEffectToChain); - CLEAR_STRUCT(RemoveEffectFromChain); - CLEAR_STRUCT(SetEffectChainParameters); - CLEAR_STRUCT(SetEffectParameters); - CLEAR_STRUCT(SetParameterParameters); -#undef CLEAR_STRUCT + // zero out the struct with the largest size to ensure the entire union memory is set to + // zero. + memset(&SetEffectChainParameters, 0, sizeof(SetEffectChainParameters)); } // This is called from the main thread by EffectsManager after receiving a @@ -97,10 +89,10 @@ struct EffectsRequest { } RemoveEffectChain; struct { EffectStatesMapArray* pEffectStatesMapArray; - const ChannelHandle* pChannelHandle; + ChannelHandle channelHandle; } EnableInputChannelForChain; struct { - const ChannelHandle* pChannelHandle; + ChannelHandle channelHandle; } DisableInputChannelForChain; struct { EngineEffect* pEffect; From 8bad0e31bfe64e551644302d19e0286bebeb32f8 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Mon, 14 Feb 2022 12:12:02 -0500 Subject: [PATCH 2/2] EffectsRequest: Remove union initialization. Add comment to make clear callers are responsible for initializing the values correctly. Double-checked call points to make sure all creations were correct. Removed unused struct parameter that I found during this process. --- src/engine/effects/message.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/engine/effects/message.h b/src/engine/effects/message.h index f5dce7ea916f..880ea48f623b 100644 --- a/src/engine/effects/message.h +++ b/src/engine/effects/message.h @@ -34,15 +34,14 @@ struct EffectsRequest { NUM_REQUEST_TYPES }; + // Creates a new uninitialized EffectsRequest. Callers are responsible for making sure that + // they initialize all the values of the struct corresponding to the type they select. EffectsRequest() : type(NUM_REQUEST_TYPES), request_id(-1), value(0.0) { pTargetChain = nullptr; pTargetEffect = nullptr; - // zero out the struct with the largest size to ensure the entire union memory is set to - // zero. - memset(&SetEffectChainParameters, 0, sizeof(SetEffectChainParameters)); } // This is called from the main thread by EffectsManager after receiving a @@ -84,7 +83,6 @@ struct EffectsRequest { } AddEffectChain; struct { EngineEffectChain* pChain; - int iIndex; SignalProcessingStage signalProcessingStage; } RemoveEffectChain; struct {