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