[PATCH v2 0/3] MR11174: mfreadwrite: Fix race condition resulting in ReadSample never completing
mfreadwrite: Set the reader SEEKING flag early. Previously the SEEKING flag is only set when the ASYNC_SEEK operation is executed. This is too late because of this race condition: - Application calls SetCurrentPosition, which queues the ASYNC_SEEK operation but does not set SEEKING. - Application calls ReadSample. Since the SEEKING flag hasn't been set, it proceeds without waiting and queues an ASYNC_READ operation. - ASYNC_SEEK operation starts its execution, but hasn't reached IMFMediaSource_Start yet. - ASYNC_READ operation starts its execution, and calls IMFMediaStream_RequestSample. Media source queues a request sample job. reader->streams[i]->requests++. ASYNC_READ finishes. - ASYNC_SEEK operation calls IMFMediaSource_Start, the request sample job queued by the media source gets dropped. At this point, there is no request sample job running, but the stream->requests counter is still positive. From this point on the source reader will not request anymore samples, future ReadSample will never complete. Setting the SEEKING flag early, then source reader will always wait in ReadSample so this kind of ordering cannot happen. I used a arbitrary 100 iterations in the test case, because there is really no good way to introduce delays to trigger this race condition predictably. Because it all depends on how the work queue schedules its jobs and I can't stub out the work queue. In my testings the current source reader usually hangs after 2/3 iterations. -- v2: mfreadwrite: Set the reader SEEKING flag early. mfreadwrite/tests: Test some async source reader behaviors. mfreadwrite: Fix segfault in ReadSample error path. https://gitlab.winehq.org/wine/wine/-/merge_requests/11174
From: Yuxuan Shui <yshui@codeweavers.com> If no stream is selected, and a virtual stream index is used for ReadSample, source_reader_queue_response will use that virtual index to index reader->streams, resulting in out of bound access. --- dlls/mfreadwrite/reader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mfreadwrite/reader.c b/dlls/mfreadwrite/reader.c index 68b90357928..fdbc40fc2ba 100644 --- a/dlls/mfreadwrite/reader.c +++ b/dlls/mfreadwrite/reader.c @@ -1543,7 +1543,7 @@ static HRESULT WINAPI source_reader_async_commands_callback_Invoke(IMFAsyncCallb } else { - stub_stream.index = command->u.read.stream_index; + stub_stream.index = hr == MF_E_MEDIA_SOURCE_NO_STREAMS_SELECTED ? 0 : stream_index; source_reader_queue_response(reader, &stub_stream, hr, MF_SOURCE_READERF_ERROR, 0, NULL); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11174
From: Yuxuan Shui <yshui@codeweavers.com> --- dlls/mfreadwrite/tests/mfplat.c | 229 ++++++++++++++++++++++++++++++-- 1 file changed, 219 insertions(+), 10 deletions(-) diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 95e940e2449..763aecc6908 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -263,6 +263,10 @@ struct test_media_stream IMFMediaEventQueue *event_queue; BOOL is_new; LONGLONG sample_duration, sample_time; + unsigned pending_requests; + IUnknown *tokens[10]; + BOOL request_sample_called; + BOOL manual_samples; }; static struct test_media_stream *impl_from_IMFMediaStream(IMFMediaStream *iface) @@ -357,16 +361,11 @@ static HRESULT WINAPI test_media_stream_GetStreamDescriptor(IMFMediaStream *ifac static BOOL fail_request_sample; -static HRESULT WINAPI test_media_stream_RequestSample(IMFMediaStream *iface, IUnknown *token) +static inline HRESULT test_media_stream_queue_sample(struct test_media_stream *stream, IUnknown *token) { - struct test_media_stream *stream = impl_from_IMFMediaStream(iface); IMFMediaBuffer *buffer; IMFSample *sample; HRESULT hr; - - if (fail_request_sample) - return E_NOTIMPL; - hr = MFCreateSample(&sample); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); if (stream->sample_duration) @@ -406,6 +405,42 @@ static HRESULT WINAPI test_media_stream_RequestSample(IMFMediaStream *iface, IUn return S_OK; } +static HRESULT test_media_stream_handle_request_samples(struct test_media_stream *stream) +{ + HRESULT hr; + unsigned i; + + for (i = 0; i < stream->pending_requests; i++) + { + hr = test_media_stream_queue_sample(stream, stream->tokens[i]); + if (stream->tokens[i]) IUnknown_Release(stream->tokens[i]); + if (FAILED(hr)) return hr; + } + + stream->pending_requests = 0; + return S_OK; +} + +static HRESULT WINAPI test_media_stream_RequestSample(IMFMediaStream *iface, IUnknown *token) +{ + struct test_media_stream *stream = impl_from_IMFMediaStream(iface); + HRESULT hr = S_OK; + + if (fail_request_sample) + return E_NOTIMPL; + + if (stream->manual_samples) + { + stream->tokens[stream->pending_requests] = token; + if (token) IUnknown_AddRef(token); + stream->pending_requests++; + } else hr = test_media_stream_queue_sample(stream, token); + + stream->request_sample_called = TRUE; + + return hr; +} + static const IMFMediaStreamVtbl test_media_stream_vtbl = { test_media_stream_QueryInterface, @@ -432,6 +467,8 @@ struct test_source unsigned stream_count; enum source_state state; CRITICAL_SECTION cs; + + BOOL start_called; }; static struct test_source *impl_from_IMFMediaSource(IMFMediaSource *iface) @@ -554,18 +591,29 @@ static HRESULT WINAPI test_source_Start(IMFMediaSource *iface, IMFPresentationDe for (i = 0; i < source->stream_count; ++i) { + struct test_media_stream *stream; + if (!is_stream_selected(pd, i)) continue; + stream = source->streams[i]; + if (stream->pending_requests) + { + unsigned j; + for (j = 0; j < stream->pending_requests; j++) + if (stream->tokens[j]) IUnknown_Release(stream->tokens[j]); + stream->pending_requests = 0; + } + var.vt = VT_UNKNOWN; - var.punkVal = (IUnknown *)&source->streams[i]->IMFMediaStream_iface; + var.punkVal = (IUnknown *)&stream->IMFMediaStream_iface; event_type = source->streams[i]->is_new ? MENewStream : MEUpdatedStream; source->streams[i]->is_new = FALSE; hr = IMFMediaEventQueue_QueueEventParamVar(source->event_queue, event_type, &GUID_NULL, S_OK, &var); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); event_type = source->state == SOURCE_RUNNING ? MEStreamSeeked : MEStreamStarted; - hr = IMFMediaEventQueue_QueueEventParamVar(source->streams[i]->event_queue, event_type, &GUID_NULL, + hr = IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, event_type, &GUID_NULL, S_OK, NULL); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); } @@ -574,6 +622,8 @@ static HRESULT WINAPI test_source_Start(IMFMediaSource *iface, IMFPresentationDe LeaveCriticalSection(&source->cs); + source->start_called = TRUE; + return S_OK; } @@ -720,6 +770,13 @@ struct async_callback IMFSourceReaderCallback IMFSourceReaderCallback_iface; LONG refcount; HANDLE event; + BOOL sample_requested; + + HRESULT hr; + DWORD stream_index; + DWORD stream_flags; + LONGLONG timestamp; + IMFSample *sample; }; static struct async_callback *impl_from_IMFSourceReaderCallback(IMFSourceReaderCallback *iface) @@ -761,7 +818,14 @@ static ULONG WINAPI async_callback_Release(IMFSourceReaderCallback *iface) static HRESULT WINAPI async_callback_OnReadSample(IMFSourceReaderCallback *iface, HRESULT hr, DWORD stream_index, DWORD stream_flags, LONGLONG timestamp, IMFSample *sample) { - ok(0, "Unexpected call.\n"); + struct async_callback *callback = impl_from_IMFSourceReaderCallback(iface); + ok(callback->sample_requested, "Unexpected OnReadSample call.\n"); + callback->stream_index = stream_index; + callback->stream_flags = stream_flags; + callback->timestamp = timestamp; + callback->sample = sample; + callback->hr = hr; + SetEvent(callback->event); return E_NOTIMPL; } @@ -794,6 +858,8 @@ static struct async_callback *create_async_callback(void) callback = calloc(1, sizeof(*callback)); callback->IMFSourceReaderCallback_iface.lpVtbl = &async_callback_vtbl; callback->refcount = 1; + callback->event = CreateEventA(NULL, FALSE, FALSE, NULL); + callback->sample_requested = FALSE; return callback; } @@ -1154,6 +1220,149 @@ static void test_source_reader(const char *filename, bool video) winetest_pop_context(); } +static void test_async_source_reader(void) +{ + static const struct attribute_desc rgb32_stream_type_desc[] = + { + ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), + ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_RGB32), + ATTR_RATIO(MF_MT_FRAME_SIZE, 96, 96), + {0}, + }; + + struct async_callback *callback = create_async_callback(); + IMFStreamDescriptor *video_stream; + struct test_source *test_source; + IMFAttributes *attributes; + IMFMediaType *media_type; + IMFSourceReader *reader; + IMFMediaSource *source; + PROPVARIANT var; + HRESULT hr; + DWORD res, i; + + hr = MFCreateMediaType(&media_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + init_media_type(media_type, rgb32_stream_type_desc, -1); + hr = MFCreateStreamDescriptor(0, 1, &media_type, &video_stream); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + source = create_test_source(&video_stream, 1); + ok(!!source, "Failed to create test source.\n"); + test_source = impl_from_IMFMediaSource(source); + + hr = MFCreateAttributes(&attributes, 1); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFAttributes_SetUnknown(attributes, &MF_SOURCE_READER_ASYNC_CALLBACK, + (IUnknown *)&callback->IMFSourceReaderCallback_iface); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = MFCreateSourceReaderFromMediaSource(source, attributes, &reader); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFSourceReader_SetCurrentMediaType(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, NULL, media_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + /* ReadSample without any streams selected. */ + callback->sample_requested = TRUE; + hr = IMFSourceReader_ReadSample(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, 0, NULL, NULL, NULL, NULL); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + res = WaitForSingleObject(callback->event, 200); + todo_wine ok(!res, "ReadSample didn't finish\n"); + if (!res) + { + ok(callback->sample == NULL, "Unexpected sample\n"); + ok(callback->stream_index == MF_SOURCE_READER_FIRST_VIDEO_STREAM, "Unexpected stream_index %lu\n", callback->stream_index); + ok(callback->stream_flags == 1, "Unexpected stream_flags %lu\n", callback->stream_flags); + ok(callback->timestamp == 0, "Unexpected timestamp\n"); + ok(callback->hr == MF_E_INVALIDREQUEST, "Unexpected hr %#lx\n", callback->hr); + } + + /* Second time fails immediately, not asynchronously. */ + hr = IMFSourceReader_ReadSample(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, 0, NULL, NULL, NULL, NULL); + todo_wine ok(hr == MF_E_INVALIDREQUEST, "Unexpected hr %#lx.\n", hr); + + IMFSourceReader_Release(reader); + IMFMediaSource_Release(source); + + source = create_test_source(&video_stream, 1); + ok(!!source, "Failed to create test source.\n"); + IMFStreamDescriptor_Release(video_stream); + test_source = impl_from_IMFMediaSource(source); + + hr = MFCreateSourceReaderFromMediaSource(source, attributes, &reader); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFSourceReader_SetCurrentMediaType(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, NULL, media_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + IMFAttributes_Release(attributes); + IMFMediaType_Release(media_type); + + hr = IMFSourceReader_SetStreamSelection(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, TRUE); + ok(hr == S_OK, "Unexpected hr %#lx\n", hr); + + /* Race ReadSample with SetCurrentPosition. + * Trying to figure out if a SetCurrentPosition will be rejected while + * a ReadSample is in flight. */ + test_source->streams[0]->manual_samples = TRUE; + hr = IMFSourceReader_ReadSample(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, 0, NULL, NULL, NULL, NULL); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + while (!test_source->streams[0]->request_sample_called) + Sleep(0); + + var.vt = VT_I8; + var.hVal.QuadPart = 0; + hr = IMFSourceReader_SetCurrentPosition(reader, &GUID_NULL, &var); + ok(hr == MF_E_INVALIDREQUEST, "Unexpected hr %#lx.\n", hr); + + test_media_stream_handle_request_samples(test_source->streams[0]); + + res = WaitForSingleObject(callback->event, 1000); + ok(res == WAIT_OBJECT_0, "ReadSample didn't finish\n"); + + /* The other order. */ + + /* A hypothetical scenario. Since ReadSample and SetCurrentPosition happens asynchronously, + * it's perceivable that the media stream RequestSample job created by the later ReadSample call + * _may_ be added to queue prior to the SetCurrentPosition triggers a media source Start. This + * media source Start may cause the RequestSample job to be dropped, and the source reader + * should handle this correctly and not hang. + * + * The source reader implementation can handle this in one of at least two ways. It can either + * make sure this ordering never happens, e.g. by synchronously set a flag of some sort in + * SetCurrentPosition, then wait for the completion of pending seek in ReadSample; or it should + * react to the RequestSample job being dropped. + */ + + for (i = 0; i < 100; i++) + { + test_source->start_called = FALSE; + test_source->streams[0]->request_sample_called = FALSE; + + hr = IMFSourceReader_SetCurrentPosition(reader, &GUID_NULL, &var); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFSourceReader_ReadSample(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, 0, NULL, NULL, NULL, NULL); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + while (!test_source->start_called) + Sleep(0); + while (!test_source->streams[0]->request_sample_called) + Sleep(0); + + test_media_stream_handle_request_samples(test_source->streams[0]); + + res = WaitForSingleObject(callback->event, 200); + if (res) break; + } + + todo_wine ok(!res, "ReadSample didn't finish\n"); + + IMFSourceReaderCallback_Release(&callback->IMFSourceReaderCallback_iface); + IMFSourceReader_Release(reader); + IMFMediaSource_Release(source); +} + static void test_source_reader_from_media_source(void) { static const DWORD expected_sample_order[10] = {0, 0, 1, 1, 0, 0, 0, 0, 1, 0}; @@ -3902,6 +4111,7 @@ START_TEST(mfplat) test_source_reader("test.wav", false); test_source_reader("test.mp4", true); test_source_reader_from_media_source(); + test_async_source_reader(); test_source_reader_transforms(FALSE, FALSE); test_source_reader_transforms(TRUE, FALSE); test_source_reader_transforms(FALSE, TRUE); @@ -3914,7 +4124,6 @@ START_TEST(mfplat) test_sink_writer_get_object(); test_sink_writer_add_stream(); test_sink_writer_sample_process(); - hr = MFShutdown(); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11174
From: Yuxuan Shui <yshui@codeweavers.com> Previously the SEEKING flag is only set when the ASYNC_SEEK operation is executed. This is too late because of this race condition: - Application calls SetCurrentPosition, which queues the ASYNC_SEEK operation but does not set SEEKING. - Application calls ReadSample. Since the SEEKING flag hasn't been set, it proceeds without waiting and queues an ASYNC_READ operation. - ASYNC_SEEK operation starts its execution, but hasn't reached IMFMediaSource_Start yet. - ASYNC_READ operation starts its execution, and calls IMFMediaStream_RequestSample. Media source queues a request sample job. reader->streams[i]->requests++. ASYNC_READ finishes. - ASYNC_SEEK operation calls IMFMediaSource_Start, the request sample job queued by the media source gets dropped. At this point, there is no request sample job running, but the stream->requests counter is still positive. From this point on the source reader will not request anymore samples, future ReadSample will never complete. Setting the SEEKING flag early, then source reader will always wait in ReadSample so this kind of ordering cannot happen. --- dlls/mfreadwrite/reader.c | 7 ++++--- dlls/mfreadwrite/tests/mfplat.c | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/mfreadwrite/reader.c b/dlls/mfreadwrite/reader.c index fdbc40fc2ba..30fef71e26b 100644 --- a/dlls/mfreadwrite/reader.c +++ b/dlls/mfreadwrite/reader.c @@ -1562,10 +1562,10 @@ static HRESULT WINAPI source_reader_async_commands_callback_Invoke(IMFAsyncCallb case SOURCE_READER_ASYNC_SEEK: EnterCriticalSection(&reader->cs); - if (SUCCEEDED(IMFMediaSource_Start(reader->source, reader->descriptor, &command->u.seek.format, + if (FAILED(IMFMediaSource_Start(reader->source, reader->descriptor, &command->u.seek.format, &command->u.seek.position))) { - reader->flags |= SOURCE_READER_SEEKING; + reader->flags &= ~SOURCE_READER_SEEKING; } LeaveCriticalSection(&reader->cs); @@ -2358,6 +2358,8 @@ static HRESULT WINAPI src_reader_SetCurrentPosition(IMFSourceReaderEx *iface, RE if (SUCCEEDED(hr)) { + reader->flags |= SOURCE_READER_SEEKING; + for (i = 0; i < reader->stream_count; ++i) { reader->streams[i].last_sample_ts = 0; @@ -2378,7 +2380,6 @@ static HRESULT WINAPI src_reader_SetCurrentPosition(IMFSourceReaderEx *iface, RE { if (SUCCEEDED(IMFMediaSource_Start(reader->source, reader->descriptor, format, position))) { - reader->flags |= SOURCE_READER_SEEKING; while (reader->flags & SOURCE_READER_SEEKING) { SleepConditionVariableCS(&reader->state_event, &reader->cs, INFINITE); diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 763aecc6908..2cbc946847c 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -1356,7 +1356,7 @@ static void test_async_source_reader(void) if (res) break; } - todo_wine ok(!res, "ReadSample didn't finish\n"); + ok(!res, "ReadSample didn't finish\n"); IMFSourceReaderCallback_Release(&callback->IMFSourceReaderCallback_iface); IMFSourceReader_Release(reader); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11174
I think what really matters here is that there is no attempt to preserve command order, but it's not clear if that happens in all cases. For example we don't need to serialize read requests for different streams. Regarding the test, I agree with @rbernon, it looks large and fragile. I suggest testing (with external program for example) other methods when seeking never completes, by using some source stub that never finishes seeking. I suspect we need to keep track of read requests until seek is finished, because there is no way for the user to know when that happens. That's where it differs from OnFlush(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11174#note_144629
Regarding the test, I agree with @rbernon, it looks large and fragile.
There is no way to deterministically trigger the bug this MR is fixing. I acknowledge that this might not be something we want to test for anyway. So I need some suggestion about what kind of tests I should write instead?
... by using some source stub that never finishes seeking. I suspect we need to keep track of read requests until seek is finished.
I don't think we necessarily do, if read requests are never queued until seeking has finished. (like we do right now, i.e. setting the SEEKING flag, then in `ReadSample` loop sleeping until the flag is cleared). Primarily testing shows this is what native does as well (if seeking never finishes, `ReadSample` will block). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11174#note_144633
On Wed Jul 1 19:02:03 2026 +0000, Yuxuan Shui wrote:
Regarding the test, I agree with @rbernon, it looks large and fragile. There is no way to deterministically trigger the bug this MR is fixing. I acknowledge that this might not be something we want to test for anyway. So I need some suggestion about what kind of tests I should write instead? ... by using some source stub that never finishes seeking. I suspect we need to keep track of read requests until seek is finished. I don't think we necessarily do, if read requests are never queued until seeking has finished. (like we do right now, i.e. setting the SEEKING flag, then in `ReadSample` loop sleeping until the flag is cleared). Primarily testing shows this is what native does as well (if seeking never finishes, `ReadSample` will block). ping @nsivov. what's your thoughts on how to write testcases for this?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11174#note_147115
On Wed Jul 29 12:25:51 2026 +0000, Yuxuan Shui wrote:
ping @nsivov. what's your thoughts on how to write testcases for this? Regarding tests, it's enough to have some adhoc patch to wine tests that demonstrates what's happening. It's fine to keep it attached here, we don't need it committed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11174#note_148746
On Wed Aug 12 13:28:48 2026 +0000, Nikolay Sivov wrote:
Regarding tests, it's enough to have some adhoc patch to wine tests that demonstrates what's happening. It's fine to keep it attached here, we don't need it committed. So what about commands order? Do we need something to preserve it, and will it solve this issue too? Something like queuing commands, maybe conditionally, and waiting for completion.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11174#note_148747
participants (3)
-
Nikolay Sivov (＠nsivov) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)