Skip to content

Commit

Permalink
Store channelhandles in messages by value instead of pointers. This f…
Browse files Browse the repository at this point in the history 
…ixes 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.
  • Loading branch information
@ywwg
ywwg committed Feb 13, 2022
1 parent b519aba commit d3603b8
Show file tree
Hide file tree
Showing 10 changed files with 37 additions and 45 deletions.
16 changes: 8 additions & 8 deletions src/effects/backends/effectprocessor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ class EffectProcessor {
virtual void loadEngineEffectParameters(
const QMap<QString, EngineEffectParameterPointer>& 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
Expand Down Expand Up @@ -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
Expand All @@ -220,7 +220,7 @@ class EffectProcessorImpl : public EffectProcessor {
// pStatesMap to build a new ChannelHandleMap with
// dynamic_cast'ed states.
ChannelHandleMap<EffectSpecificState*>& 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
Expand Down Expand Up @@ -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
Expand All @@ -269,7 +269,7 @@ class EffectProcessorImpl : public EffectProcessor {
// engine thread in loadStatesForInputChannel.

ChannelHandleMap<EffectSpecificState*>& stateMap =
m_channelStateMatrix[*inputChannel];
m_channelStateMatrix[inputChannel];
for (EffectSpecificState* pState : stateMap) {
VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
continue;
Expand Down
4 changes: 2 additions & 2 deletions src/effects/effectchain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions src/effects/effectsmessenger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
2 changes: 1 addition & 1 deletion src/engine/channelhandle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class ChannelHandleAndGroup {
return m_name;
}

inline const ChannelHandle& handle() const {
inline ChannelHandle handle() const {
return m_handle;
}

Expand Down
2 changes: 1 addition & 1 deletion src/engine/channels/enginechannel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
6 changes: 3 additions & 3 deletions src/engine/effects/engineeffect.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions src/engine/effects/engineeffect.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
20 changes: 10 additions & 10 deletions src/engine/effects/engineeffectchain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,21 @@ 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:
if (kEffectDebugOutput) {
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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions src/engine/effects/engineeffectchain.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down
18 changes: 5 additions & 13 deletions src/engine/effects/message.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit d3603b8

Please sign in to comment.