Skip to content

Commit

Permalink
Fix crash caused by Use after free
Browse files Browse the repository at this point in the history
ext_image is released after the for loop, do not create it, write
directly to the ext_artwork array.

b/329330347
  • Loading branch information
Colin Liang committed Mar 27, 2024
1 parent c65b33a commit 7985305
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 59 deletions.
97 changes: 46 additions & 51 deletions cobalt/media_session/media_session_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <cmath>
#include <memory>
#include <string>
#include <vector>

#include "base/logging.h"
#include "cobalt/media_session/media_image.h"
Expand Down Expand Up @@ -297,60 +298,54 @@ void MediaSessionClient::UpdateMediaSessionState() {

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);
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.";
}
ext_image.size = media_image.sizes().c_str();
ext_image.type = media_image.type().c_str();
ext_artwork[i] = ext_image;
}
if (!extension_ || extension_->version < 1) {
return;
}

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();

Check warning on line 309 in cobalt/media_session/media_session_client.cc

View check run for this annotation

Codecov / codecov/patch

cobalt/media_session/media_session_client.cc#L305-L309

Added lines #L305 - L309 were not covered by tests
}
std::vector<CobaltExtensionMediaImage> ext_artwork(artwork_size);

Check warning on line 311 in cobalt/media_session/media_session_client.cc

View check run for this annotation

Codecov / codecov/patch

cobalt/media_session/media_session_client.cc#L311

Added line #L311 was not covered by tests

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);
std::string album = "";
std::string artist = "";
std::string title = "";

Check warning on line 324 in cobalt/media_session/media_session_client.cc

View check run for this annotation

Codecov / codecov/patch

cobalt/media_session/media_session_client.cc#L313-L324

Added lines #L313 - L324 were not covered by tests

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();
ext_image.size = media_image.sizes().c_str();
ext_image.type = media_image.type().c_str();
ext_artwork[i] = ext_image;

Check warning on line 339 in cobalt/media_session/media_session_client.cc

View check run for this annotation

Codecov / codecov/patch

cobalt/media_session/media_session_client.cc#L326-L339

Added lines #L326 - L339 were not covered by tests
}
}
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);
}
CobaltExtensionMediaMetadata ext_metadata = {
album.c_str(), artist.c_str(), title.c_str(), ext_artwork.data(),
artwork_size};
ext_state.metadata = &ext_metadata;

Check warning on line 346 in cobalt/media_session/media_session_client.cc

View check run for this annotation

Codecov / codecov/patch

cobalt/media_session/media_session_client.cc#L343-L346

Added lines #L343 - L346 were not covered by tests

extension_->OnMediaSessionStateChanged(ext_state);

Check warning on line 348 in cobalt/media_session/media_session_client.cc

View check run for this annotation

Codecov / codecov/patch

cobalt/media_session/media_session_client.cc#L348

Added line #L348 was not covered by tests
}

// static
Expand Down
11 changes: 3 additions & 8 deletions starboard/android/shared/android_media_session_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,6 @@ void OnMediaSessionStateChanged(
jint playback_state = CobaltExtensionPlaybackStateToPlaybackState(
session_state.actual_playback_state);

SbOnce(&once_flag, OnceInit);
SbMutexAcquire(&mutex);

SbMutexRelease(&mutex);

jlong playback_state_actions = MediaSessionActionsToPlaybackStateActions(
session_state.available_actions);

Expand All @@ -186,15 +181,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(
Expand All @@ -210,7 +205,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));
Expand Down

0 comments on commit 7985305

Please sign in to comment.