[libvirt] [PATCH] qemu: Fix two issues in qemuDomainSetVcpus error handling

[Martin Kletzander]

On Wed, Mar 18, 2015 at 07:25:29AM -0400, John Ferlan wrote:
>Issue #1 - A call to virBitmapNew did not check if the allocation
>failed which could lead to a NULL dereference
>
>Issue #2 - When deleting the pin entries from the config file, the
>code loops from the number of elements down to the "new" vcpu count;
>however, the pin id values are numbered 0..n-1 not 1..n, so the "first"
>pin attempt would never work. Luckily the check was for whether the
>incoming 'n' (vcpu id) matched the entry in the array from 0..arraysize
>rather than a dereference of the 'n' entry
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
>
>NOTE: These were found on inspection while working/debugging the a IOThreads
>series which borrows from the SetVcpus code.  I can separate the two if
>desired, but I think the second issue is mostly an optimization.
>

If you want to, but in this particular case I think it has no difference.

>
> src/qemu/qemu_driver.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index ed6764d..6d9217b 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -4752,7 +4752,11 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>                 if (VIR_ALLOC(vcpupin) < 0)
>                     goto cleanup;
>
>-                vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN);
>+                if (!(vcpupin->cpumask =
>+                      virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) {
>+                    VIR_FREE(vcpupin);
>+                    goto cleanup;
>+                }
>                 virBitmapCopy(vcpupin->cpumask, vm->def->cpumask);
>                 vcpupin->id = i;
>                 if (VIR_APPEND_ELEMENT_COPY(vm->def->cputune.vcpupin,
>@@ -4987,7 +4991,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
>         if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>             /* remove vcpupin entries for vcpus that were unplugged */
>             if (nvcpus < persistentDef->vcpus) {
>-                for (i = persistentDef->vcpus; i >= nvcpus; i--)
>+                for (i = persistentDef->vcpus - 1; i >= nvcpus; i--)

for (i = nvcpus; i < persistentDef->vcpus, i++)

would be easily readable (I guess the construct in the code might be
just a leftover when it was an array).

ACK with any of the versions.

>                     virDomainPinDel(&persistentDef->cputune.vcpupin,
>                                     &persistentDef->cputune.nvcpupin,
>                                     i);
>--
>2.1.0
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150318/385a24fa/attachment-0001.sig>