Re: [Libvir] diagnose invalid domain ID numbers (and invalid integers, in general)

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