From d7cc04c1da168ce2050b01035dee8f51623520d0 Mon Sep 17 00:00:00 2001 From: Colin Liang Date: Thu, 14 Mar 2024 15:38:48 -0700 Subject: [PATCH] Fix crash caused by Use after free Introduce a session_state_mutex_ to protect the session_state_ variable during concurrent reading and writing of metadata. b/329330347 --- cobalt/media_session/media_session_client.cc | 116 +++++++++++------- cobalt/media_session/media_session_client.h | 2 + .../shared/android_media_session_client.cc | 6 +- 3 files changed, 74 insertions(+), 50 deletions(-) diff --git a/cobalt/media_session/media_session_client.cc b/cobalt/media_session/media_session_client.cc index 8c8bad0ea2cc..55aa6b08f27a 100644 --- a/cobalt/media_session/media_session_client.cc +++ b/cobalt/media_session/media_session_client.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include "base/logging.h" #include "cobalt/media_session/media_image.h" @@ -27,6 +28,7 @@ namespace cobalt { namespace media_session { +using starboard::ScopedLock; using MediaImageSequence = ::cobalt::script::Sequence; namespace { @@ -205,7 +207,14 @@ void MediaSessionClient::UpdatePlatformPlaybackState( } platform_playback_state_ = ConvertPlaybackState(state); - if (session_state_.actual_playback_state() != ComputeActualPlaybackState()) { + + MediaSessionPlaybackState actual_playback_state; + { + ScopedLock scoped_lock(session_state_mutex_); + actual_playback_state = session_state_.actual_playback_state(); + } + + if (actual_playback_state != ComputeActualPlaybackState()) { UpdateMediaSessionState(); } @@ -272,10 +281,14 @@ void MediaSessionClient::UpdateMediaSessionState() { metadata->set_artwork(session_metadata->artwork()); } - session_state_ = MediaSessionState( - metadata, media_session_->last_position_updated_time_, - media_session_->media_position_state_, ComputeActualPlaybackState(), - ComputeAvailableActions()); + { + ScopedLock scoped_lock(session_state_mutex_); + + session_state_ = MediaSessionState( + metadata, media_session_->last_position_updated_time_, + media_session_->media_position_state_, ComputeActualPlaybackState(), + ComputeAvailableActions()); + } // Compute the media position state if it's not set in the media session. if (!media_session_->media_position_state_ && media_player_factory_) { @@ -299,55 +312,64 @@ void MediaSessionClient::OnMediaSessionStateChanged( const MediaSessionState& session_state) { if (extension_ && extension_->version >= 1) { CobaltExtensionMediaSessionState ext_state; - size_t artwork_size = 0; - if (session_state.has_metadata() && - session_state.metadata().value().has_artwork()) { - artwork_size = session_state.metadata().value().artwork().size(); - } - std::unique_ptr ext_artwork = - std::unique_ptr( - new CobaltExtensionMediaImage[artwork_size]); - - ext_state.duration = session_state.duration(); - ext_state.actual_playback_rate = session_state.actual_playback_rate(); - ext_state.current_playback_position = - session_state.current_playback_position(); - ext_state.has_position_state = session_state.has_position_state(); - ext_state.actual_playback_state = - ConvertPlaybackState(session_state.actual_playback_state()); - ConvertMediaSessionActions(session_state.available_actions(), - ext_state.available_actions); + CobaltExtensionMediaMetadata ext_metadata; std::string album = ""; std::string artist = ""; std::string title = ""; - - if (session_state.has_metadata()) { - const MediaMetadataInit& metadata = session_state.metadata().value(); - album = metadata.album(); - artist = metadata.artist(); - title = metadata.title(); - if (artwork_size > 0) { - const MediaImageSequence& artwork(metadata.artwork()); - for (MediaImageSequence::size_type i = 0; i < artwork_size; i++) { - const MediaImage& media_image(artwork.at(i)); - CobaltExtensionMediaImage ext_image; - ext_image.src = media_image.src().c_str(); - if (ext_image.src == nullptr) { - // src() is required, but Cobalt IDL parser doesn't enforce it. - // See cobalt/media_session/media_image.idl for more info. - // https://wicg.github.io/mediasession/#dictdef-mediaimage - LOG(ERROR) << "Required src string for MediaImage is missing."; + std::unique_ptr ext_artwork; + std::vector image_srcs; + std::vector image_sizes; + std::vector image_types; + + { + ScopedLock scoped_lock(session_state_mutex_); + + size_t artwork_size = 0; + if (session_state.has_metadata() && + session_state.metadata().value().has_artwork()) { + artwork_size = session_state.metadata().value().artwork().size(); + } + ext_artwork = std::unique_ptr( + new CobaltExtensionMediaImage[artwork_size]); + + ext_state.duration = session_state.duration(); + ext_state.actual_playback_rate = session_state.actual_playback_rate(); + ext_state.current_playback_position = + session_state.current_playback_position(); + ext_state.has_position_state = session_state.has_position_state(); + ext_state.actual_playback_state = + ConvertPlaybackState(session_state.actual_playback_state()); + ConvertMediaSessionActions(session_state.available_actions(), + ext_state.available_actions); + + if (session_state.has_metadata()) { + const MediaMetadataInit& metadata = session_state.metadata().value(); + album = metadata.album(); + artist = metadata.artist(); + title = metadata.title(); + if (artwork_size > 0) { + const MediaImageSequence& artwork = metadata.artwork(); + for (MediaImageSequence::size_type i = 0; i < artwork_size; i++) { + const MediaImage& media_image = artwork.at(i); + image_srcs.push_back(media_image.src()); + ext_artwork[i].src = image_srcs.back().c_str(); + if (ext_artwork[i].src == nullptr) { + // src() is required, but Cobalt IDL parser doesn't enforce it. + // See cobalt/media_session/media_image.idl for more info. + // https://wicg.github.io/mediasession/#dictdef-mediaimage + LOG(ERROR) << "Required src string for MediaImage is missing."; + } + image_sizes.push_back(media_image.sizes()); + ext_artwork[i].size = image_sizes.back().c_str(); + image_types.push_back(media_image.type()); + ext_artwork[i].type = image_types.back().c_str(); } - ext_image.size = media_image.sizes().c_str(); - ext_image.type = media_image.type().c_str(); - ext_artwork[i] = ext_image; } } + ext_metadata = {album.c_str(), artist.c_str(), title.c_str(), + ext_artwork.get(), artwork_size}; + ext_state.metadata = &ext_metadata; } - CobaltExtensionMediaMetadata ext_metadata = { - album.c_str(), artist.c_str(), title.c_str(), ext_artwork.get(), - artwork_size}; - ext_state.metadata = &ext_metadata; extension_->OnMediaSessionStateChanged(ext_state); } diff --git a/cobalt/media_session/media_session_client.h b/cobalt/media_session/media_session_client.h index 3c174982fcb3..4929977fd597 100644 --- a/cobalt/media_session/media_session_client.h +++ b/cobalt/media_session/media_session_client.h @@ -25,6 +25,7 @@ #include "cobalt/media_session/media_session.h" #include "cobalt/media_session/media_session_action_details.h" #include "cobalt/media_session/media_session_state.h" +#include "starboard/common/mutex.h" #include "starboard/extension/media_session.h" namespace cobalt { @@ -106,6 +107,7 @@ class MediaSessionClient : public base::SupportsWeakPtr { MediaSessionPlaybackState platform_playback_state_; const media::WebMediaPlayerFactory* media_player_factory_ = nullptr; const CobaltExtensionMediaSessionApi* extension_; + starboard::Mutex session_state_mutex_; void UpdateMediaSessionState(); MediaSessionPlaybackState ComputeActualPlaybackState() const; diff --git a/starboard/android/shared/android_media_session_client.cc b/starboard/android/shared/android_media_session_client.cc index ddea11679f16..41dc1b392fb1 100644 --- a/starboard/android/shared/android_media_session_client.cc +++ b/starboard/android/shared/android_media_session_client.cc @@ -186,7 +186,7 @@ void OnMediaSessionStateChanged( ScopedLocalJavaRef j_artwork; if (session_state.metadata != NULL) { - CobaltExtensionMediaMetadata* media_metadata(session_state.metadata); + const CobaltExtensionMediaMetadata* media_metadata = session_state.metadata; j_title.Reset(env->NewStringStandardUTFOrAbort(media_metadata->title)); j_artist.Reset(env->NewStringStandardUTFOrAbort(media_metadata->artist)); @@ -194,7 +194,7 @@ void OnMediaSessionStateChanged( size_t artwork_count = media_metadata->artwork_count; if (artwork_count > 0) { - CobaltExtensionMediaImage* artwork(media_metadata->artwork); + const CobaltExtensionMediaImage* artwork = media_metadata->artwork; ScopedLocalJavaRef media_image_class( env->FindClassExtOrAbort("dev/cobalt/media/MediaImage")); jmethodID media_image_constructor = env->GetMethodID( @@ -210,7 +210,7 @@ void OnMediaSessionStateChanged( ScopedLocalJavaRef j_sizes; ScopedLocalJavaRef j_type; for (size_t i = 0; i < artwork_count; i++) { - const CobaltExtensionMediaImage& media_image(artwork[i]); + const CobaltExtensionMediaImage& media_image = artwork[i]; j_src.Reset(env->NewStringStandardUTFOrAbort(media_image.src)); j_sizes.Reset(env->NewStringStandardUTFOrAbort(media_image.size)); j_type.Reset(env->NewStringStandardUTFOrAbort(media_image.type));