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

@@ -18,6 +18,7 @@
 #include <cmath>
 #include <memory>
 #include <string>
+#include <vector>
 
 #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<MediaImage>;
 
 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<CobaltExtensionMediaImage[]> ext_artwork =
-        std::unique_ptr<CobaltExtensionMediaImage[]>(
-            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<CobaltExtensionMediaImage[]> ext_artwork;
+    std::vector<std::string> image_srcs;
+    std::vector<std::string> image_sizes;
+    std::vector<std::string> 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<CobaltExtensionMediaImage[]>(
+          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);
   }

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<MediaSessionClient> {
   MediaSessionPlaybackState platform_playback_state_;
   const media::WebMediaPlayerFactory* media_player_factory_ = nullptr;
   const CobaltExtensionMediaSessionApi* extension_;
+  starboard::Mutex session_state_mutex_;
 
   void UpdateMediaSessionState();
   MediaSessionPlaybackState ComputeActualPlaybackState() const;

starboard/android/shared/android_media_session_client.cc

@@ -186,15 +186,15 @@ void OnMediaSessionStateChanged(
   ScopedLocalJavaRef<jobjectArray> 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));
     j_album.Reset(env->NewStringStandardUTFOrAbort(media_metadata->album));
 
     size_t artwork_count = media_metadata->artwork_count;
     if (artwork_count > 0) {
-      CobaltExtensionMediaImage* artwork(media_metadata->artwork);
+      const CobaltExtensionMediaImage* artwork = media_metadata->artwork;
       ScopedLocalJavaRef<jclass> media_image_class(
           env->FindClassExtOrAbort("dev/cobalt/media/MediaImage"));
       jmethodID media_image_constructor = env->GetMethodID(
@@ -210,7 +210,7 @@ void OnMediaSessionStateChanged(
       ScopedLocalJavaRef<jstring> j_sizes;
       ScopedLocalJavaRef<jstring> 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));