From 36f35817d281aa117e5aa2c0c2eaf953251e9cfe Mon Sep 17 00:00:00 2001
From: Tristan Matthews <tristan.matthews@savoirfairelinux.com>
Date: Wed, 17 Oct 2012 15:57:02 -0400
Subject: [PATCH] * #16719: manager: lock mutex properly before getting
 telephone tone

TODO: alsalayer and pulselayer are just grabbing the telephone pointer,
there's no guarantee that the tone will not have been destroyed.
---
 daemon/src/audio/alsa/alsalayer.cpp        |  2 ++
 daemon/src/audio/pulseaudio/pulselayer.cpp |  2 ++
 daemon/src/audio/sound/tonelist.cpp        |  2 +-
 daemon/src/managerimpl.cpp                 | 26 +++++++++++++---------
 4 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/daemon/src/audio/alsa/alsalayer.cpp b/daemon/src/audio/alsa/alsalayer.cpp
index c66f3ce750..e36ac54c08 100644
--- a/daemon/src/audio/alsa/alsalayer.cpp
+++ b/daemon/src/audio/alsa/alsalayer.cpp
@@ -703,6 +703,8 @@ void AlsaLayer::playback(int maxSamples)
     const size_t bytesToPut = maxSamples * sizeof(SFLDataFormat);
     // no audio available, play tone or silence
     if (bytesToGet <= 0) {
+        // FIXME: not thread safe! we only lock the mutex when we get the
+        // pointer, we have no guarantee that it will stay safe to use
         AudioLoop *tone = Manager::instance().getTelephoneTone();
         AudioLoop *file_tone = Manager::instance().getTelephoneFile();
 
diff --git a/daemon/src/audio/pulseaudio/pulselayer.cpp b/daemon/src/audio/pulseaudio/pulselayer.cpp
index e039d7493c..e7414b0217 100644
--- a/daemon/src/audio/pulseaudio/pulselayer.cpp
+++ b/daemon/src/audio/pulseaudio/pulselayer.cpp
@@ -360,6 +360,8 @@ void PulseLayer::writeToSpeaker()
         return;
     }
 
+    // FIXME: not thread safe! we only lock the mutex when we get the
+    // pointer, we have no guarantee that it will stay safe to use
     AudioLoop *toneToPlay = Manager::instance().getTelephoneTone();
 
     if (toneToPlay) {
diff --git a/daemon/src/audio/sound/tonelist.cpp b/daemon/src/audio/sound/tonelist.cpp
index 95ac499c62..c5e7531353 100644
--- a/daemon/src/audio/sound/tonelist.cpp
+++ b/daemon/src/audio/sound/tonelist.cpp
@@ -127,7 +127,7 @@ TelephoneTone::setCurrentTone(Tone::TONEID toneId)
 Tone*
 TelephoneTone::getCurrentTone()
 {
-    if (currentTone_ == Tone::TONE_NULL)
+    if (currentTone_ < Tone::TONE_DIALTONE or currentTone_ >= Tone::TONE_NULL)
         return NULL;
 
     return tone_[currentTone_];
diff --git a/daemon/src/managerimpl.cpp b/daemon/src/managerimpl.cpp
index 94a450581e..7d98db590a 100644
--- a/daemon/src/managerimpl.cpp
+++ b/daemon/src/managerimpl.cpp
@@ -125,7 +125,10 @@ void ManagerImpl::init(const std::string &config_file)
     {
         ost::MutexLock lock(audioLayerMutex_);
         if (audiodriver_) {
-            telephoneTone_.reset(new TelephoneTone(preferences.getZoneToneChoice(), audiodriver_->getSampleRate()));
+            {
+                ost::MutexLock toneLock(toneMutex_);
+                telephoneTone_.reset(new TelephoneTone(preferences.getZoneToneChoice(), audiodriver_->getSampleRate()));
+            }
             dtmfKey_.reset(new DTMF(getMainBuffer().getInternalSamplingRate()));
         }
     }
@@ -1673,9 +1676,10 @@ void ManagerImpl::playATone(Tone::TONEID toneId)
         audiodriver_->startStream();
     }
 
-    if (telephoneTone_.get() != 0) {
+    {
         ost::MutexLock lock(toneMutex_);
-        telephoneTone_->setCurrentTone(toneId);
+        if (telephoneTone_.get() != 0)
+            telephoneTone_->setCurrentTone(toneId);
     }
 }
 
@@ -1688,7 +1692,6 @@ void ManagerImpl::stopTone()
         return;
 
     ost::MutexLock lock(toneMutex_);
-
     if (telephoneTone_.get() != NULL)
         telephoneTone_->setCurrentTone(Tone::TONE_NULL);
 
@@ -1801,10 +1804,10 @@ void ManagerImpl::ringtone(const std::string& accountID)
 
 AudioLoop* ManagerImpl::getTelephoneTone()
 {
-    if (telephoneTone_.get()) {
-        ost::MutexLock m(toneMutex_);
+    ost::MutexLock m(toneMutex_);
+    if (telephoneTone_.get())
         return telephoneTone_->getCurrentTone();
-    } else
+    else
         return NULL;
 }
 
@@ -2084,9 +2087,9 @@ bool ManagerImpl::startRecordedFilePlayback(const std::string& filepath)
 
 void ManagerImpl::recordingPlaybackSeek(const double value)
 {
-    if(audiofile_.get()) {
+    ost::MutexLock m(toneMutex_);
+    if (audiofile_.get())
         audiofile_.get()->seek(value);
-    }
 }
 
 
@@ -2262,7 +2265,10 @@ void ManagerImpl::audioSamplingRateChanged(int samplerate)
 
     unsigned int sampleRate = audiodriver_->getSampleRate();
 
-    telephoneTone_.reset(new TelephoneTone(preferences.getZoneToneChoice(), sampleRate));
+    {
+        ost::MutexLock toneLock(toneMutex_);
+        telephoneTone_.reset(new TelephoneTone(preferences.getZoneToneChoice(), sampleRate));
+    }
     dtmfKey_.reset(new DTMF(sampleRate));
 
     if (wasActive)
-- 
GitLab