Jim Meyering <jim@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list