Chris Lalancette <clalance(a)redhat.com&gt; wrote: 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: Jim Meyering <meyering(a)redhat.com&gt; 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

Chris Lalancette <clalance(a)redhat.com&gt; wrote: Thanks for the quick review. I've gone ahead and added a small test script to exercise the bug, so the following is what I now expect to commit. From: Jim Meyering <meyering(a)redhat.com&gt; Date: Thu, 19 Jun 2008 13:41:23 +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. * tests/Makefile.am (test_scripts): Add read-bufsiz. * tests/read-bufsiz: New test for the above. --- src/util.c | 3 +-- tests/Makefile.am | 1 + tests/read-bufsiz | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) create mode 100755 tests/read-bufsiz 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; } - diff --git a/tests/Makefile.am b/tests/Makefile.am index 303388c..4021a39 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -47,6 +47,7 @@ noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \ test_scripts = \ daemon-conf \ int-overflow \ + read-bufsiz \ read-non-seekable \ vcpupin diff --git a/tests/read-bufsiz b/tests/read-bufsiz new file mode 100755 index 0000000..3037452 --- /dev/null +++ b/tests/read-bufsiz @@ -0,0 +1,43 @@ +#!/bin/sh +# ensure that reading a file larger than BUFSIZ works + +# Copyright (C) 2008 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>;. + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +. $srcdir/test-lib.sh + +fail=0 + +# Output a valid definition, to be used as input. +virsh -c test:///default dumpxml 1 > xml || fail=1 + +for i in before after; do + # The largest BUFSIZ I've seen is 128K. This is slightly larger. + printf %132000s ' ' > sp || fail=1 + in=in-$i + # Append or prepend enough spaces to push the size over the limit: + ( test $i = before && cat sp xml || cat xml sp ) > $in || fail=1 + + virsh --connect test:///default define $in > out || fail=1 + printf "Domain test defined from $in\n\n" > exp || fail=1 + compare out exp || fail=1 +done + +(exit $fail); exit $fail -- 1.5.6.rc3.23.gc3bdd