[PATCH 0/2] qemu: blockjob: Fix two issues with block pull job finishing
Peter Krempa (2): qemuBlockJobProcessEventCompletedPull: Avoid dangling pointer after blockpull qemuBlockJobProcessEventCompletedPull: Add backingStore terminators if base is NULL src/qemu/qemu_blockjob.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) -- 2.30.2
When doing a full block pull job (base == NULL) and the config XML contains a compatible disk, the completer function would leave a dangling pointer in 'cfgdisk->src->backingStore' as cfgdisk->src would be set to the value of 'cfgbase' which was always set to 'cfgdisk->src->backingStore'. This is wrong though since for the live definition XML we set the respective counterpart to 'job->data.pull.base' which is NULL in the above scenario. This leads to a invalid pointer read when saving the config XML and may end up in a crash. Resolve it by setting 'cfgbase' only when 'job->data.pull.base' is non-NULL. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1946918 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 66268a365a..d708fd18fd 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1005,10 +1005,7 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, if (!job->disk) return; - if ((cfgdisk = qemuBlockJobGetConfigDisk(vm, job->disk, job->data.pull.base))) - cfgbase = cfgdisk->src->backingStore; - - if (!cfgdisk) + if (!(cfgdisk = qemuBlockJobGetConfigDisk(vm, job->disk, job->data.pull.base))) qemuBlockJobClearConfigChain(vm, job->disk); qemuBlockJobProcessEventCompletedPullBitmaps(vm, job, asyncJob); @@ -1018,6 +1015,8 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, return; if (job->data.pull.base) { + if (cfgdisk) + cfgbase = cfgdisk->src->backingStore; for (n = job->disk->src->backingStore; n && n != job->data.pull.base; n = n->backingStore) { /* find the image on top of 'base' */ -- 2.30.2
When doing a blockpull with NULL base the full contents of the disk are pulled into the topmost image which then becomes fullu self-contained. qemuBlockJobProcessEventCompletedPull doesn't install the backing chain terminators though, although it's guaranteed that there will be no backing chain behind disk->src. Add the terminators for completness and for disabling backing chain detection on further boots. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index d708fd18fd..d2a769136d 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -992,6 +992,7 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, qemuBlockJobDataPtr job, qemuDomainAsyncJob asyncJob) { + virStorageSource *base = NULL; virStorageSourcePtr baseparent = NULL; virDomainDiskDefPtr cfgdisk = NULL; virStorageSourcePtr cfgbase = NULL; @@ -1015,8 +1016,11 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, return; if (job->data.pull.base) { + base = job->data.pull.base; + if (cfgdisk) cfgbase = cfgdisk->src->backingStore; + for (n = job->disk->src->backingStore; n && n != job->data.pull.base; n = n->backingStore) { /* find the image on top of 'base' */ @@ -1027,10 +1031,17 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, baseparent = n; } + } else { + /* create terminators for the chain; since we are pulling everything + * into the top image the chain is automatically considered terminated */ + base = virStorageSourceNew(); + + if (cfgdisk) + cfgbase = virStorageSourceNew(); } tmp = job->disk->src->backingStore; - job->disk->src->backingStore = job->data.pull.base; + job->disk->src->backingStore = base; if (baseparent) baseparent->backingStore = NULL; qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, tmp); -- 2.30.2
On Mon, Apr 12, 2021 at 05:57:46PM +0200, Peter Krempa wrote:
When doing a blockpull with NULL base the full contents of the disk are pulled into the topmost image which then becomes fullu self-contained.
s/fullu/fully/
qemuBlockJobProcessEventCompletedPull doesn't install the backing chain terminators though, although it's guaranteed that there will be no backing chain behind disk->src.
Add the terminators for completness and for disabling backing chain detection on further boots.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index d708fd18fd..d2a769136d 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -992,6 +992,7 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, qemuBlockJobDataPtr job, qemuDomainAsyncJob asyncJob) { + virStorageSource *base = NULL; virStorageSourcePtr baseparent = NULL; virDomainDiskDefPtr cfgdisk = NULL; virStorageSourcePtr cfgbase = NULL; @@ -1015,8 +1016,11 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, return;
if (job->data.pull.base) { + base = job->data.pull.base; + if (cfgdisk) cfgbase = cfgdisk->src->backingStore; + for (n = job->disk->src->backingStore; n && n != job->data.pull.base; n = n->backingStore) { /* find the image on top of 'base' */
@@ -1027,10 +1031,17 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
baseparent = n; } + } else { + /* create terminators for the chain; since we are pulling everything + * into the top image the chain is automatically considered terminated */ + base = virStorageSourceNew(); + + if (cfgdisk) + cfgbase = virStorageSourceNew(); }
tmp = job->disk->src->backingStore; - job->disk->src->backingStore = job->data.pull.base; + job->disk->src->backingStore = base; if (baseparent) baseparent->backingStore = NULL; qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, tmp); -- 2.30.2
On Mon, Apr 12, 2021 at 05:57:44PM +0200, Peter Krempa wrote:
Peter Krempa (2): qemuBlockJobProcessEventCompletedPull: Avoid dangling pointer after blockpull qemuBlockJobProcessEventCompletedPull: Add backingStore terminators if base is NULL
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa