RESTRICT AUTOMERGE Prevent MediaPlayerService::Client's use-after-free
Test: make cts -j123 && cts-tradefed run cts-dev -m \
CtsMediaTestCases --compatibility:module-arg \
CtsMediaTestCases:include-annotation:\
android.platform.test.annotations.RequiresDevice
Bug: 70546581
Merged-In: Ia3a8eb99c2faf6935c63800ba08f65970cede48e
Change-Id: Ia142a7735c6685eb67b2c00917c0ed5ea7e0da9e
(cherry picked from commit c7fe16530e1ab27a1298114925334e1c46ebd36f)
diff --git a/include/media/MediaPlayerInterface.h b/include/media/MediaPlayerInterface.h
index 4977efd..cde4f1d 100644
--- a/include/media/MediaPlayerInterface.h
+++ b/include/media/MediaPlayerInterface.h
@@ -65,14 +65,17 @@
// duration below which we do not allow deep audio buffering
#define AUDIO_SINK_MIN_DEEP_BUFFER_DURATION_US 5000000
-// callback mechanism for passing messages to MediaPlayer object
-typedef void (*notify_callback_f)(void* cookie,
- int msg, int ext1, int ext2, const Parcel *obj);
-
// abstract base class - use MediaPlayerInterface
class MediaPlayerBase : public RefBase
{
public:
+ // callback mechanism for passing messages to MediaPlayer object
+ class Listener : public RefBase {
+ public:
+ virtual void notify(int msg, int ext1, int ext2, const Parcel *obj) = 0;
+ virtual ~Listener() {}
+ };
+
// AudioSink: abstraction layer for audio output
class AudioSink : public RefBase {
public:
@@ -146,7 +149,7 @@
virtual String8 getParameters(const String8& /* keys */) { return String8::empty(); }
};
- MediaPlayerBase() : mCookie(0), mNotify(0) {}
+ MediaPlayerBase() {}
virtual ~MediaPlayerBase() {}
virtual status_t initCheck() = 0;
virtual bool hardwareOutput() = 0;
@@ -247,22 +250,22 @@
};
void setNotifyCallback(
- void* cookie, notify_callback_f notifyFunc) {
+ const sp<Listener> &listener) {
Mutex::Autolock autoLock(mNotifyLock);
- mCookie = cookie; mNotify = notifyFunc;
+ mListener = listener;
}
void sendEvent(int msg, int ext1=0, int ext2=0,
const Parcel *obj=NULL) {
- notify_callback_f notifyCB;
- void* cookie;
+ sp<Listener> listener;
{
Mutex::Autolock autoLock(mNotifyLock);
- notifyCB = mNotify;
- cookie = mCookie;
+ listener = mListener;
}
- if (notifyCB) notifyCB(cookie, msg, ext1, ext2, obj);
+ if (listener != NULL) {
+ listener->notify(msg, ext1, ext2, obj);
+ }
}
virtual status_t dump(int /* fd */, const Vector<String16>& /* args */) const {
@@ -272,9 +275,8 @@
private:
friend class MediaPlayerService;
- Mutex mNotifyLock;
- void* mCookie;
- notify_callback_f mNotify;
+ Mutex mNotifyLock;
+ sp<Listener> mListener;
};
// Implement this class for media players that use the AudioFlinger software mixer
diff --git a/media/libmediaplayerservice/MediaPlayerFactory.cpp b/media/libmediaplayerservice/MediaPlayerFactory.cpp
index 605c710..f5ec20c 100644
--- a/media/libmediaplayerservice/MediaPlayerFactory.cpp
+++ b/media/libmediaplayerservice/MediaPlayerFactory.cpp
@@ -127,8 +127,7 @@
sp<MediaPlayerBase> MediaPlayerFactory::createPlayer(
player_type playerType,
- void* cookie,
- notify_callback_f notifyFunc,
+ const sp<MediaPlayerBase::Listener> &listener,
pid_t pid) {
sp<MediaPlayerBase> p;
IFactory* factory;
@@ -153,7 +152,7 @@
init_result = p->initCheck();
if (init_result == NO_ERROR) {
- p->setNotifyCallback(cookie, notifyFunc);
+ p->setNotifyCallback(listener);
} else {
ALOGE("Failed to create player object of type %d, initCheck failed"
" (res = %d)", playerType, init_result);
diff --git a/media/libmediaplayerservice/MediaPlayerFactory.h b/media/libmediaplayerservice/MediaPlayerFactory.h
index e22a56f..e88700c 100644
--- a/media/libmediaplayerservice/MediaPlayerFactory.h
+++ b/media/libmediaplayerservice/MediaPlayerFactory.h
@@ -65,8 +65,7 @@
const sp<DataSource> &source);
static sp<MediaPlayerBase> createPlayer(player_type playerType,
- void* cookie,
- notify_callback_f notifyFunc,
+ const sp<MediaPlayerBase::Listener> &listener,
pid_t pid);
static void registerBuiltinFactories();
diff --git a/media/libmediaplayerservice/MediaPlayerService.cpp b/media/libmediaplayerservice/MediaPlayerService.cpp
index f8dc0dc..a8e33b1 100644
--- a/media/libmediaplayerservice/MediaPlayerService.cpp
+++ b/media/libmediaplayerservice/MediaPlayerService.cpp
@@ -603,10 +603,11 @@
mUID = uid;
mRetransmitEndpointValid = false;
mAudioAttributes = NULL;
+ mListener = new Listener(this);
#if CALLBACK_ANTAGONIZER
ALOGD("create Antagonizer");
- mAntagonizer = new Antagonizer(notify, this);
+ mAntagonizer = new Antagonizer(mListener);
#endif
}
@@ -642,7 +643,7 @@
// and reset the player. We assume the player will serialize
// access to itself if necessary.
if (p != 0) {
- p->setNotifyCallback(0, 0);
+ p->setNotifyCallback(0);
#if CALLBACK_ANTAGONIZER
ALOGD("kill Antagonizer");
mAntagonizer->kill();
@@ -667,7 +668,7 @@
p.clear();
}
if (p == NULL) {
- p = MediaPlayerFactory::createPlayer(playerType, this, notify, mPid);
+ p = MediaPlayerFactory::createPlayer(playerType, mListener, mPid);
}
if (p != NULL) {
@@ -1321,23 +1322,18 @@
}
void MediaPlayerService::Client::notify(
- void* cookie, int msg, int ext1, int ext2, const Parcel *obj)
+ int msg, int ext1, int ext2, const Parcel *obj)
{
- Client* client = static_cast<Client*>(cookie);
- if (client == NULL) {
- return;
- }
-
sp<IMediaPlayerClient> c;
{
- Mutex::Autolock l(client->mLock);
- c = client->mClient;
- if (msg == MEDIA_PLAYBACK_COMPLETE && client->mNextClient != NULL) {
- if (client->mAudioOutput != NULL)
- client->mAudioOutput->switchToNextOutput();
- client->mNextClient->start();
- if (client->mNextClient->mClient != NULL) {
- client->mNextClient->mClient->notify(
+ Mutex::Autolock l(mLock);
+ c = mClient;
+ if (msg == MEDIA_PLAYBACK_COMPLETE && mNextClient != NULL) {
+ if (mAudioOutput != NULL)
+ mAudioOutput->switchToNextOutput();
+ mNextClient->start();
+ if (mNextClient->mClient != NULL) {
+ mNextClient->mClient->notify(
MEDIA_INFO, MEDIA_INFO_STARTED_AS_NEXT, 0, obj);
}
}
@@ -1347,17 +1343,17 @@
MEDIA_INFO_METADATA_UPDATE == ext1) {
const media::Metadata::Type metadata_type = ext2;
- if(client->shouldDropMetadata(metadata_type)) {
+ if(shouldDropMetadata(metadata_type)) {
return;
}
// Update the list of metadata that have changed. getMetadata
// also access mMetadataUpdated and clears it.
- client->addNewMetadataUpdate(metadata_type);
+ addNewMetadataUpdate(metadata_type);
}
if (c != NULL) {
- ALOGV("[%d] notify (%p, %d, %d, %d)", client->mConnId, cookie, msg, ext1, ext2);
+ ALOGV("[%d] notify (%d, %d, %d)", mConnId, msg, ext1, ext2);
c->notify(msg, ext1, ext2, obj);
}
}
@@ -1389,8 +1385,8 @@
#if CALLBACK_ANTAGONIZER
const int Antagonizer::interval = 10000; // 10 msecs
-Antagonizer::Antagonizer(notify_callback_f cb, void* client) :
- mExit(false), mActive(false), mClient(client), mCb(cb)
+Antagonizer::Antagonizer(const sp<MediaPlayerBase::Listener> &listener) :
+ mExit(false), mActive(false), mListener(listener)
{
createThread(callbackThread, this);
}
@@ -1410,7 +1406,7 @@
while (!p->mExit) {
if (p->mActive) {
ALOGV("send event");
- p->mCb(p->mClient, 0, 0, 0);
+ p->mListener->notify(0, 0, 0, 0);
}
usleep(interval);
}
diff --git a/media/libmediaplayerservice/MediaPlayerService.h b/media/libmediaplayerservice/MediaPlayerService.h
index 42c41cf..c0a4d30 100644
--- a/media/libmediaplayerservice/MediaPlayerService.h
+++ b/media/libmediaplayerservice/MediaPlayerService.h
@@ -49,7 +49,7 @@
#if CALLBACK_ANTAGONIZER
class Antagonizer {
public:
- Antagonizer(notify_callback_f cb, void* client);
+ Antagonizer(const sp<MediaPlayerBase::Listener> &listener);
void start() { mActive = true; }
void stop() { mActive = false; }
void kill();
@@ -57,12 +57,11 @@
static const int interval;
Antagonizer();
static int callbackThread(void* cookie);
- Mutex mLock;
- Condition mCondition;
- bool mExit;
- bool mActive;
- void* mClient;
- notify_callback_f mCb;
+ Mutex mLock;
+ Condition mCondition;
+ bool mExit;
+ bool mActive;
+ sp<MediaPlayerBase::Listener> mListener;
};
#endif
@@ -206,7 +205,6 @@
}; // AudioOutput
-
public:
static void instantiate();
@@ -335,8 +333,7 @@
status_t setDataSource_post(const sp<MediaPlayerBase>& p,
status_t status);
- static void notify(void* cookie, int msg,
- int ext1, int ext2, const Parcel *obj);
+ void notify(int msg, int ext1, int ext2, const Parcel *obj);
pid_t pid() const { return mPid; }
virtual status_t dump(int fd, const Vector<String16>& args);
@@ -391,23 +388,38 @@
status_t setAudioAttributes_l(const Parcel &request);
- mutable Mutex mLock;
- sp<MediaPlayerBase> mPlayer;
- sp<MediaPlayerService> mService;
- sp<IMediaPlayerClient> mClient;
- sp<AudioOutput> mAudioOutput;
- pid_t mPid;
- status_t mStatus;
- bool mLoop;
- int32_t mConnId;
- audio_session_t mAudioSessionId;
- audio_attributes_t * mAudioAttributes;
- uid_t mUID;
- sp<ANativeWindow> mConnectedWindow;
- sp<IBinder> mConnectedWindowBinder;
- struct sockaddr_in mRetransmitEndpoint;
- bool mRetransmitEndpointValid;
- sp<Client> mNextClient;
+ class Listener : public MediaPlayerBase::Listener {
+ public:
+ Listener(const wp<Client> &client) : mClient(client) {}
+ virtual ~Listener() {}
+ virtual void notify(int msg, int ext1, int ext2, const Parcel *obj) {
+ sp<Client> client = mClient.promote();
+ if (client != NULL) {
+ client->notify(msg, ext1, ext2, obj);
+ }
+ }
+ private:
+ wp<Client> mClient;
+ };
+
+ mutable Mutex mLock;
+ sp<MediaPlayerBase> mPlayer;
+ sp<MediaPlayerService> mService;
+ sp<IMediaPlayerClient> mClient;
+ sp<AudioOutput> mAudioOutput;
+ pid_t mPid;
+ status_t mStatus;
+ bool mLoop;
+ int32_t mConnId;
+ audio_session_t mAudioSessionId;
+ audio_attributes_t * mAudioAttributes;
+ uid_t mUID;
+ sp<ANativeWindow> mConnectedWindow;
+ sp<IBinder> mConnectedWindowBinder;
+ struct sockaddr_in mRetransmitEndpoint;
+ bool mRetransmitEndpointValid;
+ sp<Client> mNextClient;
+ sp<MediaPlayerBase::Listener> mListener;
// Metadata filters.
media::Metadata::Filter mMetadataAllow; // protected by mLock
@@ -422,7 +434,7 @@
sp<IBinder::DeathRecipient> mExtractorDeathListener;
sp<IBinder::DeathRecipient> mCodecDeathListener;
#if CALLBACK_ANTAGONIZER
- Antagonizer* mAntagonizer;
+ Antagonizer* mAntagonizer;
#endif
}; // Client
