Re: [libvirt] [PATCH 2/2] blockjob: fix use-after-free in blockcopy

On 08/06/14 23:12, Eric Blake wrote: > Commit febf84c2 tried to delay in-memory modification of the actual > domain disk structure until after the qemu event was received. > However, I missed that the code for block pivot had been temporarily > setting disk->src = disk->mirror prior to the qemu command, in order > to label the backing chain of a reused external blockcopy disk; > and calls into qemu while still in that state before finally undoing > things at the cleanup label. Since the qemu event handler then does: > virStorageSourceFree(disk->src); > disk->src = disk->mirror; > we have the sad race that a fast enough qemu event can cause a leak of > the original disk->src, as well as a use-after-free of the disk->mirror > contents, bad enough to crash libvirtd in some of my test runs, even > though the common case of the qemu event being much later won't trip > the race. >

> > - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) > - goto cleanup; > + if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) > + goto cleanup; >

> - if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && > - (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || > - qemuSetupDiskCgroup(vm, disk) < 0 || > - virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, disk) < 0)) > - goto cleanup; > + if (disk->mirror->format && > + disk->mirror->format != VIR_STORAGE_FILE_RAW && > + (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, > + disk) < 0 || > + qemuSetupDiskCgroup(vm, disk) < 0 || > + virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, > + disk) < 0)) > + goto cleanup; > + > + disk->src = oldsrc; > + oldsrc = NULL;

> + } > > /* Attempt the pivot. Record the attempt now, to prevent duplicate > * attempts; but the actual disk change will be made when emitting > In the cleanup section there's the original place where oldsrc was returned back to disk->src. That would now be dead code.

ACK with that part removed.