FIX(client) Address crash with pipewire and rtkit (Issue #6780)#7098
FIX(client) Address crash with pipewire and rtkit (Issue #6780)#7098Granahir2 wants to merge 1 commit intomumble-voip:masterfrom
Conversation
In AudioInput, the preprocessor could be reset in the audio thread. With recent versions of PipeWire enabling rtkit, this could make the real-time audio thread time-out and crash the client. This fix moves preprocessor resets out of the audio input thread. Fixes mumble-voip#6780
WalkthroughChanges were made to src/mumble/AudioInput.cpp to relocate the audio processor reset timing. The Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mumble/AudioInput.cpp`:
- Around line 550-552: Call to resetAudioProcessor() must be performed while
holding the qmSpeex mutex to match encodeAudioFrame()'s locking and avoid races
on sesEcho and m_preprocessor; surround the resetAudioProcessor() invocation
(and the bResetProcessor write if not already protected) with the same qmSpeex
lock used in encodeAudioFrame(), ensuring sesEcho and m_preprocessor are
modified only under that lock and release it after the reset completes.
| bResetProcessor = true; | ||
| resetAudioProcessor(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AudioInput::initializeMixer definition and call sites =="
rg -nP --type=cpp -C4 '\bAudioInput::initializeMixer\s*\(|\binitializeMixer\s*\('
echo
echo "== AudioInput::resetAudioProcessor definition and call sites =="
rg -nP --type=cpp -C4 '\bAudioInput::resetAudioProcessor\s*\(|\bresetAudioProcessor\s*\('
echo
echo "== Sites using qmSpeex lock =="
rg -nP --type=cpp -C4 '\bQMutexLocker\s+\w+\s*\(&qmSpeex\)'Repository: mumble-voip/mumble
Length of output: 12414
🏁 Script executed:
# View resetAudioProcessor() full implementation
sed -n '750,850p' src/mumble/AudioInput.cpp
echo "---"
# View encodeAudioFrame() to see qmSpeex lock scope
rg -n 'void AudioInput::encodeAudioFrame' -A 200 src/mumble/AudioInput.cpp | head -250Repository: mumble-voip/mumble
Length of output: 10738
🏁 Script executed:
# Check where bRunning is set and how it relates to initializeMixer/encodeAudioFrame
rg -n 'bRunning\s*=' src/mumble/AudioInput.cpp | head -30
echo "---"
# Check if there's any synchronization around initializeMixer calls
rg -n 'while.*bRunning' src/mumble/AudioInput.cpp
echo "---"
# Look for any thread management or synchronization in backend files
rg -n 'bRunning' src/mumble/PAAudio.cpp -B2 -A2 | head -40Repository: mumble-voip/mumble
Length of output: 114
🏁 Script executed:
# Get context around those bRunning assignments
sed -n '285,305p' src/mumble/AudioInput.cpp
echo "---"
# Look at the constructor to understand initialization order
sed -n '240,310p' src/mumble/AudioInput.cppRepository: mumble-voip/mumble
Length of output: 2393
🏁 Script executed:
# Check if backends stop audio before calling initializeMixer
# Look at PAAudio as a representative example
rg -n 'initializeMixer|bRunning' src/mumble/PAAudio.cpp -B2 -A2 | head -60
echo "---"
# Check threading model - is initializeMixer called from backend thread?
rg -n 'class PAAudioInput|void run|initializeMixer' src/mumble/PAAudio.cpp -B1 -A3 | head -100Repository: mumble-voip/mumble
Length of output: 662
🏁 Script executed:
# Check if there are any settings changes or reconfigurations that trigger initializeMixer while running
rg -n 'initializeMixer|preferredFormat|preferredChannels' src/mumble/AudioInput.h -B2 -A2
echo "---"
# Search for any calls to initializeMixer outside of initial setup
rg -n 'initializeMixer' src/mumble/ --type=cpp | grep -v 'AudioInput.cpp\|AudioOutput.cpp\|ASIOInput\|OSS.cpp\|PAAudio\|WASAPI\|PulseAudio\|PipeWire\|JackAudio\|ALSAAudio'
echo "---"
# Check if there's any mechanism to restart/reinitialize audio dynamically
rg -n 'on.*Settings.*Changed|on.*Preferences|restartAudio' src/mumble/AudioInput.cpp -B1 -A3 | head -50Repository: mumble-voip/mumble
Length of output: 458
🏁 Script executed:
# Look for the run() method in AudioInput to see when encodeAudioFrame is called
rg -n 'void AudioInput::run' -A 100 src/mumble/AudioInput.cpp | head -150
echo "---"
# Check if any Qt signals/slots trigger initializeMixer or settings changes
rg -n 'Q_SLOTS|public slots|private slots' src/mumble/AudioInput.h -A 5 | head -100Repository: mumble-voip/mumble
Length of output: 146
🏁 Script executed:
# Search for run() method - it's inherited, so check the base class pattern
rg -n 'virtual void run|void.*run.*override' src/mumble/AudioInput.cpp -B2 -A5
echo "---"
# Search in all audio files for run() implementation
find src/mumble -name '*.cpp' -exec grep -l 'void.*::run()' {} \;
echo "---"
# Check the actual flow - look at PAAudioInput::run() specifically
rg -n 'void PAAudioInput::run|class PAAudioInput' -A 200 src/mumble/PAAudio.cpp | head -250Repository: mumble-voip/mumble
Length of output: 444
🏁 Script executed:
# Get PAAudioInput::run() implementation
rg -n 'void PAAudioInput::run' -A 150 src/mumble/PAAudio.cpp
echo "---"
# Also check the callback mechanism - how is encodeAudioFrame called?
rg -n 'encodeAudioFrame|StreamCallback' src/mumble/PAAudio.cpp -B3 -A3Repository: mumble-voip/mumble
Length of output: 387
🏁 Script executed:
# Find where encodeAudioFrame is called from
rg -n 'encodeAudioFrame|addMic' src/mumble/PAAudio.cpp -B2 -A2
echo "---"
# Check the main AudioInput::run() method that likely calls encodeAudioFrame
rg -n 'class AudioInput|AudioInput::run|void run' src/mumble/AudioInput.h
echo "---"
# Look for actual run() implementation in AudioInput.cpp
rg -n 'void.*run.*\{' src/mumble/AudioInput.cpp -A 100 | head -150Repository: mumble-voip/mumble
Length of output: 358
🏁 Script executed:
# Find addMic implementation and how it relates to encodeAudioFrame
rg -n 'void.*addMic|void.*addEcho' src/mumble/AudioInput.cpp -B2 -A 20 | head -100
echo "---"
# Check the threading - is AudioInput::run() called as a QThread?
rg -n 'class AudioInput.*QThread|QThread.*start' src/mumble/AudioInput.h -B2 -A2
echo "---"
# Look for where audio actually starts - when run() begins
sed -n '125,200p' src/mumble/AudioInput.hRepository: mumble-voip/mumble
Length of output: 5330
🏁 Script executed:
# Look at addMic continuation to see where encodeAudioFrame is called
sed -n '561,611p' src/mumble/AudioInput.cpp
echo "---"
# Check if there's any slot that could reinitialize mixer during runtime
rg -n 'onUserMuteDeafStateChanged|Q_SLOTS' src/mumble/AudioInput.cpp -A 10Repository: mumble-voip/mumble
Length of output: 2222
Wrap resetAudioProcessor() call in qmSpeex lock to prevent data race with encodeAudioFrame().
resetAudioProcessor() mutates sesEcho and m_preprocessor without synchronization, while encodeAudioFrame() accesses these same objects under the qmSpeex lock. The asymmetric locking creates a potential UAF/data race if both execution paths overlap.
🔧 Proposed fix
void AudioInput::initializeMixer() {
bResetProcessor = true;
- resetAudioProcessor();
+ {
+ QMutexLocker l(&qmSpeex);
+ resetAudioProcessor();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bResetProcessor = true; | |
| resetAudioProcessor(); | |
| bResetProcessor = true; | |
| { | |
| QMutexLocker l(&qmSpeex); | |
| resetAudioProcessor(); | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/AudioInput.cpp` around lines 550 - 552, Call to
resetAudioProcessor() must be performed while holding the qmSpeex mutex to match
encodeAudioFrame()'s locking and avoid races on sesEcho and m_preprocessor;
surround the resetAudioProcessor() invocation (and the bResetProcessor write if
not already protected) with the same qmSpeex lock used in encodeAudioFrame(),
ensuring sesEcho and m_preprocessor are modified only under that lock and
release it after the reset completes.
|
I honestly didn't expect this to be the cause, because we have always been requesting real-time threading in our code: mumble/src/mumble/PipeWire.cpp Lines 282 to 299 in 1f6efaa I know from experience race conditions could be masked when omitting |
Hartmnt
added
client
bug
| connect(this, SIGNAL(doMute()), Global::get().mw->qaAudioMute, SLOT(trigger()), Qt::QueuedConnection); | ||
| connect(this, SIGNAL(doMuteCue()), Global::get().mw, SLOT(on_muteCuePopup_triggered())); | ||
|
|
||
| resetAudioProcessor(); |
There was a problem hiding this comment.
While this might fix the rtkit crash, I don't think this fix is correct for the following reason:
The resetAudioProcessor(); is called over and over again and usually terminates early because bResetProcessor is usually false except for the first run and when the child class calls initializeAudioMixer which can happen multiple times in the lifetime of an AudioInput.
Moving resetAudioProcessor to the constructor makes setting bResetProcessor to true a no-op which will very likely regress most if not all audio backends.
This needs more consideration and possibly a refactor.
There was a problem hiding this comment.
You're absolutely right. I wanted to mention the function's purpose and why moving it was potentially going to be a problem, but then I decided to focus on why it wouldn't be the correct solution anyway.
A refactor is definitely needed, albeit we should do it after switching to libxound.
|
I just remembered @eyJhb had mentioned the potential cause: apparently PipeWire with rtkit enabled enforces thread timing constraints and when resetting the processor we fall out of that. The actual problem is not What we should do, instead, is have a separate thread that handles all (pre)processor stuff and takes care of feeding a proper (ring)buffer. |
3 participants
In AudioInput, the preprocessor could be reset in the audio thread. With recent versions of PipeWire enabling rtkit, this could make the real-time audio thread time-out and crash the client.
This fix moves preprocessor resets out of the audio input thread. I have tested it on my machine (pipewire-1.4.7).