AudioSourceProviderAVFObjC::m_tap member access is not thread safe.
authorpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Jun 2017 10:57:23 +0000 (10:57 +0000)
committerpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Jun 2017 10:57:23 +0000 (10:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172263

Reviewed by Darin Adler.

It it possible that the main thread will modify the m_tap member by calling
AudioSourceProviderAVFObjC::destroyMix(), while m_tap is being read on a secondary
thread under AudioSourceProviderAVFObjC::processCallback(). Instead of accessing
the m_tap member on the secondary thread, we can use the same MTAudioProcessingTapRef
provided by MediaToolbox in the callback. We assume the tap ref is retained by
MediaToolbox, so it should be safe to use even if the the m_tap member is set to null.

Covered by existing tests.

* platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h:
* platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:
(WebCore::AudioSourceProviderAVFObjC::processCallback):
(WebCore::AudioSourceProviderAVFObjC::process):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@217878 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm

index 22e23d6..6ab72ce 100644 (file)
@@ -1,3 +1,24 @@
+2017-06-07  Per Arne Vollan  <pvollan@apple.com>
+
+        AudioSourceProviderAVFObjC::m_tap member access is not thread safe.
+        https://bugs.webkit.org/show_bug.cgi?id=172263
+
+        Reviewed by Darin Adler.
+
+        It it possible that the main thread will modify the m_tap member by calling
+        AudioSourceProviderAVFObjC::destroyMix(), while m_tap is being read on a secondary
+        thread under AudioSourceProviderAVFObjC::processCallback(). Instead of accessing
+        the m_tap member on the secondary thread, we can use the same MTAudioProcessingTapRef
+        provided by MediaToolbox in the callback. We assume the tap ref is retained by
+        MediaToolbox, so it should be safe to use even if the the m_tap member is set to null.
+
+        Covered by existing tests.
+
+        * platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h:
+        * platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:
+        (WebCore::AudioSourceProviderAVFObjC::processCallback):
+        (WebCore::AudioSourceProviderAVFObjC::process):
+
 2017-06-07  Zan Dobersek  <zdobersek@igalia.com>
 
         [GCrypt] RSA-PSS support
index 605eac6..56ce2fe 100644 (file)
@@ -77,7 +77,7 @@ private:
     void finalize();
     void prepare(CMItemCount maxFrames, const AudioStreamBasicDescription *processingFormat);
     void unprepare();
-    void process(CMItemCount numberFrames, MTAudioProcessingTapFlags flagsIn, AudioBufferList *bufferListInOut, CMItemCount *numberFramesOut, MTAudioProcessingTapFlags *flagsOut);
+    void process(MTAudioProcessingTapRef, CMItemCount numberFrames, MTAudioProcessingTapFlags flagsIn, AudioBufferList *bufferListInOut, CMItemCount *numberFramesOut, MTAudioProcessingTapFlags *flagsOut);
 
     RetainPtr<AVPlayerItem> m_avPlayerItem;
     RetainPtr<AVAssetTrack> m_avAssetTrack;
index 4c8c97e..4e7c7cf 100644 (file)
@@ -287,7 +287,7 @@ void AudioSourceProviderAVFObjC::processCallback(MTAudioProcessingTapRef tap, CM
     std::lock_guard<Lock> lock(tapStorage->mutex);
 
     if (tapStorage->_this)
-        tapStorage->_this->process(numberFrames, flags, bufferListInOut, numberFramesOut, flagsOut);
+        tapStorage->_this->process(tap, numberFrames, flags, bufferListInOut, numberFramesOut, flagsOut);
 }
 
 void AudioSourceProviderAVFObjC::init(void* clientInfo, void** tapStorageOut)
@@ -357,17 +357,13 @@ void AudioSourceProviderAVFObjC::unprepare()
     m_list = nullptr;
 }
 
-void AudioSourceProviderAVFObjC::process(CMItemCount numberOfFrames, MTAudioProcessingTapFlags flags, AudioBufferList* bufferListInOut, CMItemCount* numberFramesOut, MTAudioProcessingTapFlags* flagsOut)
+void AudioSourceProviderAVFObjC::process(MTAudioProcessingTapRef tap, CMItemCount numberOfFrames, MTAudioProcessingTapFlags flags, AudioBufferList* bufferListInOut, CMItemCount* numberFramesOut, MTAudioProcessingTapFlags* flagsOut)
 {
     UNUSED_PARAM(flags);
     
-    RetainPtr<MTAudioProcessingTapRef> tap = m_tap;
-    if (!tap)
-        return;
-
     CMItemCount itemCount = 0;
     CMTimeRange rangeOut;
-    OSStatus status = MTAudioProcessingTapGetSourceAudio(tap.get(), numberOfFrames, bufferListInOut, flagsOut, &rangeOut, &itemCount);
+    OSStatus status = MTAudioProcessingTapGetSourceAudio(tap, numberOfFrames, bufferListInOut, flagsOut, &rangeOut, &itemCount);
     if (status != noErr || !itemCount)
         return;