[PATCH] qemu: code protection for qemuBlockJobEventProcessLegacy

Chongyun Wu wucy11 at chinatelecom.cn
Thu Jul 1 08:37:29 UTC 2021



On 2021/7/1 15:24, Peter Krempa wrote:
> On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81 at chinatelecom.cn wrote:
>> From: Chongyun Wu <wucy11 at chinatelecom.cn>
>>
>> pointer disk might be null in some special cases or new
>> usage scenarios, therefore code protection is needed to
>> prevent segment faults.
> 
> Please elaborate on when that happens. Generally the legacy block job
> handler should no longer be in action with new versions, elaborate
> please also where you are seeing this.
Thanks for your comments. I found it when I develop a new feature which make
quemu2.12 support the rbd block hot migration with blockcopy command, I changed
some processes and triggered this crash, and normal use will not have this 
problem. So I just want to do some protection at the code level. Thanks~

> 
>>
>> Signed-off-by: Chongyun Wu <wucy11 at chinatelecom.cn>
>> ---
>>   src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++++++++-------------------
>>   1 file changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
>> index faf9a9f..00506b9 100644
>> --- a/src/qemu/qemu_blockjob.c
>> +++ b/src/qemu/qemu_blockjob.c
>> @@ -781,12 +781,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
>>   {
>>       virDomainDiskDef *disk = job->disk;
>>   
>> -    VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
>> -              disk->dst,
>> -              NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
>> -              job->type,
>> -              job->state,
>> -              job->newstate);
>> +    if (disk)
>> +        VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
>> +                  disk->dst,
>> +                  NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
>> +                  job->type,
>> +                  job->state,
>> +                  job->newstate);
> 
> The legacy block job handler makes no real sense  if disk is NULL ...
Yes, you are right.
The reason I used to do this is that if ob->state is 
VIR_DOMAIN_BLOCK_JOB_COMPLETED, you can ignore whether the disk is empty.
But what you said also makes sens, maybe I can do the judgement at the beginning.

> 
>>   
>>       if (job->newstate == -1)
>>           return;
>> @@ -804,26 +805,30 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
>>           break;
>>   
>>       case VIR_DOMAIN_BLOCK_JOB_READY:
>> -        disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
>> -        qemuDomainSaveStatus(vm);
>> +        if (disk) {
>> +            disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
>> +            qemuDomainSaveStatus(vm);
> 
> ... so the whole function can be skipped ...
> 
>> +        }
>>           break;
>>   
>>       case VIR_DOMAIN_BLOCK_JOB_FAILED:
>>       case VIR_DOMAIN_BLOCK_JOB_CANCELED:
>> -        if (disk->mirror) {
>> -            virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
>> +        if (disk) {
>> +            if (disk->mirror) {
>> +                virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
> 
> ... rather than special casing everything.
> 
>>   
>> -            /* Ideally, we would restore seclabels on the backing chain here
>> -             * but we don't know if somebody else is not using parts of it.
>> -             * Remove security driver metadata so that they are not leaked. */
>> -            qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
>> +                /* Ideally, we would restore seclabels on the backing chain here
>> +                 * but we don't know if somebody else is not using parts of it.
>> +                 * Remove security driver metadata so that they are not leaked. */
>> +                qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
>>   
>> -            virObjectUnref(disk->mirror);
>> -            disk->mirror = NULL;
>> +                virObjectUnref(disk->mirror);
>> +                disk->mirror = NULL;
>> +            }
>> +            disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
>> +            disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
>> +            qemuBlockJobUnregister(job, vm);
>>           }
>> -        disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
>> -        disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
>> -        qemuBlockJobUnregister(job, vm);
>>           break;
>>   
>>       case VIR_DOMAIN_BLOCK_JOB_LAST:
>> -- 
>> 1.8.3.1
>>
> 

-- 
Best Regard,
Chongyun Wu




More information about the libvir-list mailing list