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

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

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 */

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 (xstrtol_i((const char *)obj->stringval, NULL, 10, &port) || port < 0)
-    if (port == -1) {
         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 (xstrtol_i(n, NULL, 10, &id) == 0 && id >= 0) {
-        if (id >= 0 && end && *end == '\0') {
             vshDebug(ctl, 5, "%s: <%s> seems like domain ID\n",
                      cmd->def->name, optname);
             dom = virDomainLookupByID(ctl->conn, id);

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

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) {
+                            if (t->u.s.car->kind == SEXPR_VALUE
-                                cpu = strtol(t->u.s.car->u.value, NULL, 0);
+                                && xstrtol_i(t->u.s.car->u.value, NULL, 10, &cpu) == 0
-                                if (cpu >= 0 && (VIR_CPU_MAPLEN(cpu+1) <= maplen)) {
+                                && cpu >= 0
-                                    VIR_USE_CPU(cpumap, cpu);
+                                && (VIR_CPU_MAPLEN(cpu+1) <= maplen)) {
-                                }
+                                VIR_USE_CPU(cpumap, cpu);
                             }
                         break;
                     }

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>
 

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 = \