[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