[PATCH] qemu: code protection for qemuBlockJobEventProcessLegacy

[Peter Krempa]

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.

> 
> 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 ...

>  
>      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
>