[PATCH v5 0/1] MR9858: qasf: Fix use-after-free of filter in reader callback.
If a asf reader callback is invoked after its corresponding reader has been destroyed, it will still try to use the reader via asf_callback->filter, resulting in use-after-free. This commits sets asf_callback->filter to NULL when the reader is destroyed, and adds NULL checks to reader callback functions to make them no-op after the reader is destroyed. Fixes: Bug 59159 -- v5: qasf: Don't start a stopped stream in media_seeking_ChangeCurrent. https://gitlab.winehq.org/wine/wine/-/merge_requests/9858
From: Yuxuan Shui <yshui@codeweavers.com> Otherwise we get a running WMReader inside a stoppped asf reader, which is unexpected by other parts of the code. For example, asf_reader_destroy expects the WMReader to have already been stopped. If it is not, use of freed memory may result from race condition between the WMReader invoking reader callback (which references the asf_reader) and asf_reader_destroy freeing asf_reader's resources. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59159 --- dlls/qasf/asfreader.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index ec7a0310e0a..ed17a93981a 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -327,7 +327,8 @@ static HRESULT WINAPI media_seeking_ChangeCurrent(IMediaSeeking *iface) struct asf_stream *stream = impl_from_IMediaSeeking(iface); struct asf_reader *filter = asf_reader_from_asf_stream(stream); struct SourceSeeking *seek = &stream->seek; - HRESULT hr; + BOOL should_start_stream = FALSE; + HRESULT hr = S_OK; UINT i; TRACE("iface %p.\n", iface); @@ -340,7 +341,11 @@ static HRESULT WINAPI media_seeking_ChangeCurrent(IMediaSeeking *iface) } /* Stop the reader. */ - hr = asf_reader_stop_stream(filter); + if (filter->status == WMT_STARTED) + { + hr = asf_reader_stop_stream(filter); + should_start_stream = hr == S_OK; + } /* Send end flush commands downstream. */ for (i = 0; i < filter->stream_count; ++i) @@ -349,8 +354,8 @@ static HRESULT WINAPI media_seeking_ChangeCurrent(IMediaSeeking *iface) WARN("Failed to EndFlush for stream %u.\n", i); } - /* Start the reader. */ - if (hr == S_OK) + /* Start the reader if we stopped it. */ + if (should_start_stream) hr = asf_reader_start_stream(filter, seek->llCurrent, seek->llDuration, seek->dRate); return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9858
``` - hr = asf_reader_stop_stream(filter); + if (filter->status == WMT_STARTED) + { + hr = asf_reader_stop_stream(filter); + should_start_stream = hr == S_OK; + } ``` If we failed to stop the stream shouldn't we just return failure? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126654
On Mon Jan 12 19:55:37 2026 +0000, Elizabeth Figura wrote:
``` - hr = asf_reader_stop_stream(filter); + if (filter->status == WMT_STARTED) + { + hr = asf_reader_stop_stream(filter); + should_start_stream = hr == S_OK; + } ``` If we failed to stop the stream shouldn't we just return failure?
i was trying to preserve the existing behavior, and i don't have enough context to know if early returning is appropriate. i can change it if you think that's what we should do. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126657
On Mon Jan 12 19:55:37 2026 +0000, Yuxuan Shui wrote:
i was trying to preserve the existing behavior, and i don't have enough context to know if early returning is appropriate. i can change it if you think that's what we should do. I don't think there's any realistic scenario where stopping the stream can fail. I wouldn't even necessarily handle it, honestly, but we do elsewhere in this file.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126668
participants (3)
-
Elizabeth Figura (＠zfigura) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)