Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: fix dead pointer in txn list

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 02.08.2016 01:39, John Snow wrote:
> On 07/27/2016 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Job may be freed in block_job_unref and in this case this would break
>> transaction QLIST.
>>
>> Fix this by removing job from this list before unref.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  blockjob.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index a5ba3be..e045091 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -216,6 +216,7 @@ static void block_job_completed_single(BlockJob 
>> *job)
>>      }
>>      job->cb(job->opaque, job->ret);
>>      if (job->txn) {
>> +        QLIST_REMOVE(job, txn_list);
>>          block_job_txn_unref(job->txn);
>>      }
>>      block_job_unref(job);
>>
>
> Has this caused actual problems for you?

Yes, with the same changed test 124 (my parallel thread). Backup job can 
finish  too early (if dirty bitmap is empty) and then we use this 
transaction job list with dead pointer.

>
> This function is only ever called in a transactional context if the 
> transaction is over -- so we're not likely to use the pointers ever 
> again anyway.

Backup job may finish even earlier than all jobs are added to the list. 
(same case, empty dirty bitmap for one of drives).

>
> Still, it's good practice, and the caller uses a safe iteration of the 
> list, so I think this should be safe.
>
> But I don't think this SHOULD fix an actual bug. If it does, I think 
> something else is wrong.
>
> Tested-by: John Snow <jsnow@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>


-- 
Best regards,
Vladimir