Chris Lalancette <clalance@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list