[PATCH 2/3] winhttp: Manage task cleanup in task_callback() and queue_task().

[Paul Gofman]

On 3/15/22 18:26, Hans Leidekker wrote:
> On Mon, 2022-03-14 at 20:04 +0300, Paul Gofman wrote:
>> diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c
>> index 36a3bb909b9..f98f5f30c2a 100644
>> --- a/dlls/winhttp/request.c
>> +++ b/dlls/winhttp/request.c
>> @@ -151,18 +151,27 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, T
>>       struct task_header *task_hdr = ctx;
>>   
>>       task_hdr->callback( task_hdr );
>> +    release_object( task_hdr->obj );
>> +    free( task_hdr );
>>   }
>>   
>> -static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr )
>> +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr,
>> +                         struct object_header *obj )
>>   {
>>       TP_WORK *work;
>>       DWORD ret;
>>   
>>       if ((ret = start_queue( queue ))) return ret;
>>   
>> -    if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) return GetLastError();
>> +    if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env )))
>> +    {
>> +        free( task_hdr );
>> +        return GetLastError();
>> +    }
> You missed freeing task_hdr when start_queue() fails, though I'd prefer freeing
> it in the callers. Then you also wouldn't need that headers_dup variable in
> WinHttpSendRequest().

Do you mean always leaving the free on queue_task() failure where it is 
before the patch?


>> @@ -3036,13 +3014,11 @@ BOOL WINAPI WinHttpWriteData( HINTERNET hrequest, const void *buffer, DWORD to_w
>>           struct write_data *w;
>>   
>>           if (!(w = malloc( sizeof(*w) ))) return FALSE;
>> -        w->request  = request;
>>           w->buffer   = buffer;
>>           w->to_write = to_write;
>>           w->written  = written;
>>   
>> -        addref_object( &request->hdr );
>> -        if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr)))
>> +        if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr, &request->hdr )))
>>           {
>>               release_object( &request->hdr );
>>               free( w );
> This release_object() call should also be removed.
>
Indeed, thanks.


You signed off the last patch, do you mean I can just resend this one?