[PATCH 1/2] quartz/tests: Test IGraphBuilder::RenderFile cleanup on failure

[Tim Clem]

June 28, 2021 12:46 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:

> On 6/28/21 1:46 PM, Tim Clem wrote:
> 
>> Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds
>> if the file cannot be played. This was resulting in leaked file handles and threads if a
>> program tried to play media without a proper set of filters installed.
>> Signed-off-by: Tim Clem <tclem at codeweavers.com>
>> ---
>> dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++
>> 1 file changed, 54 insertions(+)
>> diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c
>> index fdcfcead0be..a509a07872c 100644
>> --- a/dlls/quartz/tests/filtergraph.c
>> +++ b/dlls/quartz/tests/filtergraph.c
>> @@ -4186,6 +4186,59 @@ static void test_ec_complete(void)
>> ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref);
>> }
>>> +static void test_renderfile_failure(void)
>> +{
>> + static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef};
>> +
>> + struct testfilter testfilter;
>> + IFilterGraph2 *graph;
>> + IEnumFilters *filterenum;
>> + IBaseFilter *filter;
>> + HRESULT hr;
>> + BOOL saw_testfilter = FALSE;
>> + FILTER_INFO filter_info;
>> + const WCHAR *filename;
>> +
>> + /* Windows removes the source filter from the graph if a RenderFile
>> + * call fails. It leaves the rest of the graph intact. */
>> +
>> + graph = create_graph();
>> + testfilter_init(&testfilter, NULL, 0);
>> + hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy");
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> +
>> + filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data));
>> + hr = IFilterGraph2_RenderFile(graph, filename, NULL);
>> + if (SUCCEEDED(hr)) {
>> + skip("Expected RenderFile to fail on invalid file.\n");
>> + goto out;
>> + }
> 
> Any reason why this isn't "ok(hr == VFW_E_CANNOT_RENDER, ...)"?

Windows reports VFW_E_UNSUPPORTED_STREAM in this case. The documentation on the various return values is really vague, so I thought it was best to be non-specific here.

> 
>> +
>> + hr = IFilterGraph2_EnumFilters(graph, &filterenum);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> +
>> + while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK)
>> + {
>> + if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface)
>> + saw_testfilter = TRUE;
>> + else {
>> + hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n",
>> + filter, wine_dbgstr_w(filter_info.achName));
>> + }
>> +
>> + IBaseFilter_Release(filter);
>> + }
>> + IEnumFilters_Release(filterenum);
>> +
>> + ok(saw_testfilter, "Expected testfilter to remain in graph.\n");
> 
> Or, even easier:
> 
> ok(testfilter.graph == (IFilterGraph *)graph, "Got graph %p.\n", testfilter.graph);

I updated the test as per your other email. Much simpler. Thanks for the notes.

> 
>> +
>> +out:
>> + IFilterGraph2_Release(graph);
>> + DeleteFileW(filename);
> 
> Can you please check for failure from DeleteFile() here? I try to be extra paranoid that we aren't
> leaking handles to the file anywhere; we've done so in the past.

Good idea, especially since this *is* leaking a file handle here before the patch - the source made by RenderFile will not be closed.

> 
>> +}
>> +
>> /* Remove and re-add the filter, to flush the graph's internal
>> * IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show
>> * that it's only queried when needed. */
>> @@ -5645,6 +5698,7 @@ START_TEST(filtergraph)
>> test_sync_source();
>> test_filter_state();
>> test_ec_complete();
>> + test_renderfile_failure();
>> test_graph_seeking();
>> test_default_sync_source();
>> test_add_source_filter();