[Qemu-devel] [PATCH] wrap up use of dangerous ctype.h macros

[Ian Jackson]

The macros isupper, isspace, tolower, and so forth (from <ctype.h>)
are defined to take an int containing the value of an unsigned char,
or EOF.

This means that you mustn't write:
   char *p;
   ...
   if (isspace(*p)) { ...

If *p is a top-bit-set character, and your host architecture has
signed chars by default (most do), then the result is passing a
negative number to isspace, which is not allowed.  glibc permits this
but not all libcs do, and it is wrong according to C89 and C99.

The correct use is:
   if (isspace((unsigned char)*p)) { ...

This is rather unweildy so I in the attached patch have invented a
CTYPE macro which takes some but not all of the pain out of it.

I haven't checked the context of every use I changed to confirm that
the char* is actually a char* rather than an unsigned char*, and I did
not check whether the data is static data which is part of qemu and
guaranteed to contain only 7-bit characters - so technically it is
possible that some of these changes are unnecessary.  However they are
all correct changes and changing it everywhere sets the right example.

Ian.

diff --git a/audio/audio.c b/audio/audio.c
index 2ccef58..653e196 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -235,7 +235,7 @@ static char *audio_alloc_prefix (const char *s)
         strcat (r, s);
 
         for (i = 0; i < len; ++i) {
-            u[i] = toupper (u[i]);
+            u[i] = CTYPE(toupper, u[i]);
         }
     }
     return r;
@@ -489,7 +489,7 @@ static void audio_process_options (const char *prefix,
 
         /* copy while upper-casing, including trailing zero */
         for (i = 0; i <= preflen; ++i) {
-            optname[i + sizeof (qemu_prefix) - 1] = toupper (prefix[i]);
+            optname[i + sizeof (qemu_prefix) - 1] = CTYPE(toupper, prefix[i]);
         }
         strcat (optname, "_");
         strcat (optname, opt->name);
diff --git a/block-vvfat.c b/block-vvfat.c
index ad81425..7fe7e6a 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -1476,7 +1476,7 @@ static int parse_short_name(BDRVVVFATState* s,
        if (direntry->name[i] <= ' ' || direntry->name[i] > 0x7f)
            return -1;
        else if (s->downcase_short_names)
-           lfn->name[i] = tolower(direntry->name[i]);
+           lfn->name[i] = CTYPE(tolower,direntry->name[i]);
        else
            lfn->name[i] = direntry->name[i];
     }
@@ -1489,7 +1489,7 @@ static int parse_short_name(BDRVVVFATState* s,
            if (direntry->extension[j] <= ' ' || direntry->extension[j] > 0x7f)
                return -2;
            else if (s->downcase_short_names)
-               lfn->name[i + j] = tolower(direntry->extension[j]);
+               lfn->name[i + j] = CTYPE(tolower,direntry->extension[j]);
            else
                lfn->name[i + j] = direntry->extension[j];
        }
diff --git a/cutils.c b/cutils.c
index 9ef2fa6..e8e022c 100644
--- a/cutils.c
+++ b/cutils.c
@@ -72,7 +72,7 @@ int stristart(const char *str, const char *val, const char 
**ptr)
     p = str;
     q = val;
     while (*q != '\0') {
-        if (toupper(*p) != toupper(*q))
+        if (CTYPE(toupper,*p) != CTYPE(toupper,*q))
             return 0;
         p++;
         q++;
diff --git a/monitor.c b/monitor.c
index 63d9fad..8e2187f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1779,7 +1779,7 @@ static void next(void)
 {
     if (pch != '\0') {
         pch++;
-        while (isspace(*pch))
+        while (CTYPE(isspace,*pch))
             pch++;
     }
 }
@@ -1838,7 +1838,7 @@ static int64_t expr_unary(void)
                     *q++ = *pch;
                 pch++;
             }
-            while (isspace(*pch))
+            while (CTYPE(isspace,*pch))
                 pch++;
             *q = 0;
             ret = get_monitor_def(&reg, buf);
@@ -1863,7 +1863,7 @@ static int64_t expr_unary(void)
             expr_error("invalid char in expression");
         }
         pch = p;
-        while (isspace(*pch))
+        while (CTYPE(isspace,*pch))
             pch++;
         break;
     }
