From 0f6bcd18b2ef109850c6430fa0d474688571da5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sami=20Kalliom=C3=A4ki?= Date: Mon, 3 Feb 2020 16:09:45 +0100 Subject: [PATCH] Hold a reference to AndroidVideoTrackSource while calling onFrameCaptured. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it safe to deliver frames to the sink from VideoProcessor even after setSink has been called with null reference without danger of use after free. Bug: b/148063550 Change-Id: Ib78f75ac49fc6117f744c55da1a4e671bbdcdf22 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168160 Reviewed-by: Paulina Hensman Commit-Queue: Sami Kalliomäki Cr-Commit-Position: refs/heads/master@{#30455} --- sdk/android/BUILD.gn | 8 +- sdk/android/api/org/webrtc/MediaSource.java | 18 +++- .../api/org/webrtc/VideoProcessor.java | 2 +- sdk/android/api/org/webrtc/VideoSource.java | 4 +- .../src/java/org/webrtc/RefCountDelegate.java | 15 ++++ .../src/org/webrtc/RefCountDelegateTest.java | 83 +++++++++++++++++++ 6 files changed, 125 insertions(+), 5 deletions(-) create mode 100644 sdk/android/tests/src/org/webrtc/RefCountDelegateTest.java diff --git a/sdk/android/BUILD.gn b/sdk/android/BUILD.gn index 2d511e2eac..6bcf36c082 100644 --- a/sdk/android/BUILD.gn +++ b/sdk/android/BUILD.gn @@ -281,8 +281,11 @@ if (is_android) { "api/org/webrtc/NativePeerConnectionFactory.java", "api/org/webrtc/NetEqFactoryFactory.java", "api/org/webrtc/NetworkControllerFactoryFactory.java", - "api/org/webrtc/NetworkMonitor.java", # TODO(sakal): Break dependencies and move to base_java. - "api/org/webrtc/NetworkMonitorAutoDetect.java", # TODO(sakal): Break dependencies and move to base_java. + "api/org/webrtc/NetworkMonitor.java", # TODO(sakal): Break dependencies + # and move to base_java. + "api/org/webrtc/NetworkMonitorAutoDetect.java", # TODO(sakal): Break + # dependencies and move + # to base_java. "api/org/webrtc/NetworkStatePredictorFactoryFactory.java", "api/org/webrtc/PeerConnection.java", "api/org/webrtc/PeerConnectionDependencies.java", @@ -1537,6 +1540,7 @@ if (is_android) { "tests/src/org/webrtc/GlGenericDrawerTest.java", "tests/src/org/webrtc/HardwareVideoEncoderTest.java", "tests/src/org/webrtc/IceCandidateTest.java", + "tests/src/org/webrtc/RefCountDelegateTest.java", "tests/src/org/webrtc/ScalingSettingsTest.java", ] diff --git a/sdk/android/api/org/webrtc/MediaSource.java b/sdk/android/api/org/webrtc/MediaSource.java index 0b19e1a775..9245e3e2eb 100644 --- a/sdk/android/api/org/webrtc/MediaSource.java +++ b/sdk/android/api/org/webrtc/MediaSource.java @@ -25,9 +25,11 @@ public class MediaSource { } } + private final RefCountDelegate refCountDelegate; private long nativeSource; public MediaSource(long nativeSource) { + refCountDelegate = new RefCountDelegate(() -> JniCommon.nativeReleaseRef(nativeSource)); this.nativeSource = nativeSource; } @@ -38,7 +40,7 @@ public class MediaSource { public void dispose() { checkMediaSourceExists(); - JniCommon.nativeReleaseRef(nativeSource); + refCountDelegate.release(); nativeSource = 0; } @@ -48,6 +50,20 @@ public class MediaSource { return nativeSource; } + /** + * Runs code in {@code runnable} holding a reference to the media source. If the object has + * already been released, does nothing. + */ + void runWithReference(Runnable runnable) { + if (refCountDelegate.safeRetain()) { + try { + runnable.run(); + } finally { + refCountDelegate.release(); + } + } + } + private void checkMediaSourceExists() { if (nativeSource == 0) { throw new IllegalStateException("MediaSource has been disposed."); diff --git a/sdk/android/api/org/webrtc/VideoProcessor.java b/sdk/android/api/org/webrtc/VideoProcessor.java index 3a89090e2d..19a2b382c9 100644 --- a/sdk/android/api/org/webrtc/VideoProcessor.java +++ b/sdk/android/api/org/webrtc/VideoProcessor.java @@ -54,7 +54,7 @@ public interface VideoProcessor extends CapturerObserver { /** * Set the sink that receives the output from this processor. Null can be passed in to unregister - * a sink. After this call returns, no frames should be delivered to an unregistered sink. + * a sink. */ void setSink(@Nullable VideoSink sink); diff --git a/sdk/android/api/org/webrtc/VideoSource.java b/sdk/android/api/org/webrtc/VideoSource.java index 6c528fd05b..b0bffd6ff1 100644 --- a/sdk/android/api/org/webrtc/VideoSource.java +++ b/sdk/android/api/org/webrtc/VideoSource.java @@ -135,7 +135,9 @@ public class VideoSource extends MediaSource { } videoProcessor = newVideoProcessor; if (newVideoProcessor != null) { - newVideoProcessor.setSink(nativeAndroidVideoTrackSource::onFrameCaptured); + newVideoProcessor.setSink( + (frame) + -> runWithReference(() -> nativeAndroidVideoTrackSource.onFrameCaptured(frame))); if (isCapturerRunning) { newVideoProcessor.onCapturerStarted(/* success= */ true); } diff --git a/sdk/android/src/java/org/webrtc/RefCountDelegate.java b/sdk/android/src/java/org/webrtc/RefCountDelegate.java index 58be7aa0fb..acbc0c3ed9 100644 --- a/sdk/android/src/java/org/webrtc/RefCountDelegate.java +++ b/sdk/android/src/java/org/webrtc/RefCountDelegate.java @@ -45,4 +45,19 @@ class RefCountDelegate implements RefCounted { releaseCallback.run(); } } + + /** + * Tries to retain the object. Can be used in scenarios where it is unknown if the object has + * already been released. Returns true if successful or false if the object was already released. + */ + boolean safeRetain() { + int currentRefCount = refCount.get(); + while (currentRefCount != 0) { + if (refCount.weakCompareAndSet(currentRefCount, currentRefCount + 1)) { + return true; + } + currentRefCount = refCount.get(); + } + return false; + } } diff --git a/sdk/android/tests/src/org/webrtc/RefCountDelegateTest.java b/sdk/android/tests/src/org/webrtc/RefCountDelegateTest.java new file mode 100644 index 0000000000..1f449c8bb7 --- /dev/null +++ b/sdk/android/tests/src/org/webrtc/RefCountDelegateTest.java @@ -0,0 +1,83 @@ +/* + * Copyright 2020 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +package org.webrtc; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import org.chromium.testing.local.LocalRobolectricTestRunner; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.robolectric.annotation.Config; + +@RunWith(LocalRobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class RefCountDelegateTest { + @Mock Runnable mockReleaseCallback; + private RefCountDelegate refCountDelegate; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + + refCountDelegate = new RefCountDelegate(mockReleaseCallback); + } + + @Test + public void testReleaseRunsReleaseCallback() { + refCountDelegate.release(); + verify(mockReleaseCallback).run(); + } + + @Test + public void testRetainIncreasesRefCount() { + refCountDelegate.retain(); + + refCountDelegate.release(); + verify(mockReleaseCallback, never()).run(); + + refCountDelegate.release(); + verify(mockReleaseCallback).run(); + } + + @Test(expected = IllegalStateException.class) + public void testReleaseAfterFreeThrowsIllegalStateException() { + refCountDelegate.release(); + refCountDelegate.release(); + } + + @Test(expected = IllegalStateException.class) + public void testRetainAfterFreeThrowsIllegalStateException() { + refCountDelegate.release(); + refCountDelegate.retain(); + } + + @Test + public void testSafeRetainBeforeFreeReturnsTrueAndIncreasesRefCount() { + assertThat(refCountDelegate.safeRetain()).isTrue(); + + refCountDelegate.release(); + verify(mockReleaseCallback, never()).run(); + + refCountDelegate.release(); + verify(mockReleaseCallback).run(); + } + + @Test + public void testSafeRetainAfterFreeReturnsFalse() { + refCountDelegate.release(); + assertThat(refCountDelegate.safeRetain()).isFalse(); + } +}