[libvirt] [PATCH]: Fix allocation of tapfds when starting qemu

[Jim Meyering]

Chris Lalancette <clalance at redhat.com> wrote:
> All,
>      When doing the conversion to danpb's new memory API, a small bug was
> introduced into the qemudNetworkIfaceConnect() function.  In particular, there
> is a call:
>
>     if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0)
>         goto no_memory;
>
> However, the tapfds structure is used to track *all* of the tap fds, and is
> called once for each network that is being attached to the domain.  VIR_ALLOC_N
> maps to calloc().  So the first network would work just fine, but if you had
> more than one network, subsequent calls to this function would blow away the
> stored fd's that were already there and fill them all in with zeros.  This
> causes multiple problems, from the qemu domains not starting properly to
> improper cleanup on shutdown.  The attached patch just changes the VIR_ALLOC_N()
> to a VIR_REALLOC_N(), and everything is happy again.
>
> Signed-off-by: Chris Lalancette <clalance at redhat.com>
> Index: src/qemu_conf.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/qemu_conf.c,v
> retrieving revision 1.78
> diff -u -r1.78 qemu_conf.c
> --- a/src/qemu_conf.c	13 Jun 2008 07:56:59 -0000	1.78
> +++ b/src/qemu_conf.c	19 Jun 2008 10:01:53 -0000
> @@ -2317,7 +2317,7 @@
>      if (!(retval = strdup(tapfdstr)))
>          goto no_memory;
>
> -    if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0)
> +    if (VIR_REALLOC_N(vm->tapfds, vm->ntapfds+2) < 0)
>          goto no_memory;
>
>      vm->tapfds[vm->ntapfds++] = tapfd;

Yow.  Another good catch.
That's obviously the right fix.
ACK.

I went looking for similar bugs and actually found one!

  $ g show d3470efcda15f59549ac0aaa76cd25df319c217b \
    |grep -A2 realloc|grep VIR_ALLOC
  +    if (VIR_ALLOC_N(vm->tapfds, vm->ntapfds+2) < 0)
  +            if (VIR_ALLOC_N(buf, alloc) < 0) {

That's in the fread_file_lim function, and the fix is the same.
To demonstrate, make virsh read a file containing more than
BUFSIZ bytes, e.g.,

Create a legit definition, but with enough spaces
at the end to push the size over the limit:

  { ./virsh -c test:///default dumpxml 1; printf %9000s ' ' } > t

Demonstrate that virsh-0.4.2 reads/parses it fine:

    $ virsh --version
    0.4.2
    $ virsh --connect test:///default define t
    Domain test defined from t

Show that just-built (pre-patch) virsh fails:

    $ ./virsh --version
    0.4.3
    $ ./virsh --connect test:///default define t
    libvir: Test error : XML description for domain is not well formed or invalid    error: Failed to define domain from t

    [Exit 1]

Show that patched, it works fine:

    $ ./virsh --connect test:///default define t
    Domain test defined from t
    $

Here's the patch I'll push soon:

>From 02172b2c2e529a0afd04d5880ff8f32ad480ed9d Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Thu, 19 Jun 2008 12:36:36 +0200
Subject: [PATCH] virsh fails to read files larger than BUFSIZ bytes

* src/util.c (fread_file_lim): Use VIR_REALLOC_N, not VIR_ALLOC_N.
Bug introduced in d3470efcda15f59549ac0aaa76cd25df319c217b
---
 src/util.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/util.c b/src/util.c
index ad7683d..5e50ef2 100644
--- a/src/util.c
+++ b/src/util.c
@@ -306,7 +306,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length)
             if (alloc < size + BUFSIZ + 1)
                 alloc = size + BUFSIZ + 1;

-            if (VIR_ALLOC_N(buf, alloc) < 0) {
+            if (VIR_REALLOC_N(buf, alloc) < 0) {
                 save_errno = errno;
                 break;
             }
@@ -797,4 +797,3 @@ int virDiskNameToIndex(const char *name) {

     return idx;
 }
-
--
1.5.6.rc3.23.gc3bdd