From c64b271f077939abcd33353efb50f1b0b2ca7588 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 22 Jan 2024 04:06:27 -0800 Subject: [PATCH] Check int and float parameters from external apps for validity Some player method calls sent from MediaControllers accept int or float values with a very clear API contract that disallows some values. Filtering by these values early avoids calling a Player implementation with invalid values. PiperOrigin-RevId: 600413993 --- .../session/MediaSessionLegacyStub.java | 8 +- .../media3/session/MediaSessionStub.java | 37 ++++---- .../media3/session/MediaSessionStubTest.java | 69 ++++++++++++++- ...CallbackWithMediaControllerCompatTest.java | 85 +++++++++++++++++++ .../androidx/media3/session/MockPlayer.java | 3 + 5 files changed, 185 insertions(+), 17 deletions(-) diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java index 61e9c9adc0..1717fad622 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java @@ -462,6 +462,9 @@ import org.checkerframework.checker.initialization.qual.Initialized; @Override public void onSetPlaybackSpeed(float speed) { + if (!(speed > 0f)) { + return; + } dispatchSessionTaskWithPlayerCommand( COMMAND_SET_SPEED_AND_PITCH, controller -> sessionImpl.getPlayerWrapper().setPlaybackSpeed(speed), @@ -470,6 +473,9 @@ import org.checkerframework.checker.initialization.qual.Initialized; @Override public void onSkipToQueueItem(long queueId) { + if (queueId < 0) { + return; + } dispatchSessionTaskWithPlayerCommand( COMMAND_SEEK_TO_MEDIA_ITEM, controller -> { @@ -851,7 +857,7 @@ import org.checkerframework.checker.initialization.qual.Initialized; } private void handleOnAddQueueItem(@Nullable MediaDescriptionCompat description, int index) { - if (description == null) { + if (description == null || (index != C.INDEX_UNSET && index < 0)) { return; } dispatchSessionTaskWithPlayerCommand( diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java index cd2cd3a902..1e09988851 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java @@ -771,7 +771,7 @@ import java.util.concurrent.ExecutionException; @Override public void seekToDefaultPositionWithMediaItemIndex( @Nullable IMediaController caller, int sequenceNumber, int mediaItemIndex) { - if (caller == null) { + if (caller == null || mediaItemIndex < 0) { return; } queueSessionTaskWithPlayerCommand( @@ -799,7 +799,7 @@ import java.util.concurrent.ExecutionException; @Override public void seekToWithMediaItemIndex( @Nullable IMediaController caller, int sequenceNumber, int mediaItemIndex, long positionMs) { - if (caller == null) { + if (caller == null || mediaItemIndex < 0) { return; } queueSessionTaskWithPlayerCommand( @@ -930,7 +930,7 @@ import java.util.concurrent.ExecutionException; @Override public void setPlaybackSpeed(@Nullable IMediaController caller, int sequenceNumber, float speed) { - if (caller == null) { + if (caller == null || !(speed > 0f)) { return; } queueSessionTaskWithPlayerCommand( @@ -1088,7 +1088,9 @@ import java.util.concurrent.ExecutionException; @Nullable IBinder mediaItemsRetriever, int startIndex, long startPositionMs) { - if (caller == null || mediaItemsRetriever == null) { + if (caller == null + || mediaItemsRetriever == null + || (startIndex != C.INDEX_UNSET && startIndex < 0)) { return; } List mediaItemList; @@ -1172,7 +1174,7 @@ import java.util.concurrent.ExecutionException; int sequenceNumber, int index, @Nullable Bundle mediaItemBundle) { - if (caller == null || mediaItemBundle == null) { + if (caller == null || mediaItemBundle == null || index < 0) { return; } MediaItem mediaItem; @@ -1229,7 +1231,7 @@ import java.util.concurrent.ExecutionException; int sequenceNumber, int index, @Nullable IBinder mediaItemsRetriever) { - if (caller == null || mediaItemsRetriever == null) { + if (caller == null || mediaItemsRetriever == null || index < 0) { return; } List mediaItems; @@ -1256,7 +1258,7 @@ import java.util.concurrent.ExecutionException; @Override public void removeMediaItem(@Nullable IMediaController caller, int sequenceNumber, int index) { - if (caller == null) { + if (caller == null || index < 0) { return; } queueSessionTaskWithPlayerCommand( @@ -1271,7 +1273,7 @@ import java.util.concurrent.ExecutionException; @Override public void removeMediaItems( @Nullable IMediaController caller, int sequenceNumber, int fromIndex, int toIndex) { - if (caller == null) { + if (caller == null || fromIndex < 0 || toIndex < fromIndex) { return; } queueSessionTaskWithPlayerCommand( @@ -1300,7 +1302,7 @@ import java.util.concurrent.ExecutionException; @Override public void moveMediaItem( @Nullable IMediaController caller, int sequenceNumber, int currentIndex, int newIndex) { - if (caller == null) { + if (caller == null || currentIndex < 0 || newIndex < 0) { return; } queueSessionTaskWithPlayerCommand( @@ -1317,7 +1319,7 @@ import java.util.concurrent.ExecutionException; int fromIndex, int toIndex, int newIndex) { - if (caller == null) { + if (caller == null || fromIndex < 0 || toIndex < fromIndex || newIndex < 0) { return; } queueSessionTaskWithPlayerCommand( @@ -1333,7 +1335,7 @@ import java.util.concurrent.ExecutionException; int sequenceNumber, int index, @Nullable Bundle mediaItemBundle) { - if (caller == null || mediaItemBundle == null) { + if (caller == null || mediaItemBundle == null || index < 0) { return; } MediaItem mediaItem; @@ -1371,7 +1373,7 @@ import java.util.concurrent.ExecutionException; int fromIndex, int toIndex, @Nullable IBinder mediaItemsRetriever) { - if (caller == null || mediaItemsRetriever == null) { + if (caller == null || mediaItemsRetriever == null || fromIndex < 0 || toIndex < fromIndex) { return; } ImmutableList mediaItems; @@ -1468,6 +1470,11 @@ import java.util.concurrent.ExecutionException; if (caller == null) { return; } + if (repeatMode != Player.REPEAT_MODE_ALL + && repeatMode != Player.REPEAT_MODE_OFF + && repeatMode != Player.REPEAT_MODE_ONE) { + return; + } queueSessionTaskWithPlayerCommand( caller, sequenceNumber, @@ -1503,7 +1510,7 @@ import java.util.concurrent.ExecutionException; @Override public void setVolume(@Nullable IMediaController caller, int sequenceNumber, float volume) { - if (caller == null) { + if (caller == null || !(volume >= 0f && volume <= 1f)) { return; } queueSessionTaskWithPlayerCommand( @@ -1516,7 +1523,7 @@ import java.util.concurrent.ExecutionException; @SuppressWarnings("deprecation") // Backwards compatibility a for flag-less method @Override public void setDeviceVolume(@Nullable IMediaController caller, int sequenceNumber, int volume) { - if (caller == null) { + if (caller == null || volume < 0) { return; } queueSessionTaskWithPlayerCommand( @@ -1529,7 +1536,7 @@ import java.util.concurrent.ExecutionException; @Override public void setDeviceVolumeWithFlags( @Nullable IMediaController caller, int sequenceNumber, int volume, int flags) { - if (caller == null) { + if (caller == null || volume < 0) { return; } queueSessionTaskWithPlayerCommand( diff --git a/libraries/session/src/test/java/androidx/media3/session/MediaSessionStubTest.java b/libraries/session/src/test/java/androidx/media3/session/MediaSessionStubTest.java index c15e6fb938..941384cf3e 100644 --- a/libraries/session/src/test/java/androidx/media3/session/MediaSessionStubTest.java +++ b/libraries/session/src/test/java/androidx/media3/session/MediaSessionStubTest.java @@ -411,6 +411,73 @@ public class MediaSessionStubTest { /* libraryParams= */ getThrowingBundle()); binder.subscribe( caller, /* seq= */ 0, /* parentId= */ "", /* libraryParams= */ getThrowingBundle()); + + // Pass invalid int or float values. + binder.setVolume(/* caller= */ null, /* seq= */ 0, /* volume= */ -0.00001f); + binder.setVolume(/* caller= */ null, /* seq= */ 0, /* volume= */ 1.00001f); + binder.setVolume(/* caller= */ null, /* seq= */ 0, /* volume= */ Float.NaN); + binder.setDeviceVolume(/* caller= */ null, /* seq= */ 0, /* volume= */ -1); + binder.setDeviceVolumeWithFlags( + /* caller= */ null, /* seq= */ 0, /* volume= */ -1, /* flags= */ 0); + binder.setMediaItemsWithStartIndex( + /* caller= */ null, + /* seq= */ 0, + /* mediaItems= */ new BundleListRetriever( + ImmutableList.of(new MediaItem.Builder().build().toBundle())), + /* startIndex= */ -1, + /* startPositionMs= */ 0); + binder.setRepeatMode(/* caller= */ null, /* seq= */ 0, /* repeatMode= */ -1); + binder.setRepeatMode(/* caller= */ null, /* seq= */ 0, /* repeatMode= */ 3); + binder.removeMediaItem(/* caller= */ null, /* seq= */ 0, /* index= */ -1); + binder.removeMediaItems( + /* caller= */ null, /* seq= */ 0, /* fromIndex= */ -1, /* toIndex= */ 0); + binder.removeMediaItems(/* caller= */ null, /* seq= */ 0, /* fromIndex= */ 1, /* toIndex= */ 0); + binder.moveMediaItem( + /* caller= */ null, /* seq= */ 0, /* currentIndex= */ -1, /* newIndex= */ 0); + binder.moveMediaItem( + /* caller= */ null, /* seq= */ 0, /* currentIndex= */ 0, /* newIndex= */ -1); + binder.moveMediaItems( + /* caller= */ null, /* seq= */ 0, /* fromIndex= */ -1, /* toIndex= */ 0, /* newIndex= */ 0); + binder.moveMediaItems( + /* caller= */ null, /* seq= */ 0, /* fromIndex= */ 1, /* toIndex= */ 0, /* newIndex= */ 0); + binder.moveMediaItems( + /* caller= */ null, /* seq= */ 0, /* fromIndex= */ 0, /* toIndex= */ 0, /* newIndex= */ -1); + binder.replaceMediaItem( + /* caller= */ null, + /* seq= */ 0, + /* index= */ -1, + /* mediaItemBundle= */ new MediaItem.Builder().build().toBundle()); + binder.replaceMediaItems( + /* caller= */ null, + /* seq= */ 0, + /* fromIndex= */ -1, + /* toIndex= */ 0, + /* mediaItems= */ new BundleListRetriever( + ImmutableList.of(new MediaItem.Builder().build().toBundle()))); + binder.replaceMediaItems( + /* caller= */ null, + /* seq= */ 0, + /* fromIndex= */ 1, + /* toIndex= */ 0, + /* mediaItems= */ new BundleListRetriever( + ImmutableList.of(new MediaItem.Builder().build().toBundle()))); + binder.setPlaybackSpeed(/* caller= */ null, /* seq= */ 0, /* speed= */ -0.0001f); + binder.setPlaybackSpeed(/* caller= */ null, /* seq= */ 0, /* speed= */ Float.NaN); + binder.addMediaItemWithIndex( + /* caller= */ null, + /* seq= */ 0, + /* index= */ -1, + /* mediaItemBundle= */ new MediaItem.Builder().build().toBundle()); + binder.addMediaItemsWithIndex( + /* caller= */ null, + /* seq= */ 0, + /* index= */ -1, + /* mediaItems= */ new BundleListRetriever( + ImmutableList.of(new MediaItem.Builder().build().toBundle()))); + binder.seekToDefaultPositionWithMediaItemIndex( + /* caller= */ null, /* seq= */ 0, /* mediaItemIndex= */ -1); + binder.seekToWithMediaItemIndex( + /* caller= */ null, /* seq= */ 0, /* mediaItemIndex= */ -1, /* positionMs= */ 0); } @Test @@ -430,7 +497,7 @@ public class MediaSessionStubTest { caller, /* seq= */ 0, /* connectionRequest= */ new ConnectionRequest( - /* packageName= */ "invalud", /* pid= */ 9999, /* connectionHints= */ new Bundle()) + /* packageName= */ "invalid", /* pid= */ 9999, /* connectionHints= */ new Bundle()) .toBundle()); } } diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionCallbackWithMediaControllerCompatTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionCallbackWithMediaControllerCompatTest.java index ef2d663dc0..eb5a3f76ab 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionCallbackWithMediaControllerCompatTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionCallbackWithMediaControllerCompatTest.java @@ -397,6 +397,25 @@ public class MediaSessionCallbackWithMediaControllerCompatTest { assertThat(player.playbackParameters.speed).isEqualTo(testSpeed); } + @Test + public void setPlaybackSpeed_withInvalidSpeed_doesNotCrashSession() throws Exception { + session = + new MediaSession.Builder(context, player) + .setId("setPlaybackSpeed") + .setCallback(new TestSessionCallback()) + .build(); + controller = + new RemoteMediaControllerCompat( + context, session.getSessionCompat().getSessionToken(), /* waitForConnection= */ true); + + controller.getTransportControls().setPlaybackSpeed(-0.0001f); + controller.getTransportControls().setPlaybackSpeed(Float.NaN); + controller.getTransportControls().setPlaybackSpeed(0.5f); // Add a valid action to wait for. + player.awaitMethodCalled(MockPlayer.METHOD_SET_PLAYBACK_SPEED, TIMEOUT_MS); + + assertThat(player.playbackParameters.speed).isEqualTo(0.5f); + } + @Test public void addQueueItem() throws Exception { AtomicReference> requestedMediaItems = new AtomicReference<>(); @@ -491,6 +510,48 @@ public class MediaSessionCallbackWithMediaControllerCompatTest { assertThat(player.mediaItems.get(1)).isEqualTo(resolvedMediaItem); } + @Test + public void addQueueItemWithIndex_withInvalidIndex_doesNotCrashSession() throws Exception { + MediaItem resolvedMediaItem = MediaItem.fromUri(TEST_URI); + MediaSession.Callback callback = + new MediaSession.Callback() { + @Override + public ListenableFuture> onAddMediaItems( + MediaSession mediaSession, ControllerInfo controller, List mediaItems) { + // Resolve MediaItem asynchronously to test correct threading logic. + return executorService.submit(() -> ImmutableList.of(resolvedMediaItem)); + } + }; + session = + new MediaSession.Builder(context, player) + .setId("addQueueItemWithIndex_invalidIndex") + .setCallback(callback) + .build(); + controller = + new RemoteMediaControllerCompat( + context, session.getSessionCompat().getSessionToken(), /* waitForConnection= */ true); + + handler.postAndSync( + () -> { + List mediaItems = MediaTestUtils.createMediaItems(/* size= */ 10); + player.setMediaItems(mediaItems); + player.timeline = MediaTestUtils.createTimeline(mediaItems); + player.notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + }); + // Prepare an item to add. + MediaDescriptionCompat desc = + new MediaDescriptionCompat.Builder() + .setMediaId("media_id") + .setMediaUri(Uri.parse("https://test.test")) + .build(); + + controller.addQueueItem(desc, /* index= */ -1); + controller.addQueueItem(desc, /* index= */ 1); // Add valid call to wait for. + player.awaitMethodCalled(MockPlayer.METHOD_ADD_MEDIA_ITEMS_WITH_INDEX, TIMEOUT_MS); + + assertThat(player.index).isEqualTo(1); + } + @Test public void removeQueueItem() throws Exception { session = @@ -617,6 +678,30 @@ public class MediaSessionCallbackWithMediaControllerCompatTest { assertThat(player.seekMediaItemIndex).isEqualTo(targetIndex); } + @Test + public void skipToQueueItem_withInvalidValue_doesNotCrashSession() throws Exception { + session = + new MediaSession.Builder(context, player) + .setId("skipToQueueItem_invalidValues") + .setCallback(new TestSessionCallback()) + .build(); + controller = + new RemoteMediaControllerCompat( + context, session.getSessionCompat().getSessionToken(), /* waitForConnection= */ true); + handler.postAndSync( + () -> { + player.timeline = MediaTestUtils.createTimeline(/* windowCount= */ 10); + player.notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + }); + + controller.getTransportControls().skipToQueueItem(-1); + controller.getTransportControls().skipToQueueItem(1); // Add valid call to wait for. + player.awaitMethodCalled( + MockPlayer.METHOD_SEEK_TO_DEFAULT_POSITION_WITH_MEDIA_ITEM_INDEX, TIMEOUT_MS); + + assertThat(player.seekMediaItemIndex).isEqualTo(1); + } + @Test public void dispatchMediaButtonEvent_playWithEmptyTimeline_callsPlaybackResumptionPrepareAndPlay() throws Exception { diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java index 010c9f2105..09a4e812ec 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java @@ -15,6 +15,7 @@ */ package androidx.media3.session; +import static androidx.media3.common.util.Assertions.checkArgument; import static androidx.media3.common.util.Assertions.checkNotNull; import static java.lang.annotation.ElementType.TYPE_USE; @@ -431,6 +432,7 @@ public class MockPlayer implements Player { @Override public void seekToDefaultPosition(int mediaItemIndex) { + checkArgument(mediaItemIndex >= 0); seekMediaItemIndex = mediaItemIndex; checkNotNull(conditionVariables.get(METHOD_SEEK_TO_DEFAULT_POSITION_WITH_MEDIA_ITEM_INDEX)) .open(); @@ -444,6 +446,7 @@ public class MockPlayer implements Player { @Override public void seekTo(int mediaItemIndex, long positionMs) { + checkArgument(mediaItemIndex >= 0); seekMediaItemIndex = mediaItemIndex; seekPositionMs = positionMs; checkNotNull(conditionVariables.get(METHOD_SEEK_TO_WITH_MEDIA_ITEM_INDEX)).open();