Jim Meyering <jim(a)meyering.net&gt; wrote: ... Daniel Veillard suggested to put the definition of xstrtol_i in a header file, so that it can be used both by virsh.c and by the library itself, so now it's in src/internal.h. I've added a fix for one strtol use in the library, in xend_internal.c. Finally, I've adjusted the ChangeLog to more closely match Daniel's preference. Thu Nov 8 09:59:43 CET 2007 Jim Meyering <meyering(a)redhat.com&gt; Begin fixing uses of strtol: parse integers more carefully. * src/internal.h: Include <errno.h>. Define new static inline function, xstrtol_i. * src/virsh.c: Detect integer overflow in domain ID number in vshCommandOptDomainBy. Detect overflow and invalid port number suffix in cmdVNCDisplay. * src/xend_internal.c: Parse CPU number more carefully in xenDaemonDomainGetVcpus. * tests/int-overflow: New script. Test for the above-fixed bug. * tests/Makefile.am: Add int-overflow to TESTS. Define TESTS_ENVIRONMENT, to propagate $abs_top_* variables into the int-overflow script. Adapt the "valgrind" rule not to clobber new TESTS_ENVIRONMENT. diff --git a/src/internal.h b/src/internal.h index dadb25b..4a31ea6 100644 --- a/src/internal.h +++ b/src/internal.h @@ -5,6 +5,7 @@ #ifndef __VIR_INTERNAL_H__ #define __VIR_INTERNAL_H__ +#include <errno.h> #include <sys/types.h> #include <sys/socket.h> #include <sys/un.h> @@ -239,6 +240,30 @@ int __virDomainMigratePrepare (virConnectPtr dconn, char **cookie, int *cookiele int __virDomainMigratePerform (virDomainPtr domain, const char *cookie, int cookielen, const char *uri, unsigned long flags, const char *dname, unsigned long bandwidth); virDomainPtr __virDomainMigrateFinish (virConnectPtr dconn, const char *dname, const char *cookie, int cookielen, const char *uri, unsigned long flags); +/* Like strtol, but produce an "int" result, and check more carefully. + Return 0 upon success; return -1 to indicate failure. + When END_PTR is NULL, the byte after the final valid digit must be NUL. + Otherwise, it's like strtol and lets the caller check any suffix for + validity. This function is careful to return -1 when the string S + represents a number that is not representable as an "int". */ +static inline int +xstrtol_i(char const *s, char **end_ptr, int base, int *result) +{ + long int val; + char *p; + int err; + + errno = 0; + val = strtol(s, &p, base); + err = (errno || (!end_ptr && *p) || p == s || (int) val != val); + if (end_ptr) + *end_ptr = p; + if (err) + return -1; + *result = val; + return 0; +} + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/src/virsh.c b/src/virsh.c index 5327a28..5b50524 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -2961,10 +2961,8 @@ cmdVNCDisplay(vshControl * ctl, vshCmd * cmd) (obj->stringval == NULL) || (obj->stringval[0] == 0)) { goto cleanup; } - port = strtol((const char *)obj->stringval, NULL, 10); - if (port == -1) { + if (xstrtol_i((const char *)obj->stringval, NULL, 10, &port) || port < 0) goto cleanup; - } xmlXPathFreeObject(obj); obj = xmlXPathEval(BAD_CAST "string(/domain/devices/graphics[@type='vnc']/@listen)", ctxt); @@ -3997,7 +3995,7 @@ vshCommandOptDomainBy(vshControl * ctl, vshCmd * cmd, const char *optname, char **name, int flag) { virDomainPtr dom = NULL; - char *n, *end = NULL; + char *n; int id; if (!(n = vshCommandOptString(cmd, optname, NULL))) { @@ -4013,8 +4011,7 @@ vshCommandOptDomainBy(vshControl * ctl, vshCmd * cmd, const char *optname, /* try it by ID */ if (flag & VSH_BYID) { - id = (int) strtol(n, &end, 10); - if (id >= 0 && end && *end == '\0') { + if (xstrtol_i(n, NULL, 10, &id) == 0 && id >= 0) { vshDebug(ctl, 5, "%s: <%s> seems like domain ID\n", cmd->def->name, optname); dom = virDomainLookupByID(ctl->conn, id); diff --git a/src/xend_internal.c b/src/xend_internal.c index 42242d7..c7425f6 100644 --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -3038,11 +3038,11 @@ xenDaemonDomainGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, !strcmp(t->u.s.car->u.s.car->u.value, "cpumap") && (t->u.s.car->u.s.cdr->kind == SEXPR_CONS)) { for (t = t->u.s.car->u.s.cdr->u.s.car; t->kind == SEXPR_CONS; t = t->u.s.cdr) - if (t->u.s.car->kind == SEXPR_VALUE) { - cpu = strtol(t->u.s.car->u.value, NULL, 0); - if (cpu >= 0 && (VIR_CPU_MAPLEN(cpu+1) <= maplen)) { - VIR_USE_CPU(cpumap, cpu); - } + if (t->u.s.car->kind == SEXPR_VALUE + && xstrtol_i(t->u.s.car->u.value, NULL, 10, &cpu) == 0 + && cpu >= 0 + && (VIR_CPU_MAPLEN(cpu+1) <= maplen)) { + VIR_USE_CPU(cpumap, cpu); } break; } diff --git a/tests/Makefile.am b/tests/Makefile.am index 80692e0..8a472f8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -38,13 +38,19 @@ noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \ nodeinfotest TESTS = xml2sexprtest sexpr2xmltest virshtest test_conf.sh xmconfigtest \ - xencapstest qemuxml2argvtest qemuxml2xmltest nodeinfotest + xencapstest qemuxml2argvtest qemuxml2xmltest nodeinfotest \ + int-overflow if ENABLE_XEN_TESTS TESTS += reconnect endif +TESTS_ENVIRONMENT = \ + abs_top_builddir='$(abs_top_builddir)' \ + abs_top_srcdir='$(abs_top_srcdir)' \ + $(VG) + valgrind: - $(MAKE) check TESTS_ENVIRONMENT="valgrind --quiet --leak-check=full" + $(MAKE) check VG="valgrind --quiet --leak-check=full" # Note: xmlrpc.[c|h] is not in libvirt yet xmlrpctest_SOURCES = \ diff --git a/tests/int-overflow b/tests/int-overflow new file mode 100755 index 0000000..97a1ab2 --- /dev/null +++ b/tests/int-overflow @@ -0,0 +1,22 @@ +#!/bin/bash +# Ensure that an invalid domain ID isn't interpreted as a valid one. +# Before, an ID of 2^32+2 would be treated just like an ID of 2. + +# Boilerplate code to set up a test directory, cd into it, +# and to ensure we remove it upon completion. +this_test_() { echo "./$0" | sed 's,.*/,,'; } +t_=$(this_test_)-$$ +init_cwd_=$(pwd) +trap 'st=$?; d='"$t_"'; + cd '"$init_cwd_"' && chmod -R u+rwx "$d" && rm -rf "$d" && exit $st' 0 +trap '(exit $?); exit $?' 1 2 13 15 +mkdir "$t_" || fail=1 +cd "$t_" || fail=1 + +echo "error: failed to get domain '4294967298'" > exp || fail=1 +echo domname 4294967298 | $abs_top_builddir/src/virsh --quiet \ + --connect test://$abs_top_srcdir/docs/testnode.xml \ + > /dev/null 2> err || fail=1 +diff -u err exp || fail=1 + +exit $fail -- 1.5.3.5.602.g53d1

