[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Jim Meyering <jim meyering net> wrote:
> There are over 30 uses of strtol in libvirt, and they all can silently
> accept invalid input.  The invalid string might range from an outlandish
> domain ID like 4294967298 to strings of digits followed by bogus alpha.
> Maybe not worth worrying about, you say?  But what if they indicate user
> confusion, e.g., 1,000 vs 1000?  Silently interpreting "1,000" as "1"
> would leave the poor user even more confused :-)  IMHO, they should all
> be diagnosed.
...
> Patch attached below.
> If you apply it with plain-old-patch, remember to run this:
>
> chmod a+x tests/int-overflow
>
> Thu Nov  8 09:59:43 CET 2007 Jim Meyering  <meyering redhat com>
>
> 	Diagnose an invalid domain ID number.
>
> 	* src/virsh.c: Include "xstrtol.h"
> 	(vshCommandOptDomainBy): Detect integer overflow in domain ID number.
> 	* tests/int-overflow: New script. Test for the above-fixed bug.
> 	* tests/Makefile.am (TESTS): Add int-overflow.
> 	(TESTS_ENVIRONMENT): Define, to propagate $abs_top_* variables
> 	into the int-overflow script.
> 	(valgrind): Adapt rule not to clobber new TESTS_ENVIRONMENT.
> 	* src/xstrtol.h, src/xstrtol.c: New files.
> 	* src/Makefile.am (virsh_SOURCES): Add xstrtol.c and xstrtol.h.

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 redhat com>

	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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]