[PATCH v4 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 -- v4: qasf: Stop the WMReader first in asf_reader_destroy. https://gitlab.winehq.org/wine/wine/-/merge_requests/9858
From: Yuxuan Shui <yshui@codeweavers.com> Otherwise it may invoke the reader callback of the asf_reader as we are releasing the reader's resources, which the reader callback may use, resulting in use-after-frees. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59159 --- dlls/qasf/asfreader.c | 3 ++- 1 file changed, 2 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9858
On Wed Jan 7 01:20:16 2026 +0000, Yuxuan Shui wrote:
well, that makes a lot more sense. thanks. That's not actually what I meant, no. I mean it sounds like wmvcore is misbehaving here. Are we sure it should be sending callbacks after the IWMReader is destroyed? In fact, shouldn't it stop sending callbacks after IWMReader::Stop()?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126504
On Wed Jan 7 17:52:01 2026 +0000, Elizabeth Figura wrote:
That's not actually what I meant, no. I mean it sounds like wmvcore is misbehaving here. Are we sure it should be sending callbacks after the IWMReader is destroyed? In fact, shouldn't it stop sending callbacks after IWMReader::Stop()? well, `WMReader_Stop` only queues the stop op, doesn't wait for it to complete. there is also no mechanism for waiting for op completion there.
and after reader is stopped there is at least one more possible status (`WMT_CLOSED`), which the callback may be invoked with. (on that thought, maybe `asf_reader_cleanup_stream` should call WMReader_Close instead of just Stop). do you suggest we add a event so we can wait for the completion of at least the close op? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126505
well, `WMReader_Stop` only queues the stop op, doesn't wait for it to complete. there is also no mechanism for waiting for op completion there.
Sorry, that was imprecise, not the Stop() per se indeed but we do wait for WMT_STOPPED immediately after in the same function.
and after reader is stopped there is at least one more possible status (`WMT_CLOSED`), which the callback may be invoked with. (on that thought, maybe `asf_reader_cleanup_stream` should call WMReader_Close instead of just Stop).
No, we don't want to Close() in cleanup_stream(), because the stream can be started again with the same file. But why is WMT_CLOSED a problem? We don't do anything in response to it.
do you suggest we add a event so we can wait for the completion of at least the close op?
Do we actually need to? Does native wmvcore really send WMT_CLOSED *after* the last reference to the IWMReader is released? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126509
On Wed Jan 7 18:58:55 2026 +0000, Elizabeth Figura wrote:
well, `WMReader_Stop` only queues the stop op, doesn't wait for it to complete. there is also no mechanism for waiting for op completion there. Sorry, that was imprecise, not the Stop() per se indeed but we do wait for WMT_STOPPED immediately after in the same function. and after reader is stopped there is at least one more possible status (`WMT_CLOSED`), which the callback may be invoked with. (on that thought, maybe `asf_reader_cleanup_stream` should call WMReader_Close instead of just Stop). No, we don't want to Close() in cleanup_stream(), because the stream can be started again with the same file. But why is WMT_CLOSED a problem? We don't do anything in response to it. do you suggest we add a event so we can wait for the completion of at least the close op? Do we actually need to? Does native wmvcore really send WMT_CLOSED *after* the last reference to the IWMReader is released? oh, ok. i get what you meant now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126552
On Thu Jan 8 13:53:31 2026 +0000, Yuxuan Shui wrote:
oh, ok. i get what you meant now. @zfigura so it goes like this:
`MediaFilter_Stop` calls `filter_Stop`, which calls `asf_reader_cleanup_stream`, which stops the WMReader (which _did_ stop), and decommits the allocator. As expected. But the problem is later in `MediaFilter_Stop`, it calls `IMediaSeeking::SetPositions`, which calls `media_seeking_ChangeCurrent` (`asfreader.c, line 329`), which stops then starts the stream again. This means when we finally get to `asf_reader_destroy`, the WMReader is running, and with a decommitted allocator. Thoughts? maybe `MediaFilter_Stop` shouldn't call `SetPositions`? Or `SetPositions` shouldn't restart the reader if it's stopped? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126591
On Fri Jan 9 23:44:50 2026 +0000, Yuxuan Shui wrote:
@zfigura so it goes like this: `MediaFilter_Stop` calls `filter_Stop`, which calls `asf_reader_cleanup_stream`, which stops the WMReader (which _did_ stop), and decommits the allocator. As expected. But the problem is later in `MediaFilter_Stop`, it calls `IMediaSeeking::SetPositions`, which calls `media_seeking_ChangeCurrent` (`asfreader.c, line 329`), which stops then starts the stream again. This means when we finally get to `asf_reader_destroy`, the WMReader is running, and with a decommitted allocator. Thoughts? maybe `MediaFilter_Stop` shouldn't call `SetPositions`? Or `SetPositions` shouldn't restart the reader if it's stopped? well, let me just write a test case to check if `SetPositions` is supposed to start a stopped filter.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126592
On Fri Jan 9 23:54:07 2026 +0000, Yuxuan Shui wrote:
well, let me just write a test case to check if `SetPositions` is supposed to start a stopped filter. Oh right, I can't. because the state of the WMReader is internal to the asf reader, so I won't be able to get it with `IBaseFilter_GetState`. `IBaseFilter_GetState` returns a state that is managed by strmbase, which is not touched by `IMediaSeeking` operations.
But then I think it's justifiable to say `media_seeking_ChangeCurrent` should not restart the stream if it's stopped. Because in that case we have a stopped filter encapsulating a running WMReader, which would be wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126593
participants (3)
-
Elizabeth Figura (＠zfigura) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)