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

On 08/07/2014 01:58 AM, Peter Krempa wrote: > 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 we go to cleanup here... >> - 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; ...then we miss the restore of disk->src here... >> + } >> >> /* 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. ...so the cleanup label is not dead code. > > ACK with that part removed. Is the patch okay as-is, with my explanation?