[PATCH 0/1] MR8712: strmbase: Get rid of stream_cs.
The following sequence can occur: sink_EndOfStream, lock stream_cs, renderer sink_eos, strmbase_passthrough_eos, IMediaSeeking_GetStopPosition, MediaSeekingPassThru_GetStopPosition, get_connected, IPin_ConnectedTo, lock filter_cs And at the same time: filter_Stop, lock filter_cs, dmo_wrapper_cleanup_stream, lock stream_cs Resulting in deadlock. This can occur in Space Engineers. dmo_wrapper_cleanup_stream() locks stream_cs to access `pin.peer` and `pin.pAllocator`, which are normally protected by filter_cs, but this lock was added to fix a test failure, so I don't know if there's an alternative. I'm concerned about the possible performance impact of removing stream_cs though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8712
From: Conor McCarthy <cmccarthy(a)codeweavers.com> The following sequence can occur: sink_EndOfStream lock stream_cs renderer sink_eos strmbase_passthrough_eos IMediaSeeking_GetStopPosition MediaSeekingPassThru_GetStopPosition get_connected IPin_ConnectedTo lock filter_cs And at the same time: filter_Stop lock filter_cs dmo_wrapper_cleanup_stream lock stream_cs Resulting in deadlock. --- dlls/qasf/dmowrapper.c | 4 ++-- dlls/quartz/avidec.c | 4 ++-- dlls/quartz/dsoundrender.c | 6 +++--- dlls/quartz/videorenderer.c | 12 ++++++------ dlls/quartz/vmr9.c | 6 +++--- dlls/winegstreamer/quartz_transform.c | 4 ++-- include/wine/strmbase.h | 1 - libs/strmbase/filter.c | 7 ------- libs/strmbase/pin.c | 8 ++++---- libs/strmbase/renderer.c | 8 ++++---- 10 files changed, 26 insertions(+), 34 deletions(-) diff --git a/dlls/qasf/dmowrapper.c b/dlls/qasf/dmowrapper.c index f6e586d0235..75047ab9548 100644 --- a/dlls/qasf/dmowrapper.c +++ b/dlls/qasf/dmowrapper.c @@ -750,7 +750,7 @@ static HRESULT dmo_wrapper_cleanup_stream(struct strmbase_filter *iface) IUnknown_QueryInterface(filter->dmo, &IID_IMediaObject, (void **)&dmo); - EnterCriticalSection(&filter->filter.stream_cs); + EnterCriticalSection(&filter->filter.filter_cs); for (i = 0; i < filter->source_count; ++i) { if (filter->sources[i].pin.pin.peer) @@ -761,7 +761,7 @@ static HRESULT dmo_wrapper_cleanup_stream(struct strmbase_filter *iface) IMediaObject_FreeStreamingResources(dmo); IMediaObject_Release(dmo); - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); return S_OK; } diff --git a/dlls/quartz/avidec.c b/dlls/quartz/avidec.c index 9f1a7db1586..a1bfb35861f 100644 --- a/dlls/quartz/avidec.c +++ b/dlls/quartz/avidec.c @@ -653,9 +653,9 @@ static HRESULT avi_decompressor_cleanup_stream(struct strmbase_filter *iface) if (filter->hvid) { - EnterCriticalSection(&filter->filter.stream_cs); + EnterCriticalSection(&filter->filter.filter_cs); res = ICDecompressEnd(filter->hvid); - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); if (res) { ERR("ICDecompressEnd() failed, error %Id.\n", res); diff --git a/dlls/quartz/dsoundrender.c b/dlls/quartz/dsoundrender.c index 59609a3c849..9713ace16c9 100644 --- a/dlls/quartz/dsoundrender.c +++ b/dlls/quartz/dsoundrender.c @@ -543,7 +543,7 @@ static HRESULT dsound_render_sink_end_flush(struct strmbase_sink *iface) { struct dsound_render *filter = impl_from_strmbase_pin(&iface->pin); - EnterCriticalSection(&filter->filter.stream_cs); + EnterCriticalSection(&filter->filter.filter_cs); if (filter->eos && filter->filter.state != State_Stopped) { WaitForSingleObject(filter->render_thread, INFINITE); @@ -551,7 +551,7 @@ static HRESULT dsound_render_sink_end_flush(struct strmbase_sink *iface) if (!(filter->render_thread = CreateThread(NULL, 0, render_thread_run, filter, 0, NULL))) { - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); return HRESULT_FROM_WIN32(GetLastError()); } filter->eos = FALSE; @@ -571,7 +571,7 @@ static HRESULT dsound_render_sink_end_flush(struct strmbase_sink *iface) filter->writepos = filter->buf_size; } - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); return S_OK; } diff --git a/dlls/quartz/videorenderer.c b/dlls/quartz/videorenderer.c index bbd34678dfd..7d09046101c 100644 --- a/dlls/quartz/videorenderer.c +++ b/dlls/quartz/videorenderer.c @@ -225,33 +225,33 @@ static HRESULT video_renderer_get_current_image(struct video_window *iface, LONG size_t image_size; BYTE *sample_data; - EnterCriticalSection(&filter->renderer.filter.stream_cs); + EnterCriticalSection(&filter->renderer.filter.filter_cs); bih = get_bitmap_header(&filter->renderer.sink.pin.mt); image_size = bih->biWidth * bih->biHeight * bih->biBitCount / 8; if (!image) { - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); *size = sizeof(BITMAPINFOHEADER) + image_size; return S_OK; } if (filter->renderer.filter.state != State_Paused) { - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); return VFW_E_NOT_PAUSED; } if (!filter->renderer.current_sample) { - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); return E_UNEXPECTED; } if (*size < sizeof(BITMAPINFOHEADER) + image_size) { - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); return E_OUTOFMEMORY; } @@ -259,7 +259,7 @@ static HRESULT video_renderer_get_current_image(struct video_window *iface, LONG IMediaSample_GetPointer(filter->renderer.current_sample, &sample_data); memcpy((char *)image + sizeof(BITMAPINFOHEADER), sample_data, image_size); - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); return S_OK; } diff --git a/dlls/quartz/vmr9.c b/dlls/quartz/vmr9.c index 611a1256b0e..d75b7085602 100644 --- a/dlls/quartz/vmr9.c +++ b/dlls/quartz/vmr9.c @@ -635,7 +635,7 @@ static HRESULT vmr_get_current_image(struct video_window *iface, LONG *size, LON char *dst; HRESULT hr; - EnterCriticalSection(&filter->renderer.filter.stream_cs); + EnterCriticalSection(&filter->renderer.filter.filter_cs); device = filter->allocator_d3d9_dev; bih = *get_filter_bitmap_header(filter); @@ -644,7 +644,7 @@ static HRESULT vmr_get_current_image(struct video_window *iface, LONG *size, LON if (!image) { *size = sizeof(BITMAPINFOHEADER) + bih.biSizeImage; - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); return S_OK; } @@ -680,7 +680,7 @@ static HRESULT vmr_get_current_image(struct video_window *iface, LONG *size, LON out: if (surface) IDirect3DSurface9_Release(surface); if (rt) IDirect3DSurface9_Release(rt); - LeaveCriticalSection(&filter->renderer.filter.stream_cs); + LeaveCriticalSection(&filter->renderer.filter.filter_cs); return hr; } diff --git a/dlls/winegstreamer/quartz_transform.c b/dlls/winegstreamer/quartz_transform.c index ff57ea02ed0..69b84c66a49 100644 --- a/dlls/winegstreamer/quartz_transform.c +++ b/dlls/winegstreamer/quartz_transform.c @@ -129,10 +129,10 @@ static HRESULT transform_cleanup_stream(struct strmbase_filter *iface) { IMemAllocator_Decommit(filter->source.pAllocator); - EnterCriticalSection(&filter->filter.stream_cs); + EnterCriticalSection(&filter->filter.filter_cs); wg_transform_destroy(filter->transform); wg_sample_queue_destroy(filter->sample_queue); - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); } return S_OK; diff --git a/include/wine/strmbase.h b/include/wine/strmbase.h index 978d1c67160..b44277ed25d 100644 --- a/include/wine/strmbase.h +++ b/include/wine/strmbase.h @@ -146,7 +146,6 @@ struct strmbase_filter IUnknown *outer_unk; LONG refcount; CRITICAL_SECTION filter_cs; - CRITICAL_SECTION stream_cs; FILTER_STATE state; IReferenceClock *clock; diff --git a/libs/strmbase/filter.c b/libs/strmbase/filter.c index 3229c7ba82a..fe6f3cd7215 100644 --- a/libs/strmbase/filter.c +++ b/libs/strmbase/filter.c @@ -528,10 +528,6 @@ void strmbase_filter_init(struct strmbase_filter *filter, IUnknown *outer, InitializeCriticalSection(&filter->filter_cs); if (filter->filter_cs.DebugInfo != (RTL_CRITICAL_SECTION_DEBUG *)-1) filter->filter_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": strmbase_filter.filter_cs"); - if (!InitializeCriticalSectionEx(&filter->stream_cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO)) - InitializeCriticalSection(&filter->stream_cs); - if (filter->stream_cs.DebugInfo != (RTL_CRITICAL_SECTION_DEBUG *)-1) - filter->stream_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": strmbase_filter.stream_cs"); filter->clsid = *clsid; filter->pin_version = 1; filter->ops = ops; @@ -546,7 +542,4 @@ void strmbase_filter_cleanup(struct strmbase_filter *filter) if (filter->filter_cs.DebugInfo != (RTL_CRITICAL_SECTION_DEBUG *)-1) filter->filter_cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&filter->filter_cs); - if (filter->stream_cs.DebugInfo != (RTL_CRITICAL_SECTION_DEBUG *)-1) - filter->stream_cs.DebugInfo->Spare[0] = 0; - DeleteCriticalSection(&filter->stream_cs); } diff --git a/libs/strmbase/pin.c b/libs/strmbase/pin.c index 03330837477..b47b836c872 100644 --- a/libs/strmbase/pin.c +++ b/libs/strmbase/pin.c @@ -909,9 +909,9 @@ static HRESULT WINAPI sink_EndOfStream(IPin *iface) if (pin->pFuncsTable->sink_eos) { - EnterCriticalSection(&pin->pin.filter->stream_cs); + EnterCriticalSection(&pin->pin.filter->filter_cs); hr = pin->pFuncsTable->sink_eos(pin); - LeaveCriticalSection(&pin->pin.filter->stream_cs); + LeaveCriticalSection(&pin->pin.filter->filter_cs); return hr; } @@ -1122,9 +1122,9 @@ static HRESULT WINAPI MemInputPin_Receive(IMemInputPin *iface, IMediaSample *sam if (pin->pFuncsTable->pfnReceive) { - EnterCriticalSection(&pin->pin.filter->stream_cs); + EnterCriticalSection(&pin->pin.filter->filter_cs); hr = pin->pFuncsTable->pfnReceive(pin, sample); - LeaveCriticalSection(&pin->pin.filter->stream_cs); + LeaveCriticalSection(&pin->pin.filter->filter_cs); } return hr; } diff --git a/libs/strmbase/renderer.c b/libs/strmbase/renderer.c index dc421f499ed..66d0e5cc7af 100644 --- a/libs/strmbase/renderer.c +++ b/libs/strmbase/renderer.c @@ -373,9 +373,9 @@ static HRESULT WINAPI BaseRenderer_Receive(struct strmbase_sink *pin, IMediaSamp hr = filter->ops->renderer_render(filter, sample); SetEvent(filter->state_event); - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); WaitForMultipleObjects(2, events, FALSE, INFINITE); - EnterCriticalSection(&filter->filter.stream_cs); + EnterCriticalSection(&filter->filter.filter_cs); filter->current_sample = NULL; } @@ -475,14 +475,14 @@ static HRESULT sink_end_flush(struct strmbase_sink *iface) { struct strmbase_renderer *filter = impl_from_IPin(&iface->pin.IPin_iface); - EnterCriticalSection(&filter->filter.stream_cs); + EnterCriticalSection(&filter->filter.filter_cs); filter->eos = FALSE; reset_qos(filter); strmbase_passthrough_invalidate_time(&filter->passthrough); ResetEvent(filter->flush_event); - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->filter.filter_cs); return S_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8712
You cannot take a lock from the streaming thread, ever, if that lock will be held from the main thread while waiting for the streaming thread to finish. This is why stream_cs exists in the first place. It's certainly true, of course, that the streaming thread *can* call any function that takes filter_cs. We've generally gone by assuming that it won't, and in times like these we have to update those assumptions. I'm not really sure how native deals with this. Possibly we need the passthrough to cache peers. Quartz threading is horribly underspecified in the documentation, and unfortunately, my computer that has a large portion of my personal notes on it recently broke. I'll update this when I'm able to consult those notes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8712#note_112637
This merge request was closed by Conor McCarthy. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8712
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Conor McCarthy (＠cmccarthy) -
Elizabeth Figura (＠zfigura)