[PATCH v3 0/5] MR1278: winegstreamer: Do not destroy condition variables when a thread is waiting on them.
It is possible that a stream is destroyed while another thread is waiting on event_empty_cond or event_cond. The waiting thread should have the opportunity to be rescheduled and exit the critical section before the condition variables and the mutex are destroyed.
-- v3: winegstreamer: Synchronize media source async commands and shutdown. winegstreamer: Free the GStreamer buffer when freeing a WG parser stream. winegstreamer: Synchronize access to the media source from callbacks. winegstreamer: Synchronize concurrent access to the media stream. winegstreamer: Synchronize concurrent access to the media source.
From: Giovanni Mascellani gmascellani@codeweavers.com
--- dlls/winegstreamer/media_source.c | 60 +++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 11 deletions(-)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 9bb7a441a8f..a9f0eb70902 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -92,6 +92,8 @@ struct media_source IMFMediaEventQueue *event_queue; IMFByteStream *byte_stream;
+ CRITICAL_SECTION cs; + struct wg_parser *wg_parser;
struct media_stream **streams; @@ -1114,7 +1116,9 @@ static HRESULT WINAPI media_source_rate_control_SetRate(IMFRateControl *iface, B if (FAILED(hr = IMFRateSupport_IsRateSupported(&source->IMFRateSupport_iface, thin, rate, NULL))) return hr;
+ EnterCriticalSection(&source->cs); source->rate = rate; + LeaveCriticalSection(&source->cs);
return IMFMediaEventQueue_QueueEventParamVar(source->event_queue, MESourceRateChanged, &GUID_NULL, S_OK, NULL); } @@ -1128,7 +1132,9 @@ static HRESULT WINAPI media_source_rate_control_GetRate(IMFRateControl *iface, B if (thin) *thin = FALSE;
+ EnterCriticalSection(&source->cs); *rate = source->rate; + LeaveCriticalSection(&source->cs);
return S_OK; } @@ -1190,6 +1196,7 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) { IMFMediaSource_Shutdown(&source->IMFMediaSource_iface); IMFMediaEventQueue_Release(source->event_queue); + DeleteCriticalSection(&source->cs); free(source); }
@@ -1236,10 +1243,15 @@ static HRESULT WINAPI media_source_QueueEvent(IMFMediaSource *iface, MediaEventT static HRESULT WINAPI media_source_GetCharacteristics(IMFMediaSource *iface, DWORD *characteristics) { struct media_source *source = impl_from_IMFMediaSource(iface); + BOOL is_shutdown;
TRACE("%p, %p.\n", iface, characteristics);
- if (source->state == SOURCE_SHUTDOWN) + EnterCriticalSection(&source->cs); + is_shutdown = source->state == SOURCE_SHUTDOWN; + LeaveCriticalSection(&source->cs); + + if (is_shutdown) return MF_E_SHUTDOWN;
*characteristics = MFMEDIASOURCE_CAN_SEEK | MFMEDIASOURCE_CAN_PAUSE; @@ -1250,10 +1262,15 @@ static HRESULT WINAPI media_source_GetCharacteristics(IMFMediaSource *iface, DWO static HRESULT WINAPI media_source_CreatePresentationDescriptor(IMFMediaSource *iface, IMFPresentationDescriptor **descriptor) { struct media_source *source = impl_from_IMFMediaSource(iface); + BOOL is_shutdown;
TRACE("%p, %p.\n", iface, descriptor);
- if (source->state == SOURCE_SHUTDOWN) + EnterCriticalSection(&source->cs); + is_shutdown = source->state == SOURCE_SHUTDOWN; + LeaveCriticalSection(&source->cs); + + if (is_shutdown) return MF_E_SHUTDOWN;
return IMFPresentationDescriptor_Clone(source->pres_desc, descriptor); @@ -1264,11 +1281,16 @@ static HRESULT WINAPI media_source_Start(IMFMediaSource *iface, IMFPresentationD { struct media_source *source = impl_from_IMFMediaSource(iface); struct source_async_command *command; + BOOL is_shutdown; HRESULT hr;
TRACE("%p, %p, %p, %p.\n", iface, descriptor, time_format, position);
- if (source->state == SOURCE_SHUTDOWN) + EnterCriticalSection(&source->cs); + is_shutdown = source->state == SOURCE_SHUTDOWN; + LeaveCriticalSection(&source->cs); + + if (is_shutdown) return MF_E_SHUTDOWN;
if (!(IsEqualIID(time_format, &GUID_NULL))) @@ -1290,11 +1312,16 @@ static HRESULT WINAPI media_source_Stop(IMFMediaSource *iface) { struct media_source *source = impl_from_IMFMediaSource(iface); struct source_async_command *command; + BOOL is_shutdown; HRESULT hr;
TRACE("%p.\n", iface);
- if (source->state == SOURCE_SHUTDOWN) + EnterCriticalSection(&source->cs); + is_shutdown = source->state == SOURCE_SHUTDOWN; + LeaveCriticalSection(&source->cs); + + if (is_shutdown) return MF_E_SHUTDOWN;
if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_STOP, &command))) @@ -1307,14 +1334,20 @@ static HRESULT WINAPI media_source_Pause(IMFMediaSource *iface) { struct media_source *source = impl_from_IMFMediaSource(iface); struct source_async_command *command; + BOOL is_shutdown, is_running; HRESULT hr;
TRACE("%p.\n", iface);
- if (source->state == SOURCE_SHUTDOWN) + EnterCriticalSection(&source->cs); + is_shutdown = source->state == SOURCE_SHUTDOWN; + is_running = source->state == SOURCE_RUNNING; + LeaveCriticalSection(&source->cs); + + if (is_shutdown) return MF_E_SHUTDOWN;
- if (source->state != SOURCE_RUNNING) + if (!is_running) return MF_E_INVALID_STATE_TRANSITION;
if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_PAUSE, &command))) @@ -1327,14 +1360,20 @@ static HRESULT WINAPI media_source_Pause(IMFMediaSource *iface) static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { struct media_source *source = impl_from_IMFMediaSource(iface); + BOOL is_shutdown; unsigned int i;
TRACE("%p.\n", iface);
- if (source->state == SOURCE_SHUTDOWN) - return MF_E_SHUTDOWN; - + EnterCriticalSection(&source->cs); + is_shutdown = source->state == SOURCE_SHUTDOWN; source->state = SOURCE_SHUTDOWN; + for (i = 0; i < source->stream_count; i++) + source->streams[i]->state = STREAM_SHUTDOWN; + LeaveCriticalSection(&source->cs); + + if (is_shutdown) + return MF_E_SHUTDOWN;
wg_parser_disconnect(source->wg_parser);
@@ -1350,8 +1389,6 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { struct media_stream *stream = source->streams[i];
- stream->state = STREAM_SHUTDOWN; - IMFMediaEventQueue_Shutdown(stream->event_queue); IMFStreamDescriptor_Release(stream->descriptor); IMFMediaSource_Release(&stream->parent_source->IMFMediaSource_iface); @@ -1424,6 +1461,7 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_ object->byte_stream = bytestream; IMFByteStream_AddRef(bytestream); object->rate = 1.0f; + InitializeCriticalSection(&object->cs);
if (FAILED(hr = MFCreateEventQueue(&object->event_queue))) goto fail;
From: Giovanni Mascellani gmascellani@codeweavers.com
--- dlls/winegstreamer/media_source.c | 57 ++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 17 deletions(-)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index a9f0eb70902..418f6177bea 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -746,31 +746,51 @@ static HRESULT WINAPI media_stream_QueueEvent(IMFMediaStream *iface, MediaEventT static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMediaSource **source) { struct media_stream *stream = impl_from_IMFMediaStream(iface); + HRESULT hr;
TRACE("%p, %p.\n", iface, source);
+ EnterCriticalSection(&stream->parent_source->cs); + if (stream->state == STREAM_SHUTDOWN) - return MF_E_SHUTDOWN; + { + hr = MF_E_SHUTDOWN; + } + else + { + IMFMediaSource_AddRef(&stream->parent_source->IMFMediaSource_iface); + *source = &stream->parent_source->IMFMediaSource_iface; + hr = S_OK; + }
- IMFMediaSource_AddRef(&stream->parent_source->IMFMediaSource_iface); - *source = &stream->parent_source->IMFMediaSource_iface; + LeaveCriticalSection(&stream->parent_source->cs);
- return S_OK; + return hr; }
static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IMFStreamDescriptor **descriptor) { struct media_stream *stream = impl_from_IMFMediaStream(iface); + HRESULT hr;
TRACE("%p, %p.\n", iface, descriptor);
+ EnterCriticalSection(&stream->parent_source->cs); + if (stream->state == STREAM_SHUTDOWN) - return MF_E_SHUTDOWN; + { + hr = MF_E_SHUTDOWN; + } + else + { + IMFStreamDescriptor_AddRef(stream->descriptor); + *descriptor = stream->descriptor; + hr = S_OK; + }
- IMFStreamDescriptor_AddRef(stream->descriptor); - *descriptor = stream->descriptor; + LeaveCriticalSection(&stream->parent_source->cs);
- return S_OK; + return hr; }
static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown *token) @@ -781,21 +801,22 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown
TRACE("%p, %p.\n", iface, token);
- if (stream->state == STREAM_SHUTDOWN) - return MF_E_SHUTDOWN; + EnterCriticalSection(&stream->parent_source->cs);
- if (stream->state == STREAM_INACTIVE) + if (stream->state == STREAM_SHUTDOWN) + { + hr = MF_E_SHUTDOWN; + } + else if (stream->state == STREAM_INACTIVE) { WARN("Stream isn't active\n"); - return MF_E_MEDIA_SOURCE_WRONGSTATE; + hr = MF_E_MEDIA_SOURCE_WRONGSTATE; } - - if (stream->eos) + else if (stream->eos) { - return MF_E_END_OF_STREAM; + hr = MF_E_END_OF_STREAM; } - - if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &command))) + else if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &command))) { command->u.request_sample.stream = stream; if (token) @@ -806,6 +827,8 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown &stream->parent_source->async_commands_callback, &command->IUnknown_iface); }
+ LeaveCriticalSection(&stream->parent_source->cs); + return hr; }
From: Giovanni Mascellani gmascellani@codeweavers.com
--- dlls/winegstreamer/media_source.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 418f6177bea..c56df5d62ae 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -533,10 +533,20 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) struct media_source *source = stream->parent_source; PROPVARIANT empty_var = {.vt = VT_EMPTY}; struct wg_parser_buffer buffer; + bool res;
TRACE("%p, %p\n", stream, token);
- if (wg_parser_stream_get_buffer(source->wg_parser, stream->wg_stream, &buffer)) + LeaveCriticalSection(&source->cs); + res = wg_parser_stream_get_buffer(source->wg_parser, stream->wg_stream, &buffer); + EnterCriticalSection(&source->cs); + + /* If the media source has been shut down during the wait, then + * resources have been released, so don't touch anything. */ + if (source->state == SOURCE_SHUTDOWN) + return; + + if (res) { send_buffer(stream, &buffer, token); } @@ -555,12 +565,18 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA IUnknown *state; HRESULT hr;
- if (source->state == SOURCE_SHUTDOWN) - return S_OK; - if (FAILED(hr = IMFAsyncResult_GetState(result, &state))) return hr;
+ EnterCriticalSection(&source->cs); + + if (source->state == SOURCE_SHUTDOWN) + { + LeaveCriticalSection(&source->cs); + IUnknown_Release(state); + return S_OK; + } + command = impl_from_async_command_IUnknown(state); switch (command->op) { @@ -581,6 +597,8 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA break; }
+ LeaveCriticalSection(&source->cs); + IUnknown_Release(state);
return S_OK;
From: Giovanni Mascellani gmascellani@codeweavers.com
--- dlls/winegstreamer/wg_parser.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 14970d327c8..520388de7cf 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -768,6 +768,13 @@ static void free_stream(struct wg_parser_stream *stream) } gst_object_unref(stream->my_sink);
+ if (stream->buffer) + { + gst_buffer_unmap(stream->buffer, &stream->map_info); + gst_buffer_unref(stream->buffer); + stream->buffer = NULL; + } + pthread_cond_destroy(&stream->event_cond); pthread_cond_destroy(&stream->event_empty_cond);
From: Giovanni Mascellani gmascellani@codeweavers.com
This is required so that Shutdown() won't proceed while a sample request is blocked in wg_parser_stream_get_buffer(). --- dlls/winegstreamer/media_source.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index c56df5d62ae..72607df8892 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -92,6 +92,7 @@ struct media_source IMFMediaEventQueue *event_queue; IMFByteStream *byte_stream;
+ CRITICAL_SECTION shutdown_cs; CRITICAL_SECTION cs;
struct wg_parser *wg_parser; @@ -568,6 +569,7 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA if (FAILED(hr = IMFAsyncResult_GetState(result, &state))) return hr;
+ EnterCriticalSection(&source->shutdown_cs); EnterCriticalSection(&source->cs);
if (source->state == SOURCE_SHUTDOWN) @@ -598,6 +600,7 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA }
LeaveCriticalSection(&source->cs); + LeaveCriticalSection(&source->shutdown_cs);
IUnknown_Release(state);
@@ -1238,6 +1241,7 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) IMFMediaSource_Shutdown(&source->IMFMediaSource_iface); IMFMediaEventQueue_Release(source->event_queue); DeleteCriticalSection(&source->cs); + DeleteCriticalSection(&source->shutdown_cs); free(source); }
@@ -1406,12 +1410,14 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)
TRACE("%p.\n", iface);
+ EnterCriticalSection(&source->shutdown_cs); EnterCriticalSection(&source->cs); is_shutdown = source->state == SOURCE_SHUTDOWN; source->state = SOURCE_SHUTDOWN; for (i = 0; i < source->stream_count; i++) source->streams[i]->state = STREAM_SHUTDOWN; LeaveCriticalSection(&source->cs); + LeaveCriticalSection(&source->shutdown_cs);
if (is_shutdown) return MF_E_SHUTDOWN; @@ -1502,6 +1508,7 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_ object->byte_stream = bytestream; IMFByteStream_AddRef(bytestream); object->rate = 1.0f; + InitializeCriticalSection(&object->shutdown_cs); InitializeCriticalSection(&object->cs);
if (FAILED(hr = MFCreateEventQueue(&object->event_queue)))
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126304
Your paranoid android.
=== debian11 (32 bit report) ===
d3d9: d3d9ex.c:3196: Test failed: Expected message 0x5 for window 0, but didn't receive it, i=0. d3d9ex.c:3202: Test failed: Got unexpected WINDOWPOS hwnd=00000000, x=0, y=0, cx=0, cy=0, flags=0
d3drm: d3drm.c:5561: Test failed: Cannot create IM device, skipping tests.
=== debian11 (build log) ===
0644:err:winediag:d3d_device_create The application wants to create a Direct3D device, but the current DirectDrawRenderer does not support this. 0644:err:winediag:d3d_device_create The application wants to create a Direct3D device, but the current DirectDrawRenderer does not support this. 0644:err:winediag:d3d_device_create The application wants to create a Direct3D device, but the current DirectDrawRenderer does not support this.
Zebediah Figura (@zfigura) commented about dlls/winegstreamer/media_source.c:
static HRESULT WINAPI media_source_CreatePresentationDescriptor(IMFMediaSource *iface, IMFPresentationDescriptor **descriptor) { struct media_source *source = impl_from_IMFMediaSource(iface);
BOOL is_shutdown;
TRACE("%p, %p.\n", iface, descriptor);
- if (source->state == SOURCE_SHUTDOWN)
EnterCriticalSection(&source->cs);
is_shutdown = source->state == SOURCE_SHUTDOWN;
LeaveCriticalSection(&source->cs);
if (is_shutdown) return MF_E_SHUTDOWN;
return IMFPresentationDescriptor_Clone(source->pres_desc, descriptor);
This can still access an invalid presentation descriptor if the media source is concurrently shut down.
There are other, similar cases, e.g. in Start() and other state change methods, we can access an invalid command queue. In a sense this patch is really just protecting concurrent access to the "state" field, but I don't think that makes much sense by itself, especially given the nature of the problem we're trying to solve here.
Zebediah Figura (@zfigura) commented about dlls/winegstreamer/media_source.c:
object->byte_stream = bytestream; IMFByteStream_AddRef(bytestream); object->rate = 1.0f;
- InitializeCriticalSection(&object->cs);
This could use a debug name.
Zebediah Figura (@zfigura) commented about dlls/winegstreamer/media_source.c:
struct media_source *source = stream->parent_source; PROPVARIANT empty_var = {.vt = VT_EMPTY}; struct wg_parser_buffer buffer;
bool res;
TRACE("%p, %p\n", stream, token);
- if (wg_parser_stream_get_buffer(source->wg_parser, stream->wg_stream, &buffer))
- LeaveCriticalSection(&source->cs);
- res = wg_parser_stream_get_buffer(source->wg_parser, stream->wg_stream, &buffer);
- EnterCriticalSection(&source->cs);
This is suspicious. Why do we need this?
Note also that this isn't thread-safe either; as soon as we drop the critical section we can already have been shut down.
Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_parser.c:
} gst_object_unref(stream->my_sink);
- if (stream->buffer)
- {
gst_buffer_unmap(stream->buffer, &stream->map_info);gst_buffer_unref(stream->buffer);stream->buffer = NULL;- }
Maybe. The awkward thing here is that the code currently assumes we'll *always* call wg_parser_stream_release_buffer() if we got a buffer, which makes this redundant.
On Fri Nov 18 00:25:03 2022 +0000, Zebediah Figura wrote:
This can still access an invalid presentation descriptor if the media source is concurrently shut down. There are other, similar cases, e.g. in Start() and other state change methods, we can access an invalid command queue. In a sense this patch is really just protecting concurrent access to the "state" field, but I don't think that makes much sense by itself, especially given the nature of the problem we're trying to solve here.
Yeah, you're right. I don't know how I convinced myself that that would be enough. Will fix.
On Fri Nov 18 00:25:05 2022 +0000, Zebediah Figura wrote:
This is suspicious. Why do we need this? Note also that this isn't thread-safe either; as soon as we drop the critical section we can already have been shut down.
I agree it's a bit weird, but `wg_parser_stream_get_buffer()` can block, and if the lock is kept basically no other media source or media stream method can be called. In general the lock shouldn't be kept for long, but in some cases it could. For example, we could be reading from a slow network.
I agree at this point it's still unsafe, but that's fixed in a later patch ("winegstreamer: Synchronize media source async commands and shutdown.").
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Marvin -
Zebediah Figura (＠zfigura)