[PATCH] mfplat/tests: Avoid crash by async cancel callback in linux.
Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> --- The cancel in test_scheduled_items puts a work item to the queue by invoke_async_callback. But when this work item is processed the function test_scheduled_items is possibly already left, and the stack memory overwritten by the next test. This testbot run tried to force the crash by invalidating the callback: https://testbot.winehq.org/JobDetails.pl?Key=103981 --- dlls/mfplat/tests/mfplat.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 4dfdc18bb52..d1664e81031 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -2933,19 +2933,24 @@ static void test_MFHeapAlloc(void) MFHeapFree(res); } +/* Documentation for MFCancelWorkItem mentions that the callback + might still be called, even when MFCancelWorkItem is left. + In Wine implementation the cancel queues a work item + via invoke_async_callback which needs the callback later. */ +static struct test_callback scheduled_items_callback; + static void test_scheduled_items(void) { - struct test_callback callback; IMFAsyncResult *result; MFWORKITEM_KEY key, key2; HRESULT hr; - init_test_callback(&callback); + init_test_callback(&scheduled_items_callback); hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); ok(hr == S_OK, "Failed to start up, hr %#x.\n", hr); - hr = MFScheduleWorkItem(&callback.IMFAsyncCallback_iface, NULL, -5000, &key); + hr = MFScheduleWorkItem(&scheduled_items_callback.IMFAsyncCallback_iface, NULL, -5000, &key); ok(hr == S_OK, "Failed to schedule item, hr %#x.\n", hr); hr = MFCancelWorkItem(key); @@ -2960,7 +2965,7 @@ static void test_scheduled_items(void) return; } - hr = MFCreateAsyncResult(NULL, &callback.IMFAsyncCallback_iface, NULL, &result); + hr = MFCreateAsyncResult(NULL, &scheduled_items_callback.IMFAsyncCallback_iface, NULL, &result); ok(hr == S_OK, "Failed to create result, hr %#x.\n", hr); hr = pMFPutWaitingWorkItem(NULL, 0, result, &key); @@ -2977,7 +2982,7 @@ static void test_scheduled_items(void) IMFAsyncResult_Release(result); - hr = MFScheduleWorkItem(&callback.IMFAsyncCallback_iface, NULL, -5000, &key); + hr = MFScheduleWorkItem(&scheduled_items_callback.IMFAsyncCallback_iface, NULL, -5000, &key); ok(hr == S_OK, "Failed to schedule item, hr %#x.\n", hr); hr = MFCancelWorkItem(key); -- 2.33.0
On 12/12/21 18:50, Bernhard Übelacker wrote:
Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> --- The cancel in test_scheduled_items puts a work item to the queue by invoke_async_callback. But when this work item is processed the function test_scheduled_items is possibly already left, and the stack memory overwritten by the next test. Thank you for working on this.
I think it's better to make "struct test_callback*" heap-allocated instead, for all tests, not just for scheduled items.
Am 12.12.21 um 18:33 schrieb Nikolay Sivov:
On 12/12/21 18:50, Bernhard Übelacker wrote:
Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> --- The cancel in test_scheduled_items puts a work item to the queue by invoke_async_callback. But when this work item is processed the function test_scheduled_items is possibly already left, and the stack memory overwritten by the next test. Thank you for working on this.
I think it's better to make "struct test_callback*" heap-allocated instead, for all tests, not just for scheduled items.
Thanks for the review, I will send an updated version of the patch. Kind regards, Bernhard
On 12/13/21 12:16, Bernhard Übelacker wrote:
Am 12.12.21 um 18:33 schrieb Nikolay Sivov:
On 12/12/21 18:50, Bernhard Übelacker wrote:
Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> --- The cancel in test_scheduled_items puts a work item to the queue by invoke_async_callback. But when this work item is processed the function test_scheduled_items is possibly already left, and the stack memory overwritten by the next test. Thank you for working on this.
I think it's better to make "struct test_callback*" heap-allocated instead, for all tests, not just for scheduled items.
Thanks for the review, I will send an updated version of the patch.
This one seems to work. I got it done before your reply, so feel free to use that, or to send your own.
Kind regards, Bernhard
Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> ---- Am 13.12.21 um 11:11 schrieb Nikolay Sivov:
On 12/13/21 12:16, Bernhard Übelacker wrote:
Am 12.12.21 um 18:33 schrieb Nikolay Sivov:
On 12/12/21 18:50, Bernhard Übelacker wrote:
Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> --- The cancel in test_scheduled_items puts a work item to the queue by invoke_async_callback. But when this work item is processed the function test_scheduled_items is possibly already left, and the stack memory overwritten by the next test. Thank you for working on this.
I think it's better to make "struct test_callback*" heap-allocated instead, for all tests, not just for scheduled items.
Thanks for the review, I will send an updated version of the patch.
This one seems to work. I got it done before your reply, so feel free to use that, or to send your own.
Hello Nikolay, sorry for the late reply, your patch is way more advanced than everything I would have come up with. Should I send it as [PATCH v2] or would my Signed-off-by already be enough for your patch to get considered? Kind regards, Bernhard
participants (2)
-
Bernhard Übelacker -
Nikolay Sivov