[PATCH 0/1] MR3955: mf: Avoid a double free of presentation clock timers.

[Zhiyi Zhang (＠zhiyi)]

In clock_change_state() when a clock is running, a timer is removed from clock->timers. The same
timer is then used to create an async result, which will eventually calls present_clock_timer_callback_Invoke()
and removes the same timer.

[PATCH 1/1] mf: Avoid a double free of presentation clock timers.

[Zhiyi Zhang]

From: Zhiyi Zhang

In clock_change_state() when a clock is running, a timer is removed from clock->timers. The same
timer is then used to create an async result, which will eventually calls present_clock_timer_callback_Invoke()
and removes the same timer.
---
 dlls/mf/clock.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/dlls/mf/clock.c b/dlls/mf/clock.c
index aef4366c6d9..ca9937d90dd 100644
--- a/dlls/mf/clock.c
+++ b/dlls/mf/clock.c
@@ -623,9 +623,7 @@ static HRESULT clock_change_state(struct presentation_clock *clock, enum clock_c
         {
             LIST_FOR_EACH_ENTRY_SAFE(timer, timer2, &clock->timers, struct clock_timer, entry)
             {
-                list_remove(&timer->entry);
                 hr = MFCreateAsyncResult(&timer->IUnknown_iface, &clock->timer_callback, NULL, &result);
-                IUnknown_Release(&timer->IUnknown_iface);
                 if (SUCCEEDED(hr))
                 {
                     MFPutWorkItemEx(MFASYNC_CALLBACK_QUEUE_TIMER, result);

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mf/clock.c:
>      {
>          LIST_FOR_EACH_ENTRY_SAFE(timer, timer2, &clock->timers, struct clock_timer, entry)
>          {
> -
>            list_remove(&timer->entry);
>            hr = MFCreateAsyncResult(&timer->IUnknown_iface, &clock->timer_callback, NULL, &result);
> -
>            IUnknown_Release(&timer->IUnknown_iface);
>            if (SUCCEEDED(hr))
>            {
>                MFPutWorkItemEx(MFASYNC_CALLBACK_QUEUE_TIMER, result);

I think it should be enough to list_init() it here, so that callback works correctly. Why do you think Release() is not needed here?

[Zhiyi Zhang (＠zhiyi)]

On Tue Sep 26 10:15:23 2023 +0000, Nikolay Sivov wrote:
> I think it should be enough to list_init() it here, so that callback
> works correctly. Why do you think Release() is not needed here?

Because present_clock_timer_callback_Invoke() releases the same object. If Release() is called here then present_clock_timer_callback_Invoke() also gets called, the timer object will be released twice while being in `clock->timers` only holds one reference count.

It's either this or check timer is still in clock->timers when calling present_clock_timer_callback_Invoke().

[Nikolay Sivov (＠nsivov)]

On Tue Sep 26 10:21:37 2023 +0000, Zhiyi Zhang wrote:
> Because present_clock_timer_callback_Invoke() releases the same object.
> If Release() is called here then present_clock_timer_callback_Invoke()
> also gets called, the timer object will be released twice while being in
> `clock->timers` only holds one reference count.
> It's either this or check timer is still in clock->timers when calling present_clock_timer_callback_Invoke().

Ok, it's worse than I thought. I think present_clock_timer_CancelTimer() also has the same race. The idea was that items should be removed when transitioning to the running state, so that potentially on the next transition they are not submitted again. I think we should try to make both cases work. We could, like you said, check that it's in the list, some list entry pointer check might be enough to tell if it is.