From: Derek Lesho <dlesho@codeweavers.com>
Subject: Re: [PATCH 6/6] rtworkq: Fix reference leak when canceling waiting work items.
Message-Id: <4a213faf-ed47-117d-550b-cfc07ad8e370@codeweavers.com>
Date: Mon, 10 Jan 2022 09:36:40 -0500
In-Reply-To: <2799ad41-0140-52ba-40d9-8761bc654f5a@codeweavers.com>
References: <20220104173527.77306-1-dlesho@codeweavers.com> <20220104173527.77306-6-dlesho@codeweavers.com> <2799ad41-0140-52ba-40d9-8761bc654f5a@codeweavers.com>


On 1/10/22 09:20, Nikolay Sivov wrote:
>
>
> On 1/4/22 20:35, Derek Lesho wrote:
>> A waiting work_item has two references, the initial reference from 
>> creation, and an additional reference associated with its presence 
>> pending_items list, freed through queue_release_pending_item.  
>> RtwqCancelWorkItem only releases the second reference.
>>
> ...
>> @@ -866,6 +866,7 @@ static HRESULT queue_cancel_item(struct queue 
>> *queue, RTWQWORKITEM_KEY key)
>>           if (item->key == key)
>>           {
>>               key >>= 32;
>> +            queue_release_pending_item(item);
>>               if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK)
>>               {
>>                   IRtwqAsyncResult_SetStatus(item->result, 
>> RTWQ_E_OPERATION_CANCELLED);
>> @@ -876,7 +877,7 @@ static HRESULT queue_cancel_item(struct queue 
>> *queue, RTWQWORKITEM_KEY key)
>>                   CloseThreadpoolTimer(item->u.timer_object);
>>               else
>>                   WARN("Unknown item key mask %#x.\n", (DWORD)key);
>> -            queue_release_pending_item(item);
>> +            IUnknown_Release(&item->IUnknown_iface);
>>               hr = S_OK;
>>               break;
>>           }
> Yes, this looks correct, I think. Why did you have to move 
> queue_release_pending_item() though?
I didn't have to, but I thought it made more sense not to contrast it 
with the behavior in waiting_item_cancelable_callback, as in both cases 
we don't need the item to stay in the pending list while we execute the 
callback.