[PATCH 0/2] MR2496: Draft: Fix hang when a program calls IMFMediaSession_Start with an invalid source.
This fixes a bug when the session topology contains an invalid source, which makes the session thread to hang and stop executing commands. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mf/tests/mf.c | 131 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 123 insertions(+), 8 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 399f983983f..88838b4629a 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -41,6 +41,39 @@ #include "initguid.h" #include "evr9.h" +#define DEFINE_EXPECT(func) \ + static BOOL expect_ ## func = FALSE, called_ ## func = FALSE + +#define SET_EXPECT(func) \ + expect_ ## func = TRUE + +#define CHECK_EXPECT2(func) \ + do { \ + ok(expect_ ##func, "unexpected call " #func "\n"); \ + called_ ## func = TRUE; \ + }while(0) + +#define CHECK_EXPECT(func) \ + do { \ + CHECK_EXPECT2(func); \ + expect_ ## func = FALSE; \ + }while(0) + +#define CHECK_CALLED(func) \ + do { \ + ok(called_ ## func, "expected " #func "\n"); \ + expect_ ## func = called_ ## func = FALSE; \ + }while(0) + +#define CHECK_NOT_CALLED(func) \ + do { \ + ok(!called_ ## func, "unexpected " #func "\n"); \ + expect_ ## func = called_ ## func = FALSE; \ + }while(0) + +#define CLEAR_CALLED(func) \ + expect_ ## func = called_ ## func = FALSE + extern GUID DMOVideoFormat_RGB32; HRESULT (WINAPI *pMFCreateSampleCopierMFT)(IMFTransform **copier); @@ -236,6 +269,15 @@ static void init_sink_node(IMFStreamSink *stream_sink, MF_CONNECT_METHOD method, } } +DEFINE_EXPECT(test_source_GetEvent); +DEFINE_EXPECT(test_source_BeginGetEvent); +DEFINE_EXPECT(test_source_EndGetEvent); +DEFINE_EXPECT(test_source_QueueEvent); +DEFINE_EXPECT(test_source_Start); +DEFINE_EXPECT(test_source_Stop); +DEFINE_EXPECT(test_source_Pause); +DEFINE_EXPECT(test_source_Shutdown); + struct test_source { IMFMediaSource IMFMediaSource_iface; @@ -288,26 +330,26 @@ static ULONG WINAPI test_source_Release(IMFMediaSource *iface) static HRESULT WINAPI test_source_GetEvent(IMFMediaSource *iface, DWORD flags, IMFMediaEvent **event) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_GetEvent); return E_NOTIMPL; } static HRESULT WINAPI test_source_BeginGetEvent(IMFMediaSource *iface, IMFAsyncCallback *callback, IUnknown *state) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_BeginGetEvent); return E_NOTIMPL; } static HRESULT WINAPI test_source_EndGetEvent(IMFMediaSource *iface, IMFAsyncResult *result, IMFMediaEvent **event) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_EndGetEvent); return E_NOTIMPL; } static HRESULT WINAPI test_source_QueueEvent(IMFMediaSource *iface, MediaEventType event_type, REFGUID ext_type, HRESULT hr, const PROPVARIANT *value) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_QueueEvent); return E_NOTIMPL; } @@ -326,25 +368,25 @@ static HRESULT WINAPI test_source_CreatePresentationDescriptor(IMFMediaSource *i static HRESULT WINAPI test_source_Start(IMFMediaSource *iface, IMFPresentationDescriptor *pd, const GUID *time_format, const PROPVARIANT *start_position) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_Start); return E_NOTIMPL; } static HRESULT WINAPI test_source_Stop(IMFMediaSource *iface) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_Stop); return E_NOTIMPL; } static HRESULT WINAPI test_source_Pause(IMFMediaSource *iface) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_Pause); return E_NOTIMPL; } static HRESULT WINAPI test_source_Shutdown(IMFMediaSource *iface) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_Shutdown); return E_NOTIMPL; } @@ -1914,6 +1956,41 @@ static HRESULT wait_media_event_(int line, IMFMediaSession *session, IMFAsyncCal return status; } +#define wait_media_event_until_blocking(a, b, c, d, e) wait_media_event_until_blocking_(__LINE__, a, b, c, d, e) +static HRESULT wait_media_event_until_blocking_(int line, IMFMediaSession *session, IMFAsyncCallback *callback, + MediaEventType expect_type, DWORD timeout, PROPVARIANT *value) +{ + struct test_callback *impl = impl_from_IMFAsyncCallback(callback); + MediaEventType type; + HRESULT hr, status; + DWORD ret; + GUID guid; + + do + { + hr = IMFMediaSession_BeginGetEvent(session, &impl->IMFAsyncCallback_iface, (IUnknown *)session); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ret = WaitForSingleObject(impl->event, timeout); + if (ret == WAIT_TIMEOUT) return WAIT_TIMEOUT; + hr = IMFMediaEvent_GetType(impl->media_event, &type); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + } while (type != expect_type); + + ok_(__FILE__, line)(type == expect_type, "got type %lu\n", type); + + hr = IMFMediaEvent_GetExtendedType(impl->media_event, &guid); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok_(__FILE__, line)(IsEqualGUID(&guid, &GUID_NULL), "got extended type %s\n", debugstr_guid(&guid)); + + hr = IMFMediaEvent_GetValue(impl->media_event, value); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaEvent_GetStatus(impl->media_event, &status); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + return status; +} + static IMFMediaSource *create_media_source(const WCHAR *name, const WCHAR *mime) { IMFSourceResolver *resolver; @@ -2363,6 +2440,44 @@ static void test_media_session_events(void) /* sometimes briefly leaking */ IMFMediaSession_Release(session); + /* test IMFMediaSession_Start with test source */ + SET_EXPECT(test_source_BeginGetEvent); + SET_EXPECT(test_source_QueueEvent); + SET_EXPECT(test_source_Start); + + hr = MFCreateMediaSession(NULL, &session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_SetTopology(session, 0, topology); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionTopologySet, 1000, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(propvar.vt == VT_UNKNOWN, "got vt %u\n", propvar.vt); + ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); + PropVariantClear(&propvar); + + propvar.vt = VT_EMPTY; + hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 1000, &propvar); + todo_wine ok(hr == E_NOTIMPL, "Unexpected hr %#lx.\n", hr); + ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt); + ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); + PropVariantClear(&propvar); + + hr = IMFMediaSession_ClearTopologies(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_Shutdown(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + CLEAR_CALLED(test_source_BeginGetEvent); + CLEAR_CALLED(test_source_QueueEvent); + CLEAR_CALLED(test_source_Start); + + /* sometimes briefly leaking */ + IMFMediaSession_Release(session); + IMFAsyncCallback_Release(callback); if (handler.current_type) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
From: Santino Mazza <smazza(a)codeweavers.com> This fixes a bug when the session topology contains an invalid source, which makes the session thread to hang and stop executing commands. --- dlls/mf/session.c | 7 +++++++ dlls/mf/tests/mf.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 85baf07d05a..67a4ba31fb4 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -880,6 +880,8 @@ static void session_subscribe_sources(struct media_session *session) source->object))) { WARN("Failed to subscribe to source events, hr %#lx.\n", hr); + session_command_complete_with_event(session, MESessionStarted, hr, NULL); + return; } } @@ -910,11 +912,16 @@ static void session_start(struct media_session *session, const GUID *time_format PropVariantCopy(&session->presentation.start_position, start_position); session_subscribe_sources(session); + if (!(session->presentation.flags & SESSION_FLAG_SOURCES_SUBSCRIBED)) break; LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) { if (FAILED(hr = IMFMediaSource_Start(source->source, source->pd, &GUID_NULL, start_position))) + { WARN("Failed to start media source %p, hr %#lx.\n", source->source, hr); + session_command_complete_with_event(session, MESessionStarted, hr, NULL); + return; + } } session->state = SESSION_STATE_STARTING_SOURCES; diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 88838b4629a..76dc1a1bec9 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -2460,7 +2460,7 @@ static void test_media_session_events(void) hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 1000, &propvar); - todo_wine ok(hr == E_NOTIMPL, "Unexpected hr %#lx.\n", hr); + ok(hr == E_NOTIMPL, "Unexpected hr %#lx.\n", hr); ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt); ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); PropVariantClear(&propvar); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
This looks more like a proof of concept right now. First, I don't think we need such explicit tests for call sequences. Second, subscribe helper is used in several other places. Finally the fix touches unrelated parts. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2496#note_27783
participants (3)
-
Nikolay Sivov (＠nsivov) -
Santino Mazza
-
Santino Mazza (＠tati1454)