I spotted this in existing code while doing a review:
These uses all cause trouble if the byte in question has a value
larger than 127 and the "char" type is signed.
avoid problems with sign-extended "char" operand to is* functions
* src/util.h (to_uchar): Define function.
* src/sexpr.c (_string2sexpr): Apply to_uchar to is* operand.
* src/nodeinfo.c (linuxNodeInfoCPUPopulate): Likewise.
* src/qemu_driver.c (qemudExtractMonitorPath): Likewise.
* src/stats_linux.c (xenLinuxDomainDeviceID): Likewise.
* src/util.c (TOLOWER, __virMacAddrCompare, virParseMacAddr): Likewise.
* src/virsh.c (cmdVcpupin, vshCommandGetToken): Likewise.
I'll post a separate patch to help ensure that there are no regressions.
Signed-off-by: Jim Meyering <meyering@xxxxxxxxxx>
---
src/nodeinfo.c | 12 ++++++------
src/qemu_driver.c | 2 +-
src/sexpr.c | 4 +++-
src/stats_linux.c | 8 ++++----
src/util.c | 8 ++++----
src/util.h | 5 +++++
src/virsh.c | 6 +++---
7 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 77b2072..c1e8013 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1,7 +1,7 @@
/*
* nodeinfo.c: Helper routines for OS specific node information
*
- * Copyright (C) 2006, 2007 Red Hat, Inc.
+ * Copyright (C) 2006, 2007, 2008 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -59,7 +59,7 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n
char *buf = line;
if (STREQLEN(buf, "processor", 9)) { /* aka a single logical CPU */
buf += 9;
- while (*buf && isspace(*buf))
+ while (*buf && isspace(to_uchar(*buf)))
buf++;
if (*buf != ':') {
__virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR,
@@ -72,7 +72,7 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n
char *p;
unsigned int ui;
buf += 9;
- while (*buf && isspace(*buf))
+ while (*buf && isspace(to_uchar(*buf)))
buf++;
if (*buf != ':' || !buf[1]) {
__virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR,
@@ -82,13 +82,13 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n
}
if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0
/* Accept trailing fractional part. */
- && (*p == '\0' || *p == '.' || isspace(*p)))
+ && (*p == '\0' || *p == '.' || isspace(to_uchar(*p))))
nodeinfo->mhz = ui;
} else if (STREQLEN(buf, "cpu cores", 9)) { /* aka cores */
char *p;
unsigned int id;
buf += 9;
- while (*buf && isspace(*buf))
+ while (*buf && isspace(to_uchar(*buf)))
buf++;
if (*buf != ':' || !buf[1]) {
__virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR,
@@ -97,7 +97,7 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n
return -1;
}
if (virStrToLong_ui(buf+1, &p, 10, &id) == 0
- && (*p == '\0' || isspace(*p))
+ && (*p == '\0' || isspace(to_uchar(*p)))
&& id > nodeinfo->cores)
nodeinfo->cores = id;
}
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index b65ae66..eb2f6e8 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -513,7 +513,7 @@ static int qemudExtractMonitorPath(const char *haystack, char *path, int pathmax
* The monitor path ends at first whitespace char
* so lets search for it & NULL terminate it there
*/
- if (isspace(*path)) {
+ if (isspace(to_uchar(*path))) {
*path = '\0';
return 0;
}
diff --git a/src/sexpr.c b/src/sexpr.c
index 9f6ed0d..4cb4edf 100644
--- a/src/sexpr.c
+++ b/src/sexpr.c
@@ -20,6 +20,7 @@
#include "internal.h"
#include "sexpr.h"
+#include "util.h"
/**
* virSexprError:
@@ -357,7 +358,8 @@ _string2sexpr(const char *buffer, size_t * end)
} else {
start = ptr;
- while (*ptr && !isspace(*ptr) && *ptr != ')' && *ptr != '(') {
+ while (*ptr && !isspace(to_uchar(*ptr))
+ && *ptr != ')' && *ptr != '(') {
ptr++;
}
diff --git a/src/stats_linux.c b/src/stats_linux.c
index bf46ca2..e84e24f 100644
--- a/src/stats_linux.c
+++ b/src/stats_linux.c
@@ -1,7 +1,7 @@
/*
* Linux block and network stats.
*
- * Copyright (C) 2007 Red Hat, Inc.
+ * Copyright (C) 2007, 2008 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -262,7 +262,7 @@ xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path)
}
if (path[4] != '\0') {
- if (!isdigit(path[4]) || path[4] == '0' ||
+ if (!isdigit(to_uchar(path[4])) || path[4] == '0' ||
virStrToLong_i(path+4, NULL, 10, &part) < 0 ||
part < 1 || part > 15) {
statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
@@ -306,7 +306,7 @@ xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path)
} else {
p = path + 3;
}
- if (p && (!isdigit(*p) || *p == '0' ||
+ if (p && (!isdigit(to_uchar(*p)) || *p == '0' ||
virStrToLong_i(p, NULL, 10, &part) < 0 ||
part < 1 || part > 15)) {
statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
@@ -332,7 +332,7 @@ xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path)
}
if (path[3] != '\0') {
- if (!isdigit(path[3]) || path[3] == '0' ||
+ if (!isdigit(to_uchar(path[3])) || path[3] == '0' ||
virStrToLong_i(path+3, NULL, 10, &part) < 0 ||
part < 1 || part > 63) {
statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
diff --git a/src/util.c b/src/util.c
index 5a44f18..8f3cef9 100644
--- a/src/util.c
+++ b/src/util.c
@@ -57,7 +57,7 @@
#define MAX_ERROR_LEN 1024
-#define TOLOWER(Ch) (isupper (Ch) ? tolower (Ch) : (Ch))
+#define TOLOWER(Ch) (isupper (to_uchar(Ch)) ? tolower (Ch) : (Ch))
#define virLog(msg...) fprintf(stderr, msg)
@@ -704,9 +704,9 @@ __virMacAddrCompare (const char *p, const char *q)
{
unsigned char c, d;
do {
- while (*p == '0' && isxdigit (p[1]))
+ while (*p == '0' && isxdigit (to_uchar(p[1])))
++p;
- while (*q == '0' && isxdigit (q[1]))
+ while (*q == '0' && isxdigit (to_uchar(q[1])))
++q;
c = TOLOWER (*p);
d = TOLOWER (*q);
@@ -749,7 +749,7 @@ virParseMacAddr(const char* str, unsigned char *addr)
/* This is solely to avoid accepting the leading
* space or "+" that strtoul would otherwise accept.
*/
- if (!isxdigit(*str))
+ if (!isxdigit(to_uchar(*str)))
break;
result = strtoul(str, &end_ptr, 16);
diff --git a/src/util.h b/src/util.h
index 2516504..af68bc8 100644
--- a/src/util.h
+++ b/src/util.h
@@ -27,6 +27,11 @@
#include "internal.h"
#include "util-lib.h"
+/* Convert a possibly-signed character to an unsigned character. This is
+ a bit safer than casting to unsigned char, since it catches some type
+ errors that the cast doesn't. */
+static inline unsigned char to_uchar (char ch) { return ch; }
+
int virExec(virConnectPtr conn, char **argv, int *retpid,
int infd, int *outfd, int *errfd);
int virExecNonBlock(virConnectPtr conn, char **argv, int *retpid,
diff --git a/src/virsh.c b/src/virsh.c
index 772967e..ef6165b 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -1763,7 +1763,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cmd)
for (i = 0; cpulist[i]; i++) {
switch (state) {
case expect_num:
- if (!isdigit (cpulist[i])) {
+ if (!isdigit (to_uchar(cpulist[i]))) {
vshError( ctl, FALSE, _("cpulist: %s: Invalid format. Expecting digit at position %d (near '%c')."), cpulist, i, cpulist[i]);
virDomainFree (dom);
return FALSE;
@@ -1773,7 +1773,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cmd)
case expect_num_or_comma:
if (cpulist[i] == ',')
state = expect_num;
- else if (!isdigit (cpulist[i])) {
+ else if (!isdigit (to_uchar(cpulist[i]))) {
vshError(ctl, FALSE, _("cpulist: %s: Invalid format. Expecting digit or comma at position %d (near '%c')."), cpulist, i, cpulist[i]);
virDomainFree (dom);
return FALSE;
@@ -5685,7 +5685,7 @@ vshCommandGetToken(vshControl * ctl, char *str, char **end, char **res)
if (tk == VSH_TK_NONE) {
if (*p == '-' && *(p + 1) == '-' && *(p + 2)
- && isalnum((unsigned char) *(p + 2))) {
+ && isalnum(to_uchar(*(p + 2)))) {
tk = VSH_TK_OPTION;
p += 2;
} else {
--
1.5.5.1.69.g2ffd
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list