[PATCH v3 0/1] MR2263: evr: Fix crash when clear input type for the mixer

[Tingzhong Luo (＠tzluo)]

In the implementation of
[`evr_disconnect()`](https://gitlab.winehq.org/wine/wine/-/blob/wine-8.2/dlls/evr/evr.c#L325-333),
it will try to clear types of mixer by pass null to 
`IMFTransform_SetInputType()`:

```c
static void evr_disconnect(struct strmbase_renderer *iface)
{
    struct evr *filter = impl_from_strmbase_renderer(iface);

if (filter->mixer)
        IMFTransform_SetInputType(filter->mixer, 0, NULL, 0);
    evr_set_input_type(filter, NULL);
    evr_release_services(filter);
}

```

This null ptr (`media_type`) will be pass to
[`video_mixer_collect_output_types()`](https://gitlab.winehq.org/wine/wine/-/blob/wine-8.2/dlls/evr/mixer.c#L869)
and cause a [crash](https://gitlab.winehq.org/wine/wine/-/blob/wine-8.2/dlls/evr/mixer.c#L663):

```c
static HRESULT WINAPI video_mixer_transform_SetInputType(IMFTransform *iface, DWORD id, IMFMediaType *media_type, DWORD flags)
{
...
    if (!mixer->device_manager)
        hr = MF_E_NOT_INITIALIZED;
    else
    {
        if (SUCCEEDED(hr = video_mixer_get_processor_service(mixer, &service)))
        {
            if (SUCCEEDED(hr = video_mixer_init_dxva_videodesc(media_type, &video_desc)))
...
```

In this patch, `video_mixer_transform_SetInputType()` will only clear types when
got a NULL `media_type` and return `S_OK`.

--
  v3: evr: Fix crash when clear input type for the mixer

https://gitlab.winehq.org/wine/wine/-/merge_requests/2263

[PATCH v3 1/1] evr: Fix crash when clear input type for the mixer

[Tingzhong Luo]

From: Tingzhong Luo

Signed-off-by: Tingzhong Luo 
---
 dlls/evr/mixer.c     | 18 ++++++++++++++----
 dlls/evr/tests/evr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/dlls/evr/mixer.c b/dlls/evr/mixer.c
index 668a97da979..ee7c65823ec 100644
--- a/dlls/evr/mixer.c
+++ b/dlls/evr/mixer.c
@@ -233,11 +233,16 @@ static void video_mixer_clear_types(struct video_mixer *mixer)
         mixer->inputs[i].media_type = NULL;
     }
     video_mixer_flush_input(mixer);
-    for (i = 0; i < mixer->output.rt_formats_count; ++i)
+    if (mixer->output.rt_formats)
     {
-        IMFMediaType_Release(mixer->output.rt_formats[i].media_type);
+        for (i = 0; i < mixer->output.rt_formats_count; ++i)
+        {
+            IMFMediaType_Release(mixer->output.rt_formats[i].media_type);
+        }
+        free(mixer->output.rt_formats);
+        mixer->output.rt_formats = NULL;
+        mixer->output.rt_formats_count = 0;
     }
-    free(mixer->output.rt_formats);
     if (mixer->output.media_type)
         IMFMediaType_Release(mixer->output.media_type);
     mixer->output.media_type = NULL;
@@ -855,12 +860,17 @@ static HRESULT WINAPI video_mixer_transform_SetInputType(IMFTransform *iface, DW

TRACE("%p, %lu, %p, %#lx.\n", iface, id, media_type, flags);

+    if (!media_type && (flags & MFT_SET_TYPE_TEST_ONLY))
+        return E_INVALIDARG;
+
     EnterCriticalSection(&mixer->cs);

if (!(flags & MFT_SET_TYPE_TEST_ONLY))
         video_mixer_clear_types(mixer);

-    if (!mixer->device_manager)
+    if (!media_type)
+        hr = S_OK;
+    else if (!mixer->device_manager)
         hr = MF_E_NOT_INITIALIZED;
     else
     {
diff --git a/dlls/evr/tests/evr.c b/dlls/evr/tests/evr.c
index acfb92f2ea5..6c93be4577a 100644
--- a/dlls/evr/tests/evr.c
+++ b/dlls/evr/tests/evr.c
@@ -1249,6 +1249,11 @@ static void test_default_mixer_type_negotiation(void)
         goto done;
     }

+    hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_SetInputType(transform, 0, NULL, MFT_SET_TYPE_TEST_ONLY);
+    ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr);
+
     hr = DXVA2CreateDirect3DDeviceManager9(&token, &manager);
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);

@@ -1469,9 +1474,48 @@ static void test_default_mixer_type_negotiation(void)
     hr = IMFTransform_GetOutputCurrentType(transform, 0, &media_type2);
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
     ok(media_type == media_type2, "Unexpected media type instance.\n");
+
+    IMFMediaType_Release(media_type);
+
+    /* Clear input types */
+    hr = IMFTransform_SetInputType(transform, 0, NULL, MFT_SET_TYPE_TEST_ONLY);
+    ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr);
+
+    hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_GetInputCurrentType(transform, 0, &media_type);
+    ok(hr == MF_E_TRANSFORM_TYPE_NOT_SET, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_GetOutputCurrentType(transform, 0, &media_type);
+    ok(hr == MF_E_TRANSFORM_TYPE_NOT_SET, "Unexpected hr %#lx.\n", hr);
+
+    /* Restore types */
+    hr = IMFTransform_SetOutputType(transform, 0, media_type2, 0);
+    ok(hr == MF_E_INVALIDMEDIATYPE, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_GetOutputCurrentType(transform, 0, &media_type);
+    ok(hr == MF_E_TRANSFORM_TYPE_NOT_SET, "Unexpected hr %#lx.\n", hr);
+
+    hr = IMFTransform_SetInputType(transform, 0, video_type, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_GetInputCurrentType(transform, 0, &media_type);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(media_type == video_type, "Unexpected media type instance.\n");
+    IMFMediaType_Release(media_type);
+
+    hr = IMFTransform_SetOutputType(transform, 0, media_type2, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_GetOutputCurrentType(transform, 0, &media_type);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(media_type2 == media_type, "Unexpected media type instance.\n");
+
     IMFMediaType_Release(media_type2);
     IMFMediaType_Release(media_type);

+    /* It shouldn't cause memory exception if clear again */
+    hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
     IMFVideoProcessor_Release(processor);

IMFMediaType_Release(video_type);

[Tingzhong Luo (＠tzluo)]

Tingzhong Luo (@tzluo) commented about dlls/evr/tests/evr.c:
>      goto done;
>  }
>
> - hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
> - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> - hr = IMFTransform_SetInputType(transform, 0, NULL, MFT_SET_TYPE_TEST_ONLY);
> - ok(hr == E_INVALIDARG, "Unexpected hr %#lx.\n", hr);
> -

Now added TEST_ONLY case in v3, and there is no failures on my windows virtual machine.

[Tingzhong Luo (＠tzluo)]

On Thu Feb 23 08:19:32 2023 +0000, Nikolay Sivov wrote:
> There should be no need to check for null 'rt_formats', counter should
> be 0 if this pointer is null.

Sorry that I have not found any code to reset `mixer->output.rt_formats` or
`mixer->output.rt_formats_count` in current source tree.

It might cause another crash if application want to clear media type of mixer
twice, like this:

```c
hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
```

in this case, it `mixer->output.rt_formats` has been free, but it try to release
a invalid pointer in second call.

Another way to cause a crash will looks like this:

```c
hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
hr = IMFTransform_SetOutputType(transform, 0, media_type, 0);
```

`output.rt_formats[i].media_type` might be released and IMFTransform_SetOutputType() try to access it.

In v3 of this patch, those crash can be reproduce in the test if reverse changes in
`video_mixer_clear_types()`.

[Nikolay Sivov (＠nsivov)]

On Thu Feb 23 11:25:41 2023 +0000, Tingzhong Luo wrote:
> Sorry that I have not found any code to reset `mixer->output.rt_formats` or
> `mixer->output.rt_formats_count` in current source tree.
> It might cause another crash if application want to clear media type of mixer
> twice, like this:
> hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
> hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
>
> in this case, it `mixer->output.rt_formats` has been free, but it try to release
> a invalid pointer in second call.
> Another way to cause a crash will looks like this:
> hr = IMFTransform_SetInputType(transform, 0, NULL, 0);
> hr = IMFTransform_SetOutputType(transform, 0, media_type, 0);
>
> `output.rt_formats[i].media_type` might be released and
> IMFTransform_SetOutputType() try to access it.
> In v3 of this patch, those crash can be reproduce in the test if reverse
> changes in
> `video_mixer_clear_types()`.

For that we could memset() whole mixer->output structure in video_mixer_clear_types().