Begin fixing uses of strtol: parse integers more carefully.

Patch from Jim Meyering
* 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.
Daniel

diff --git a/ChangeLog b/ChangeLog
index c316d17b2707f95b34bd7a5ad74e8e5de682f9a2..2d5a228ee4e04bc152e30d1da7dc1a4ed5469d41 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+Mon Nov 12 14:56:33 CET 2007 Daniel Veillard <veillard@redhat.com>
+
+       Begin fixing uses of strtol: parse integers more carefully.
+       Patch from Jim Meyering
+       * 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.
+
 Thu Nov  8 19:06:13 CET 2007 Daniel Veillard <veillard@redhat.com>
 
        * src/virsh.c: initialize a couple of variable to avoid warnings

diff --git a/src/internal.h b/src/internal.h
index dadb25bcdb43e402c4b8559dd828d54ceb578620..4a31ea6c96bc966a6dadc54e131afacce62c4119 100644 (file)
--- 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 5327a281bd8421b0743ff7fbd180026ec82e1127..5b5052496271e025bb53a52cec23644282946095 100644 (file)
--- 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 3ffac1185f5f183af23519f3a20cc15edfc70e2a..1fa8f3bf4b8a6732ef6c4073af8a8b99a73d0a8b 100644 (file)
--- a/src/xen_internal.c
+++ b/src/xen_internal.c
@@ -33,7 +33,6 @@
 /* required for dom0_getdomaininfo_t */
 #include <xen/dom0_ops.h>
 #include <xen/version.h>
-#include <xen/xen.h>
 #ifdef HAVE_XEN_LINUX_PRIVCMD_H
 #include <xen/linux/privcmd.h>
 #else

diff --git a/src/xend_internal.c b/src/xend_internal.c
index 42242d7a5487b0989f0db05e1d26eabbea2afa1c..c7425f65ed960f69ce478c6717b34159b1e1b936 100644 (file)
--- 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 fda16d251789d103be016f8474f2f9069392e390..9b8efbe7fd0290e36ccc4661290fb9e740537dbf 100644 (file)
--- a/src/xs_internal.c
+++ b/src/xs_internal.c
@@ -19,9 +19,9 @@
 
 #include <stdint.h>
 
-#include <xen/dom0_ops.h>
+/* #include <xen/dom0_ops.h> test DV */
 #include <xen/version.h>
-#include <xen/xen.h>
+/* #include <xen/xen.h> test DV */
 
 #include <xs.h>
 

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 80692e0c17685aeca28673bc9279729baa8b69a1..8a472f86f6731337f28625d4b78e527ce68fc5e2 100644 (file)
--- 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 = \