Re: [PATCH] qemu: code protection for qemuBlockJobEventProcessLegacy
On 2021/7/1 15:24, Peter Krempa wrote:
On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81(a)chinatelecom.cn
wrote:
> From: Chongyun Wu <wucy11(a)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(a)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