@@ -1957,7 +1957,7 @@ static int get_expr(int64_t *pval, const char **pp)
         *pp = pch;
         return -1;
     }
-    while (isspace(*pch))
+    while (CTYPE(isspace,*pch))
         pch++;
     *pval = expr_sum();
     *pp = pch;
@@ -1972,7 +1972,7 @@ static int get_str(char *buf, int buf_size, const char 
**pp)
 
     q = buf;
     p = *pp;
-    while (isspace(*p))
+    while (CTYPE(isspace,*p))
         p++;
     if (*p == '\0') {
     fail:
@@ -2017,7 +2017,7 @@ static int get_str(char *buf, int buf_size, const char 
**pp)
         }
         p++;
     } else {
-        while (*p != '\0' && !isspace(*p)) {
+        while (*p != '\0' && !CTYPE(isspace,*p)) {
             if ((q - buf) < buf_size - 1) {
                 *q++ = *p;
             }
@@ -2052,12 +2052,12 @@ static void monitor_handle_command(const char *cmdline)
     /* extract the command name */
     p = cmdline;
     q = cmdname;
-    while (isspace(*p))
+    while (CTYPE(isspace,*p))
         p++;
     if (*p == '\0')
         return;
     pstart = p;
-    while (*p != '\0' && *p != '/' && !isspace(*p))
+    while (*p != '\0' && *p != '/' && !CTYPE(isspace,*p))
         p++;
     len = p - pstart;
     if (len > sizeof(cmdname) - 1)
@@ -2093,7 +2093,7 @@ static void monitor_handle_command(const char *cmdline)
                 int ret;
                 char *str;
 
-                while (isspace(*p))
+                while (CTYPE(isspace,*p))
                     p++;
                 if (*typestr == '?') {
                     typestr++;
@@ -2134,7 +2134,7 @@ static void monitor_handle_command(const char *cmdline)
             {
                 int count, format, size;
 
-                while (isspace(*p))
+                while (CTYPE(isspace,*p))
                     p++;
                 if (*p == '/') {
                     /* format found */
@@ -2181,7 +2181,7 @@ static void monitor_handle_command(const char *cmdline)
                         }
                     }
                 next:
-                    if (*p != '\0' && !isspace(*p)) {
+                    if (*p != '\0' && !CTYPE(isspace,*p)) {
                         term_printf("invalid char in format: '%c'\n", *p);
                         goto fail;
                     }
@@ -2215,7 +2215,7 @@ static void monitor_handle_command(const char *cmdline)
             {
                 int64_t val;
 
-                while (isspace(*p))
+                while (CTYPE(isspace,*p))
                     p++;
                 if (*typestr == '?' || *typestr == '.') {
                     if (*typestr == '?') {
@@ -2226,7 +2226,7 @@ static void monitor_handle_command(const char *cmdline)
                     } else {
                         if (*p == '.') {
                             p++;
-                            while (isspace(*p))
+                            while (CTYPE(isspace,*p))
                                 p++;
                             has_arg = 1;
                         } else {
@@ -2271,7 +2271,7 @@ static void monitor_handle_command(const char *cmdline)
                 c = *typestr++;
                 if (c == '\0')
                     goto bad_type;
-                while (isspace(*p))
+                while (CTYPE(isspace,*p))
                     p++;
                 has_option = 0;
                 if (*p == '-') {
@@ -2296,7 +2296,7 @@ static void monitor_handle_command(const char *cmdline)
         }
     }
     /* check that all arguments were parsed */
-    while (isspace(*p))
+    while (CTYPE(isspace,*p))
         p++;
     if (*p != '\0') {
         term_printf("%s: extraneous characters at the end of line\n",
@@ -2434,7 +2434,7 @@ static void parse_cmdline(const char *cmdline,
     p = cmdline;
     nb_args = 0;
     for(;;) {
-        while (isspace(*p))
+        while (CTYPE(isspace,*p))
             p++;
         if (*p == '\0')
             break;
@@ -2468,7 +2468,7 @@ void readline_find_completion(const char *cmdline)
     /* if the line ends with a space, it means we want to complete the
        next arg */
     len = strlen(cmdline);
-    if (len > 0 && isspace(cmdline[len - 1])) {
+    if (len > 0 && CTYPE(isspace,cmdline[len - 1])) {
         if (nb_args >= MAX_ARGS)
             return;
         args[nb_args++] = qemu_strdup("");
diff --git a/qemu-common.h b/qemu-common.h
index e8ea687..9284789 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -83,6 +83,17 @@ int strstart(const char *str, const char *val, const char 
**ptr);
 int stristart(const char *str, const char *val, const char **ptr);
 time_t mktimegm(struct tm *tm);
 
+#define CTYPE(isfoobar,argumentchar) (isfoobar((unsigned char)(argumentchar)))
+  /* One must not pass a plain `char' to isupper, toupper, et al.  If
+   * it has the top bit set (ie, is negative if your chars are
+   * signed), undefined behaviour results.  The <ctype.h> functions
+   * are defined to take the value of an unsigned char, as an int.
+   * So use this macro.  You may pass toupper et al for isfoobar.
+   * Do not pass EOF as a character to this macro.  If you might have
+   * EOF then you ought to have it in an int representing an unsigned
+   * char, which is safe for the ctype macros directly.  Or test separately.
+   * Obviously don't use this for floating point things like isnan! */
+
 /* Error handling.  */
 
 void hw_error(const char *fmt, ...)
diff --git a/readline.c b/readline.c
index 81bc491..ad10e1e 100644
--- a/readline.c
+++ b/readline.c
@@ -169,7 +169,7 @@ static void term_backword(void)
 
     /* find first word (backwards) */
     while (start > 0) {
-        if (!isspace(term_cmd_buf[start])) {
+        if (!CTYPE(isspace,term_cmd_buf[start])) {
             break;
         }
 
@@ -178,7 +178,7 @@ static void term_backword(void)
 
     /* find first space (backwards) */
     while (start > 0) {
-        if (isspace(term_cmd_buf[start])) {
+        if (CTYPE(isspace,term_cmd_buf[start])) {
             ++start;
             break;
         }
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index ad12364..6fb4d1a 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -25,6 +25,7 @@
 
 #include "dis-asm.h"
 #include "host-utils.h"
+#include "qemu-common.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -9460,7 +9461,7 @@ const ppc_def_t *cpu_ppc_find_by_name (const unsigned 
char *name)
         p = name;
     check_pvr:
         for (i = 0; i < 8; i++) {
-            if (!isxdigit(*p++))
+            if (!CTYPE(isxdigit,*p++))
                 break;
         }
         if (i == 8)
diff --git a/usb-linux.c b/usb-linux.c
index d8032a8..7f75598 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -719,7 +719,7 @@ static int get_tag_value(char *buf, int buf_size,
     if (!p)
         return -1;
     p += strlen(tag);
-    while (isspace(*p))
+    while (CTYPE(isspace,*p))
         p++;
     q = buf;
     while (*p != '\0' && !strchr(stopchars, *p)) {
diff --git a/vl.c b/vl.c
index 15955db..552d4b1 100644
--- a/vl.c
+++ b/vl.c
@@ -3562,7 +3562,7 @@ int parse_host_port(struct sockaddr_in *saddr, const char 
*str)
     if (buf[0] == '\0') {
         saddr->sin_addr.s_addr = 0;
     } else {
-        if (isdigit(buf[0])) {
+        if (CTYPE(isdigit,buf[0])) {
             if (!inet_aton(buf, &saddr->sin_addr))
                 return -1;
         } else {
@@ -3964,7 +3964,7 @@ int tap_alloc(char *dev)
 
     if( *dev ){
        ptr = dev;
-       while( *ptr && !isdigit((int)*ptr) ) ptr++;
+       while( *ptr && !CTYPE(isdigit,(int)*ptr) ) ptr++;
        ppa = atoi(ptr);
     }