diff --git a/ChangeLog b/ChangeLog index c316d17b27..2d5a228ee4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,20 @@ +Mon Nov 12 14:56:33 CET 2007 Daniel Veillard + + Begin fixing uses of strtol: parse integers more carefully. + Patch from Jim Meyering + * src/internal.h: Include . + 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. + Thu Nov 8 19:06:13 CET 2007 Daniel Veillard * src/virsh.c: initialize a couple of variable to avoid warnings diff --git a/src/internal.h b/src/internal.h index dadb25bcdb..4a31ea6c96 100644 --- a/src/internal.h +++ b/src/internal.h @@ -5,6 +5,7 @@ #ifndef __VIR_INTERNAL_H__ #define __VIR_INTERNAL_H__ +#include #include #include #include @@ -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 5327a281bd..5b50524962 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/xen_internal.c b/src/xen_internal.c index 3ffac1185f..1fa8f3bf4b 100644 --- a/src/xen_internal.c +++ b/src/xen_internal.c @@ -33,7 +33,6 @@ /* required for dom0_getdomaininfo_t */ #include #include -#include #ifdef HAVE_XEN_LINUX_PRIVCMD_H #include #else diff --git a/src/xend_internal.c b/src/xend_internal.c index 42242d7a54..c7425f65ed 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/src/xs_internal.c b/src/xs_internal.c index fda16d2517..9b8efbe7fd 100644 --- a/src/xs_internal.c +++ b/src/xs_internal.c @@ -19,9 +19,9 @@ #include -#include +/* #include test DV */ #include -#include +/* #include test DV */ #include diff --git a/tests/Makefile.am b/tests/Makefile.am index 80692e0c17..8a472f86f6 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 = \