On Fri, Nov 09, 2007 at 10:30:15PM +0100, Jim Meyering wrote: I've had to make a change to this rule. automake-1.9 which is in Fedora 6, RHEL-5 and Debian does not provide any abs_* variable definitions at all. These were new in automake-1.10 The change I applied was this: --- tests/Makefile.am 15 Nov 2007 13:04:28 -0000 1.26 +++ tests/Makefile.am 17 Nov 2007 13:05:08 -0000 @@ -53,8 +53,8 @@ if ENABLE_XEN_TESTS endif TESTS_ENVIRONMENT = \ - abs_top_builddir='$(abs_top_builddir)' \ - abs_top_srcdir='$(abs_top_srcdir)' \ + abs_top_builddir=`pwd`/$(top_builddir) \ + abs_top_srcdir=`pwd`/$(top_srcdir) \ $(VG) valgrind: I checked regular & VPATH builds on FC6, RHEL-5 and F8 and they all now work again with this fix Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: ... Hi Dan, That looks fine, but it'd be nice to retain the single quotes: diff --git a/tests/Makefile.am b/tests/Makefile.am index 8539779..d059402 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -56,8 +56,8 @@ endif # abs_top_{src/build}dir variables, so don't rely # on them here. Fake them with 'pwd' TESTS_ENVIRONMENT = \ - abs_top_builddir=`pwd`/$(top_builddir) \ - abs_top_srcdir=`pwd`/$(top_srcdir) \ + abs_top_builddir=`pwd`/'$(top_builddir)' \ + abs_top_srcdir=`pwd`/'$(top_srcdir)' \ $(VG) valgrind: They protect against the unusual case in which one does a VPATH (aka non-srcdir) build, where configure is specified via a name containing spaces or shell meta-characters. I'll commit the above on Monday if no one else does it first. ---------- On the portability-to-old-automake front (automake-1.9 is over 3 years old), bear in mind that people building from checked out sources should try hard to use relatively recent versions of autoconf and automake, especially developers and packagers. There are enough portability bugs in automake-1.9 that you don't want to use it in development, and certainly not when building a tarball that might be distributed. That goes double for autoconf. Thanks for checking.