[PATCH v2 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 -- v2: qasf: Fix use-after-free of asf reader in reader callback. https://gitlab.winehq.org/wine/wine/-/merge_requests/9858
From: Yuxuan Shui <yshui@codeweavers.com> 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 --- dlls/qasf/asfreader.c | 45 ++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index ec7a0310e0a..67ef13ba2ad 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -182,6 +182,20 @@ struct asf_reader struct asf_stream streams[16]; }; +struct asf_callback +{ + IWMReaderCallback IWMReaderCallback_iface; + IWMReaderCallbackAdvanced IWMReaderCallbackAdvanced_iface; + LONG ref; + + struct asf_reader *filter; +}; + +static inline struct asf_callback *impl_from_IWMReaderCallback(IWMReaderCallback *iface) +{ + return CONTAINING_RECORD(iface, struct asf_callback, IWMReaderCallback_iface); +} + static inline struct asf_stream *impl_from_strmbase_pin(struct strmbase_pin *iface) { return CONTAINING_RECORD(iface, struct asf_stream, source.pin); @@ -442,6 +456,7 @@ static struct strmbase_pin *asf_reader_get_pin(struct strmbase_filter *iface, un static void asf_reader_destroy(struct strmbase_filter *iface) { struct asf_reader *filter = impl_from_strmbase_filter(iface); + struct asf_callback *callback = impl_from_IWMReaderCallback(filter->callback); struct strmbase_source *source; while (filter->stream_count--) @@ -453,6 +468,7 @@ static void asf_reader_destroy(struct strmbase_filter *iface) } free(filter->file_name); + callback->filter = NULL; IWMReaderCallback_Release(filter->callback); IWMReader_Release(filter->reader); @@ -730,20 +746,6 @@ static const IFileSourceFilterVtbl file_source_vtbl = file_source_GetCurFile, }; -struct asf_callback -{ - IWMReaderCallback IWMReaderCallback_iface; - IWMReaderCallbackAdvanced IWMReaderCallbackAdvanced_iface; - LONG ref; - - struct asf_reader *filter; -}; - -static inline struct asf_callback *impl_from_IWMReaderCallback(IWMReaderCallback *iface) -{ - return CONTAINING_RECORD(iface, struct asf_callback, IWMReaderCallback_iface); -} - static HRESULT WINAPI reader_callback_QueryInterface(IWMReaderCallback *iface, const IID *iid, void **out) { struct asf_callback *callback = impl_from_IWMReaderCallback(iface); @@ -804,6 +806,9 @@ static HRESULT WINAPI reader_callback_OnStatus(IWMReaderCallback *iface, WMT_STA TRACE("iface %p, status %d, result %#lx, type %d, value %p, context %p.\n", iface, status, result, type, value, context); + if (!filter) + return S_OK; + switch (status) { case WMT_OPENED: @@ -902,13 +907,17 @@ static HRESULT WINAPI reader_callback_OnSample(IWMReaderCallback *iface, DWORD o { struct asf_reader *filter = impl_from_IWMReaderCallback(iface)->filter; REFERENCE_TIME start_time = time, end_time = time + duration; - struct asf_stream *stream = filter->streams + output; + struct asf_stream *stream; struct buffer *buffer; HRESULT hr = S_OK; TRACE("iface %p, output %lu, time %I64u, duration %I64u, flags %#lx, sample %p, context %p.\n", iface, output, time, duration, flags, sample, context); + if (!filter) + return S_OK; + + stream = filter->streams + output; if (!stream->source.pin.peer) { WARN("Output %lu pin is not connected, discarding %p.\n", output, sample); @@ -1003,7 +1012,7 @@ static HRESULT WINAPI reader_callback_advanced_AllocateForOutput(IWMReaderCallba DWORD output, DWORD size, INSSBuffer **out, void *context) { struct asf_reader *filter = impl_from_IWMReaderCallbackAdvanced(iface)->filter; - struct asf_stream *stream = filter->streams + output; + struct asf_stream *stream; IMediaSample *sample; HRESULT hr; @@ -1011,6 +1020,10 @@ static HRESULT WINAPI reader_callback_advanced_AllocateForOutput(IWMReaderCallba *out = NULL; + if (!filter) + return VFW_E_NOT_CONNECTED; + + stream = filter->streams + output; if (!stream->source.pin.peer) return VFW_E_NOT_CONNECTED; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9858
This bug causes Bug 59159 because filter graph destruction happens in parallel with wm async reader reads, in separate threads. on one side the asf reader is destroyed in filter graph destruction, which (in another thread) triggers allocation failure in the wm reader (because the allocator has just been decommitted), which returns NO_MORE_SAMPLES to the wm _async_ reader's read thread, which in turn calls the asf reader's reader callback, which uses the already freed asf reader. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126458
``` qasf: Fix use-after-free of asf reader 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. ``` It doesn't sound right that wmvcore is invoking callbacks after (or while) the wmv object is destroyed. Are we sure that's supposed to happen? ``` Fixes: Bug 59159 ``` That's not how we write that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126476
It doesn't sound right that wmvcore is invoking callbacks after (or while) the wmv object is destroyed. Are we sure that's supposed to happen?
Oh wait, do you think the destruction of IWMReader should happen first? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9858#note_126480
participants (3)
-
Elizabeth Figura (＠zfigura) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)