[Qemu-devel] [PATCH] [RESEND] 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 and error-prone so I in the attached patch
have invented a CTYPE macro which takes some but not all of the pain
out of it.

Technically it is possible that some of these changes are unnecessary
as 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.  However they are all
correct changes and changing it everywhere sets the right example.

This change gets rid of compiler warnings on some platforms, too
(generally, correct compiler warnings which are reporting exactly the
problem I am fixing here).

Ian.


Use new CTYPE macro for <ctype.h>

This avoids passing a negative int to isfoobar,
which is not allowed

Signed-off-by: Ian Jackson <address@hidden>
---
 audio/audio.c               |    4 ++--
 block-vvfat.c               |    6 +++---
 cutils.c                    |    2 +-
 monitor.c                   |   38 +++++++++++++++++++-------------------
 nbd.c                       |   16 ++++++++--------
 qemu-common.h               |   10 ++++++++++
 readline.c                  |    4 ++--
 target-ppc/translate_init.c |    3 ++-
 usb-linux.c                 |    2 +-
 vl.c                        |    4 ++--
 10 files changed, 50 insertions(+), 39 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 20bb2fc..ba14066 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -215,7 +215,7 @@ static char *audio_alloc_prefix (const char *s)
         pstrcat (r, len, s);
 
         for (i = 0; i < len; ++i) {
-            u[i] = toupper (u[i]);
+            u[i] = CTYPE(toupper, u[i]);
         }
     }
     return r;
@@ -471,7 +471,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]);
         }
         pstrcat (optname, optlen, "_");
         optlen--;
diff --git a/block-vvfat.c b/block-vvfat.c
index 79804a7..2b5b0b4 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -1052,7 +1052,7 @@ DLOG(if (stderr == NULL) {
 
     i = strrchr(dirname, ':') - dirname;
     assert(i >= 3);
-    if (dirname[i-2] == ':' && isalpha(dirname[i-1]))
+    if (dirname[i-2] == ':' && CTYPE(isalpha,dirname[i-1]))
        /* workaround for DOS drive names */
        dirname += i-1;
     else
@@ -1481,7 +1481,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];
     }
@@ -1494,7 +1494,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 e71f49e..7907246 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1900,7 +1900,7 @@ static void next(void)
 {
     if (pch != '\0') {
         pch++;
-        while (isspace(*pch))
+        while (CTYPE(isspace,*pch))
             pch++;
     }
 }
@@ -1959,7 +1959,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);
@@ -1984,7 +1984,7 @@ static int64_t expr_unary(void)
             expr_error("invalid char in expression");
         }
         pch = p;
-        while (isspace(*pch))
+        while (CTYPE(isspace,*pch))
             pch++;
         break;
     }
@@ -2078,7 +2078,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;
@@ -2093,7 +2093,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:
@@ -2138,7 +2138,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;
             }
@@ -2184,12 +2184,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)
@@ -2225,7 +2225,7 @@ static void monitor_handle_command(const char *cmdline)
                 int ret;
                 char *str;
 
-                while (isspace(*p))
+                while (CTYPE(isspace,*p))
                     p++;
                 if (*typestr == '?') {
                     typestr++;
@@ -2266,15 +2266,15 @@ static void monitor_handle_command(const char *cmdline)
             {
                 int count, format, size;
 
-                while (isspace(*p))
+                while (CTYPE(isspace,*p))
                     p++;
                 if (*p == '/') {
                     /* format found */
                     p++;
                     count = 1;
-                    if (isdigit(*p)) {
+                    if (CTYPE(isdigit,*p)) {
                         count = 0;
-                        while (isdigit(*p)) {
+                        while (CTYPE(isdigit,*p)) {
                             count = count * 10 + (*p - '0');
                             p++;
                         }
@@ -2313,7 +2313,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;
                     }
@@ -2347,7 +2347,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 == '?') {
@@ -2358,7 +2358,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 {
@@ -2403,7 +2403,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 == '-') {
@@ -2428,7 +2428,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",
@@ -2576,7 +2576,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;
@@ -2610,7 +2610,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/nbd.c b/nbd.c
index 9bebe4a..5197e15 100644
--- a/nbd.c
+++ b/nbd.c
@@ -310,14 +310,14 @@ int nbd_receive_negotiate(int csock, off_t *size, size_t 
*blocksize)
        *blocksize = 1024;
 
        TRACE("Magic is %c%c%c%c%c%c%c%c",
-             isprint(buf[0]) ? buf[0] : '.',
-             isprint(buf[1]) ? buf[1] : '.',
-             isprint(buf[2]) ? buf[2] : '.',
-             isprint(buf[3]) ? buf[3] : '.',
-             isprint(buf[4]) ? buf[4] : '.',
-             isprint(buf[5]) ? buf[5] : '.',
-             isprint(buf[6]) ? buf[6] : '.',
-             isprint(buf[7]) ? buf[7] : '.');
+             CTYPE(isprint,buf[0]) ? buf[0] : '.',
+             CTYPE(isprint,buf[1]) ? buf[1] : '.',
+             CTYPE(isprint,buf[2]) ? buf[2] : '.',
+             CTYPE(isprint,buf[3]) ? buf[3] : '.',
+             CTYPE(isprint,buf[4]) ? buf[4] : '.',
+             CTYPE(isprint,buf[5]) ? buf[5] : '.',
+             CTYPE(isprint,buf[6]) ? buf[6] : '.',
+             CTYPE(isprint,buf[7]) ? buf[7] : '.');
        TRACE("Magic is 0x%" PRIx64, magic);
        TRACE("Size is %" PRIu64, *size);
 
diff --git a/qemu-common.h b/qemu-common.h
index 23d1444..5d5f46b 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -94,6 +94,16 @@ char *qemu_strdup(const char *str);
 
 void *get_mmap_addr(unsigned long size);
 
+#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.  */
 
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 c31d56a..9ed335e 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -703,7 +703,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 245177a..fd4dcc7 100644
--- a/vl.c
+++ b/vl.c
@@ -4003,7 +4003,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 {
@@ -4418,7 +4418,7 @@ int tap_alloc(char *dev, size_t dev_size)
 
     if( *dev ){
        ptr = dev;
-       while( *ptr && !isdigit((int)*ptr) ) ptr++;
+       while( *ptr && !CTYPE(isdigit,(int)*ptr) ) ptr++;
        ppa = atoi(ptr);
     }
 
-- 
1.4.4.4