[PATCH 0/1] MR9871: qasf: Stop the WMReader first in asf_reader_destroy.
Otherwise WMReader may invoke the reader callback of the asf_reader as we are releasing the its resources, which the reader callback may use, resulting in use-after-frees. * * * Found while working on !9858. Although not the root cause of Bug 59159, it is still a bug. Without this, asf reader segfaults in `asf_reader_destroy` if it is not stopped first before being released. [Testing](https://testbot.winehq.org/JobDetails.pl?Key=161453) on native shows there were no such crashes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9871
From: Yuxuan Shui <yshui@codeweavers.com> Otherwise WMReader may invoke the reader callback of the asf_reader as we are releasing the its resources, which the reader callback may use, resulting in use-after-frees. --- dlls/qasf/asfreader.c | 3 ++- dlls/qasf/tests/asfreader.c | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index ec7a0310e0a..18e01e06acf 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -444,6 +444,8 @@ static void asf_reader_destroy(struct strmbase_filter *iface) struct asf_reader *filter = impl_from_strmbase_filter(iface); struct strmbase_source *source; + IWMReader_Release(filter->reader); + while (filter->stream_count--) { source = &filter->streams[filter->stream_count].source; @@ -454,7 +456,6 @@ static void asf_reader_destroy(struct strmbase_filter *iface) free(filter->file_name); IWMReaderCallback_Release(filter->callback); - IWMReader_Release(filter->reader); strmbase_filter_cleanup(&filter->filter); diff --git a/dlls/qasf/tests/asfreader.c b/dlls/qasf/tests/asfreader.c index 62a08f6b78c..700e4ab2354 100644 --- a/dlls/qasf/tests/asfreader.c +++ b/dlls/qasf/tests/asfreader.c @@ -638,6 +638,31 @@ static void test_filter_state(void) ref = IBaseFilter_Release(filter); ok(!ref, "Got ref %ld.\n", ref); + + /* What happens if we release a running asf reader? */ + filter = create_asf_reader(); + hr = IBaseFilter_QueryInterface(filter, &IID_IFileSourceFilter, (void **)&file_source); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + hr = IFileSourceFilter_Load(file_source, filename, NULL); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + IFileSourceFilter_Release(file_source); + + hr = CoCreateInstance(&CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER, + &IID_IFilterGraph, (void **)&graph); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + hr = IFilterGraph_AddFilter(graph, filter, NULL); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + ref = IFilterGraph_Release(graph); + ok(!ref, "Got ref %ld.\n", ref); + + hr = IBaseFilter_Run(filter, GetTickCount() * 10000); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + hr = IBaseFilter_GetState(filter, 0, &state); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + ok(state == State_Running, "Got state %#x.\n", state); + + ref = IBaseFilter_Release(filter); + ok(!ref, "Got ref %ld.\n", ref); } struct test_sink -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9871
I still don't understand this explanation, I'm afraid. What are we trying to access that has been freed, and how do we know this is the right way to deal with it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9871#note_126655
On Mon Jan 12 20:22:34 2026 +0000, Elizabeth Figura wrote:
I still don't understand this explanation, I'm afraid. What are we trying to access that has been freed, and how do we know this is the right way to deal with it? So what happens is:
1. we get to asf_reader_destroy, and the WMReader is still running. 2. since WMReader is still running, it may invoke `IWMReaderCallback::OnStatus`, which is `reader_callback_OnStatus`. 3. we release the `IWMReaderCallback` object before the WMReader. 4. WMReader calls `reader_callback_OnStatus` and segfaults. In this specific test case, the asf reader is release very quickly, the background read thread hasn't completed its first `GetNextSample`. That `GetNextSample` call returns NO_MORE_SAMPLES because no streams were enabled in this test. The read thread got NO_MORE_SAMPLES and thus invokes the `OnStatus` callback which is how we got to step (4). This MR makes sure the WMReader is released first, which means read thread is stopped and no more callbacks can happen. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9871#note_126659
On Mon Jan 12 20:22:34 2026 +0000, Yuxuan Shui wrote:
So what happens is: 1. we get to asf_reader_destroy, and the WMReader is still running. 2. since WMReader is still running, it may invoke `IWMReaderCallback::OnStatus`, which is `reader_callback_OnStatus`. 3. we release the `IWMReaderCallback` object before the WMReader. 4. WMReader calls `reader_callback_OnStatus` and segfaults. In this specific test case, the asf reader is release very quickly, the background read thread hasn't completed its first `GetNextSample`. That `GetNextSample` call returns NO_MORE_SAMPLES because no streams were enabled in this test. The read thread got NO_MORE_SAMPLES and thus invokes the `OnStatus` callback which is how we got to step (4). This MR makes sure the WMReader is released first, which means read thread is stopped and no more callbacks can happen. Right okay, I forgot we rely on Release() to tear down everything if the filter is still running.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9871#note_128066
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9871
participants (3)
-
Elizabeth Figura (＠zfigura) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)