Hello,
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.
Fixing them properly requires some infrastructure,
so that you don't end up duplicating too much logic.
This patch adds part of that infrastructure and fixes only a single
instance, to start with. I'll fix the others once we're all agreed
on the form of the infrastructure. I've fixed the bug that would make
this command:
echo domname $(echo 2^32+2|bc)|src/virsh ...
act just like this one:
echo domname 2|src/virsh ...
Now, it does this:
$ echo domname 4294967298|src/virsh --quiet --connect test://$PWD/docs/testnode.xml
virsh > error: failed to get domain '4294967298'
virsh >
The new test script, tests/int-overflow demonstrates
precisely that before/after behavior.
This change adds some other new files.
src/xstrtol.c and .h contain the first of a few new
strtol-like functions. This first one, xstrtol_i,
converts to an "int". Other wrappers will convert to wider types.
The goal is to put the tedious tests into the wrappers so that
applications can be robust without all the duplicated gore.
It is important (from type-safety and maintainability standpoints) to
ensure that the resulting integral value be "returned" via an argument
pointer, not a return value. With the former, you're guaranteed to have
matching types. Simply getting the value via "return", it is too easy
to mistakenly change width or signedness.
Here's the new function's description and signature:
/* 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". */
int xstrtol_i(char const *s, char **end_ptr, int base, int *result)
My first attempt put the definition of xstrtol_i in
the logical place: util.c. But that would have required linking
virsh with util.c, and that fails due to an unsatisfied reference
to __virRaiseError. So instead, it's in its own file, now.
If you want it in some other file, just tell me where.
BTW, I have no strong preference for the name xstrtol_i. Bear in mind
that the name tells you that it's based on (and limited to) strtol's
"long" type, and produces an 'i'nt value. It happens to be the same one
used in at least one other project, and closely resembles the xstrto*
functions in gnulib. Note that there are almost certainly places in the
code where we'll want to use a variant that targets an unsigned type like
"size_t" or a wider type like long long. In addition to other uses of
strtol that I plan to fix, there are three uses of strtoll, too.
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.
---
src/Makefile.am | 2 +-
src/virsh.c | 10 ++++------
src/xstrtol.c | 33 +++++++++++++++++++++++++++++++++
src/xstrtol.h | 7 +++++++
tests/Makefile.am | 10 ++++++++--
tests/int-overflow | 22 ++++++++++++++++++++++
6 files changed, 75 insertions(+), 9 deletions(-)
create mode 100644 src/xstrtol.c
create mode 100644 src/xstrtol.h
create mode 100755 tests/int-overflow
diff --git a/src/Makefile.am b/src/Makefile.am
index e2d6164..ae0b0a8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -67,7 +67,7 @@ libvirt_la_SOURCES = $(CLIENT_SOURCES) $(SERVER_SOURCES)
bin_PROGRAMS = virsh
-virsh_SOURCES = virsh.c console.c console.h
+virsh_SOURCES = virsh.c console.c console.h xstrtol.c xstrtol.h
virsh_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDFLAGS)
virsh_DEPENDENCIES = $(DEPS)
virsh_LDADD = $(LDADDS) $(VIRSH_LIBS)
diff --git a/src/virsh.c b/src/virsh.c
index 5327a28..1f15ee4 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -44,6 +44,7 @@
#include "config.h"
#include "internal.h"
#include "console.h"
+#include "xstrtol.h"
static char *progname;
@@ -2961,10 +2962,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 +3996,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 +4012,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/xstrtol.c b/src/xstrtol.c
new file mode 100644
index 0000000..1f0a8ec
--- /dev/null
+++ b/src/xstrtol.c
@@ -0,0 +1,33 @@
+/*
+ * xstrtol.c: strtol wrappers
+ *
+ * Copyright (C) 2007 Red Hat, Inc.
+ */
+#include <config.h>
+#include <errno.h>
+#include <stdlib.h>
+
+#include "xstrtol.h"
+
+/* 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". */
+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;
+}
diff --git a/src/xstrtol.h b/src/xstrtol.h
new file mode 100644
index 0000000..2bfb812
--- /dev/null
+++ b/src/xstrtol.h
@@ -0,0 +1,7 @@
+/*
+ * xstrtol.h: strtol wrappers
+ *
+ * Copyright (C) 2007 Red Hat, Inc.
+ */
+
+int xstrtol_i(char const *s, char **end_ptr, int base, int *result);
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