Skip to content
This repository was archived by the owner on Jan 16, 2024. It is now read-only.

Commit 5990e30

Browse files
tbluc4tbluc4
committed
GstreamerMediaPlayer: Fix callback function use-after-free
Allocate a copy of the callback function and promise, and keep it until it was executed. This fixes a problem where the callback function could be freed as soon as the callback function sets the promise value and the thread waiting on the promise exits the function immediately, freeing the closure data while it is still executing.
1 parent 8e77a64 commit 5990e30

File tree

5 files changed

+84
-89
lines changed

5 files changed

+84
-89
lines changed

‎MediaPlayer/GStreamerMediaPlayer/include/MediaPlayer/BaseStreamSource.h

-6
Original file line numberDiff line numberDiff line change
@@ -181,12 +181,6 @@ class BaseStreamSource : public SourceInterface {
181181
/// Number of times reading data has been attempted since data was last successfully read.
182182
guint m_sourceRetryCount;
183183

184-
/// Function to invoke on the worker thread thread when more data is needed.
185-
const std::function<gboolean()> m_handleNeedDataFunction;
186-
187-
/// Function to invoke on the worker thread thread when there is enough data.
188-
const std::function<gboolean()> m_handleEnoughDataFunction;
189-
190184
/// ID of the handler installed to receive need data signals.
191185
guint m_needDataHandlerId;
192186

‎MediaPlayer/GStreamerMediaPlayer/include/MediaPlayer/MediaPlayer.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ class MediaPlayer
138138
void setDecoder(GstElement* decoder) override;
139139
GstElement* getDecoder() const override;
140140
GstElement* getPipeline() const override;
141-
guint queueCallback(const std::function<gboolean()>* callback) override;
141+
guint queueCallback(std::function<gboolean()>&& callback) override;
142142
guint attachSource(GSource* source) override;
143143
gboolean removeSource(guint tag) override;
144144
/// @}

‎MediaPlayer/GStreamerMediaPlayer/include/MediaPlayer/PipelineInterface.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ class PipelineInterface {
7575
* @param callback The callback to queue.
7676
* @return The ID of the queued callback (for calling @c g_source_remove).
7777
*/
78-
virtual guint queueCallback(const std::function<gboolean()>* callback) = 0;
78+
virtual guint queueCallback(std::function<gboolean()>&& callback) = 0;
7979

8080
/**
8181
* Attach the source to the worker thread.

‎MediaPlayer/GStreamerMediaPlayer/src/BaseStreamSource.cpp

+2-4
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,6 @@ BaseStreamSource::BaseStreamSource(PipelineInterface* pipeline, const std::strin
8989
m_sourceId{0},
9090
m_hasReadData{false},
9191
m_sourceRetryCount{0},
92-
m_handleNeedDataFunction{[this]() { return handleNeedData(); }},
93-
m_handleEnoughDataFunction{[this]() { return handleEnoughData(); }},
9492
m_needDataHandlerId{0},
9593
m_enoughDataHandlerId{0},
9694
m_seekDataHandlerId{0},
@@ -311,7 +309,7 @@ void BaseStreamSource::onNeedData(GstElement* pipeline, guint size, gpointer poi
311309
ACSDK_DEBUG9(LX("m_needDataCallbackId already set"));
312310
return;
313311
}
314-
source->m_needDataCallbackId = source->m_pipeline->queueCallback(&source->m_handleNeedDataFunction);
312+
source->m_needDataCallbackId = source->m_pipeline->queueCallback([source]() { return source->handleNeedData(); });
315313
}
316314

317315
gboolean BaseStreamSource::handleNeedData() {
@@ -330,7 +328,7 @@ void BaseStreamSource::onEnoughData(GstElement* pipeline, gpointer pointer) {
330328
ACSDK_DEBUG9(LX("m_enoughDataCallbackId already set"));
331329
return;
332330
}
333-
source->m_enoughDataCallbackId = source->m_pipeline->queueCallback(&source->m_handleEnoughDataFunction);
331+
source->m_enoughDataCallbackId = source->m_pipeline->queueCallback([source]() { return source->handleEnoughData(); });
334332
}
335333

336334
gboolean BaseStreamSource::handleEnoughData() {

‎MediaPlayer/GStreamerMediaPlayer/src/MediaPlayer.cpp

+80-77
Original file line numberDiff line numberDiff line change
@@ -200,13 +200,13 @@ MediaPlayer::SourceId MediaPlayer::setSource(
200200
const avsCommon::utils::AudioFormat* audioFormat,
201201
const SourceConfig& config) {
202202
ACSDK_DEBUG9(LX("setSourceCalled").d("name", RequiresShutdown::name()).d("sourceType", "AttachmentReader"));
203-
std::promise<MediaPlayer::SourceId> promise;
204-
auto future = promise.get_future();
205-
std::function<gboolean()> callback = [this, &reader, &promise, &config, audioFormat]() {
206-
handleSetAttachmentReaderSource(std::move(reader), config, &promise, audioFormat);
203+
auto promise = std::make_shared<std::promise<MediaPlayer::SourceId>>();
204+
auto future = promise->get_future();
205+
std::function<gboolean()> callback = [this, &reader, promise, &config, audioFormat]() {
206+
handleSetAttachmentReaderSource(std::move(reader), config, promise.get(), audioFormat);
207207
return false;
208208
};
209-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
209+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
210210
auto sourceId = future.get();
211211
// Assume that the Attachment is fully buffered - not ideal, revisit if needed. Should be fine for file streams
212212
// and resources.
@@ -223,13 +223,13 @@ MediaPlayer::SourceId MediaPlayer::setSource(
223223
avsCommon::utils::MediaType format) {
224224
ACSDK_DEBUG9(
225225
LX("setSourceCalled").d("name", RequiresShutdown::name()).d("sourceType", "istream").d("format", format));
226-
std::promise<MediaPlayer::SourceId> promise;
227-
auto future = promise.get_future();
228-
std::function<gboolean()> callback = [this, &stream, repeat, &config, &promise]() {
229-
handleSetIStreamSource(stream, repeat, config, &promise);
226+
auto promise = std::make_shared<std::promise<MediaPlayer::SourceId>>();
227+
auto future = promise->get_future();
228+
std::function<gboolean()> callback = [this, &stream, repeat, &config, promise]() {
229+
handleSetIStreamSource(stream, repeat, config, promise.get());
230230
return false;
231231
};
232-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
232+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
233233
auto sourceId = future.get();
234234
// Assume that the Attachment is fully buffered - not ideal, revisit if needed. Should be fine for file streams
235235
// and resources.
@@ -246,13 +246,13 @@ MediaPlayer::SourceId MediaPlayer::setSource(
246246
bool repeat,
247247
const PlaybackContext& playbackContext) {
248248
ACSDK_DEBUG9(LX("setSourceForUrlCalled").d("name", RequiresShutdown::name()).sensitive("url", url));
249-
std::promise<MediaPlayer::SourceId> promise;
250-
auto future = promise.get_future();
251-
std::function<gboolean()> callback = [this, url, offset, &config, &promise, repeat]() {
252-
handleSetUrlSource(url, offset, config, &promise, repeat);
249+
auto promise = std::make_shared<std::promise<MediaPlayer::SourceId>>();
250+
auto future = promise->get_future();
251+
std::function<gboolean()> callback = [this, url, offset, &config, promise, repeat]() {
252+
handleSetUrlSource(url, offset, config, promise.get(), repeat);
253253
return false;
254254
};
255-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
255+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
256256
return future.get();
257257
}
258258
return ERROR_SOURCE_ID;
@@ -281,71 +281,71 @@ bool MediaPlayer::play(MediaPlayer::SourceId id) {
281281

282282
m_source->preprocess();
283283

284-
std::promise<bool> promise;
285-
auto future = promise.get_future();
286-
std::function<gboolean()> callback = [this, id, &promise]() {
287-
handlePlay(id, &promise);
284+
auto promise = std::make_shared<std::promise<bool>>();
285+
auto future = promise->get_future();
286+
std::function<gboolean()> callback = [this, id, promise]() {
287+
handlePlay(id, promise.get());
288288
return false;
289289
};
290290

291-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
291+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
292292
return future.get();
293293
}
294294
return false;
295295
}
296296

297297
bool MediaPlayer::stop(MediaPlayer::SourceId id) {
298298
ACSDK_DEBUG9(LX("stopCalled").d("name", RequiresShutdown::name()));
299-
std::promise<bool> promise;
300-
auto future = promise.get_future();
301-
std::function<gboolean()> callback = [this, id, &promise]() {
302-
handleStop(id, &promise);
299+
auto promise = std::make_shared<std::promise<bool>>();
300+
auto future = promise->get_future();
301+
std::function<gboolean()> callback = [this, id, promise]() {
302+
handleStop(id, promise.get());
303303
return false;
304304
};
305-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
305+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
306306
return future.get();
307307
}
308308
return false;
309309
}
310310

311311
bool MediaPlayer::pause(MediaPlayer::SourceId id) {
312312
ACSDK_DEBUG9(LX("pausedCalled").d("name", RequiresShutdown::name()));
313-
std::promise<bool> promise;
314-
auto future = promise.get_future();
315-
std::function<gboolean()> callback = [this, id, &promise]() {
316-
handlePause(id, &promise);
313+
auto promise = std::make_shared<std::promise<bool>>();
314+
auto future = promise->get_future();
315+
std::function<gboolean()> callback = [this, id, promise]() {
316+
handlePause(id, promise.get());
317317
return false;
318318
};
319-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
319+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
320320
return future.get();
321321
}
322322
return false;
323323
}
324324

325325
bool MediaPlayer::resume(MediaPlayer::SourceId id) {
326326
ACSDK_DEBUG9(LX("resumeCalled").d("name", RequiresShutdown::name()));
327-
std::promise<bool> promise;
328-
auto future = promise.get_future();
329-
std::function<gboolean()> callback = [this, id, &promise]() {
330-
handleResume(id, &promise);
327+
auto promise = std::make_shared<std::promise<bool>>();
328+
auto future = promise->get_future();
329+
std::function<gboolean()> callback = [this, id, promise]() {
330+
handleResume(id, promise.get());
331331
return false;
332332
};
333-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
333+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
334334
return future.get();
335335
}
336336
return false;
337337
}
338338

339339
std::chrono::milliseconds MediaPlayer::getOffset(MediaPlayer::SourceId id) {
340340
ACSDK_DEBUG9(LX("getOffsetCalled").d("name", RequiresShutdown::name()));
341-
std::promise<std::chrono::milliseconds> promise;
342-
auto future = promise.get_future();
343-
std::function<gboolean()> callback = [this, id, &promise]() {
344-
handleGetOffset(id, &promise);
341+
auto promise = std::make_shared<std::promise<std::chrono::milliseconds>>();
342+
auto future = promise->get_future();
343+
std::function<gboolean()> callback = [this, id, promise]() {
344+
handleGetOffset(id, promise.get());
345345
return false;
346346
};
347347

348-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
348+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
349349
return future.get();
350350
}
351351
return MEDIA_PLAYER_INVALID_OFFSET;
@@ -363,14 +363,14 @@ void MediaPlayer::addObserver(std::shared_ptr<MediaPlayerObserverInterface> obse
363363
}
364364

365365
ACSDK_DEBUG9(LX("addObserverCalled").d("name", RequiresShutdown::name()));
366-
std::promise<void> promise;
367-
auto future = promise.get_future();
368-
std::function<gboolean()> callback = [this, &promise, &observer]() {
369-
handleAddObserver(&promise, observer);
366+
auto promise = std::make_shared<std::promise<void>>();
367+
auto future = promise->get_future();
368+
std::function<gboolean()> callback = [this, promise, &observer]() {
369+
handleAddObserver(promise.get(), observer);
370370
return false;
371371
};
372372

373-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
373+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
374374
future.wait();
375375
}
376376
}
@@ -382,27 +382,27 @@ void MediaPlayer::removeObserver(std::shared_ptr<MediaPlayerObserverInterface> o
382382
}
383383

384384
ACSDK_DEBUG9(LX("removeObserverCalled").d("name", RequiresShutdown::name()));
385-
std::promise<void> promise;
386-
auto future = promise.get_future();
387-
std::function<gboolean()> callback = [this, &promise, &observer]() {
388-
handleRemoveObserver(&promise, observer);
385+
auto promise = std::make_shared<std::promise<void>>();
386+
auto future = promise->get_future();
387+
std::function<gboolean()> callback = [this, promise, &observer]() {
388+
handleRemoveObserver(promise.get(), observer);
389389
return false;
390390
};
391391

392-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
392+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
393393
future.wait();
394394
}
395395
}
396396

397397
bool MediaPlayer::setVolume(int8_t volume) {
398398
ACSDK_DEBUG9(LX("setVolumeCalled").d("name", RequiresShutdown::name()));
399-
std::promise<bool> promise;
400-
auto future = promise.get_future();
401-
std::function<gboolean()> callback = [this, &promise, volume]() {
402-
handleSetVolume(&promise, volume);
399+
auto promise = std::make_shared<std::promise<bool>>();
400+
auto future = promise->get_future();
401+
std::function<gboolean()> callback = [this, promise, volume]() {
402+
handleSetVolume(promise.get(), volume);
403403
return false;
404404
};
405-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
405+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
406406
return future.get();
407407
}
408408
return false;
@@ -455,13 +455,13 @@ void MediaPlayer::handleSetVolume(std::promise<bool>* promise, int8_t volume) {
455455

456456
bool MediaPlayer::setMute(bool mute) {
457457
ACSDK_DEBUG9(LX("setMuteCalled").d("name", RequiresShutdown::name()));
458-
std::promise<bool> promise;
459-
auto future = promise.get_future();
460-
std::function<gboolean()> callback = [this, &promise, mute]() {
461-
handleSetMute(&promise, mute);
458+
auto promise = std::make_shared<std::promise<bool>>();
459+
auto future = promise->get_future();
460+
std::function<gboolean()> callback = [this, promise, mute]() {
461+
handleSetMute(promise.get(), mute);
462462
return false;
463463
};
464-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
464+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
465465
return future.get();
466466
}
467467
return false;
@@ -483,13 +483,13 @@ void MediaPlayer::handleSetMute(std::promise<bool>* promise, bool mute) {
483483

484484
bool MediaPlayer::getSpeakerSettings(SpeakerInterface::SpeakerSettings* settings) {
485485
ACSDK_DEBUG9(LX("getSpeakerSettingsCalled").d("name", RequiresShutdown::name()));
486-
std::promise<bool> promise;
487-
auto future = promise.get_future();
488-
std::function<gboolean()> callback = [this, &promise, settings]() {
489-
handleGetSpeakerSettings(&promise, settings);
486+
auto promise = std::make_shared<std::promise<bool>>();
487+
auto future = promise->get_future();
488+
std::function<gboolean()> callback = [this, promise, settings]() {
489+
handleGetSpeakerSettings(promise.get(), settings);
490490
return false;
491491
};
492-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
492+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
493493
return future.get();
494494
}
495495
return false;
@@ -924,13 +924,16 @@ bool MediaPlayer::seek() {
924924
return seekSuccessful;
925925
}
926926

927-
guint MediaPlayer::queueCallback(const std::function<gboolean()>* callback) {
927+
guint MediaPlayer::queueCallback(std::function<gboolean()>&& callback) {
928928
if (isShutdown()) {
929929
return UNQUEUED_CALLBACK;
930930
}
931931
auto source = g_idle_source_new();
932932
g_source_set_callback(
933-
source, reinterpret_cast<GSourceFunc>(&onCallback), const_cast<std::function<gboolean()>*>(callback), nullptr);
933+
source, reinterpret_cast<GSourceFunc>(&onCallback), new std::function<gboolean()>(std::move(callback)),
934+
[](gpointer data) {
935+
delete reinterpret_cast<std::function<gboolean()>*>(data);
936+
});
934937
auto sourceId = g_source_attach(source, m_workerContext);
935938
g_source_unref(source);
936939
return sourceId;
@@ -996,13 +999,13 @@ gboolean MediaPlayer::onCallback(const std::function<gboolean()>* callback) {
996999
void MediaPlayer::onPadAdded(GstElement* decoder, GstPad* pad, gpointer pointer) {
9971000
auto mediaPlayer = static_cast<MediaPlayer*>(pointer);
9981001
ACSDK_DEBUG9(LX("onPadAddedCalled").d("name", mediaPlayer->name()));
999-
std::promise<void> promise;
1000-
auto future = promise.get_future();
1001-
std::function<gboolean()> callback = [mediaPlayer, &promise, decoder, pad]() {
1002-
mediaPlayer->handlePadAdded(&promise, decoder, pad);
1002+
auto promise = std::make_shared<std::promise<void>>();
1003+
auto future = promise->get_future();
1004+
std::function<gboolean()> callback = [mediaPlayer, promise, decoder, pad]() {
1005+
mediaPlayer->handlePadAdded(promise.get(), decoder, pad);
10031006
return false;
10041007
};
1005-
if (mediaPlayer->queueCallback(&callback) != UNQUEUED_CALLBACK) {
1008+
if (mediaPlayer->queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
10061009
future.wait();
10071010
}
10081011
}
@@ -1932,9 +1935,9 @@ void MediaPlayer::setEqualizerBandLevels(EqualizerBandLevelMap bandLevelMap) {
19321935
if (!m_equalizerEnabled) {
19331936
return;
19341937
}
1935-
std::promise<void> promise;
1936-
auto future = promise.get_future();
1937-
std::function<gboolean()> callback = [this, &promise, bandLevelMap]() {
1938+
auto promise = std::make_shared<std::promise<void>>();
1939+
auto future = promise->get_future();
1940+
std::function<gboolean()> callback = [this, promise, bandLevelMap]() {
19381941
auto it = bandLevelMap.find(EqualizerBand::BASS);
19391942
if (bandLevelMap.end() != it) {
19401943
g_object_set(
@@ -1959,10 +1962,10 @@ void MediaPlayer::setEqualizerBandLevels(EqualizerBandLevelMap bandLevelMap) {
19591962
static_cast<gdouble>(clampEqualizerLevel(it->second)),
19601963
NULL);
19611964
}
1962-
promise.set_value();
1965+
promise->set_value();
19631966
return false;
19641967
};
1965-
if (queueCallback(&callback) != UNQUEUED_CALLBACK) {
1968+
if (queueCallback(std::move(callback)) != UNQUEUED_CALLBACK) {
19661969
future.get();
19671970
}
19681971
}

0 commit comments

Comments
 (0)