[Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Daniel P. Berrange]

From: "Daniel P. Berrange" <address@hidden>

Add -Wformat-contains-nul, -Wformat-extra-args,
-Wformat-zero-length and -Wformat-nonliteral to the compiler
flags & fix the issues they identify

It is desirable to have these warnings enabled, even though
it is not practical to fix all violations. Introduce some
macros GCC_WARNINGS_SAVE, GCC_WARNINGS_RESTORE &
GCC_WARNINGS_IGNORE, which allow specific warnings to be
ignored for small blocks of code

* cmd.c, block/vmdk.c: Pass format strings directly to
  snprintf instead of storing then in intermediate
  'const char*' variables, so that GCC can validate
  args fully
* configure: Add -Wformat-contains-nul, -Wformat-extra-args,
  -Wformat-zero-length, -Wformat-nonliteral
* compiler.h: Add macros for turning off warnings selectively
* bsd-user/strace.c, linux-user/strace.c: Disable warnings
  about non-literal format args
* ui/vnc.c, ui/vnc.h, ui/vnc-auth-sasl.c: Refactor
  vnc_socket_local_addr & vnc_socket_remote_addr to
  just take a separator instead of format string

Signed-off-by: Daniel P. Berrange <address@hidden>
---
 block/vmdk.c        |   64 ++++++++++++++++++++++++---------------------------
 bsd-user/strace.c   |    4 +++
 cmd.c               |   63 +++++++++++++++++++++++++++-----------------------
 compiler.h          |   11 ++++++++
 configure           |    4 +++
 linux-user/strace.c |    6 ++++
 ui/vnc-auth-sasl.c  |    7 ++++-
 ui/vnc.c            |   20 ++++++++-------
 ui/vnc.h            |    4 +-
 9 files changed, 107 insertions(+), 76 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 45c003a..12cad53 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1356,28 +1356,8 @@ static int vmdk_create(const char *filename, 
QEMUOptionParameter *options)
     char ext_desc_lines[BUF_SIZE] = "";
     char path[PATH_MAX], prefix[PATH_MAX], postfix[PATH_MAX];
     const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
-    const char *desc_extent_line;
     char parent_desc_line[BUF_SIZE] = "";
     uint32_t parent_cid = 0xffffffff;
-    const char desc_template[] =
-        "# Disk DescriptorFile\n"
-        "version=1\n"
-        "CID=%x\n"
-        "parentCID=%x\n"
-        "createType=\"%s\"\n"
-        "%s"
-        "\n"
-        "# Extent description\n"
-        "%s"
-        "\n"
-        "# The Disk Data Base\n"
-        "#DDB\n"
-        "\n"
-        "ddb.virtualHWVersion = \"%d\"\n"
-        "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
-        "ddb.geometry.heads = \"16\"\n"
-        "ddb.geometry.sectors = \"63\"\n"
-        "ddb.adapterType = \"ide\"\n";
 
     if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
         return -EINVAL;
@@ -1411,11 +1391,6 @@ static int vmdk_create(const char *filename, 
QEMUOptionParameter *options)
     flat = !(strcmp(fmt, "monolithicFlat") &&
              strcmp(fmt, "twoGbMaxExtentFlat"));
     compress = !strcmp(fmt, "streamOptimized");
-    if (flat) {
-        desc_extent_line = "RW %lld FLAT \"%s\" 0\n";
-    } else {
-        desc_extent_line = "RW %lld SPARSE \"%s\"\n";
-    }
     if (flat && backing_file) {
         /* not supporting backing file for flat image */
         return -ENOTSUP;
@@ -1471,18 +1446,39 @@ static int vmdk_create(const char *filename, 
QEMUOptionParameter *options)
 
         /* Format description line */
         snprintf(desc_line, sizeof(desc_line),
-                    desc_extent_line, size / 512, desc_filename);
+                 (flat
+                  ? "RW %" PRId64 " FLAT \"%s\" 0\n"
+                  : "RW %" PRId64 " SPARSE \"%s\"\n"),
+                 size / 512, desc_filename);
         pstrcat(ext_desc_lines, sizeof(ext_desc_lines), desc_line);
     }
     /* generate descriptor file */
-    snprintf(desc, sizeof(desc), desc_template,
-            (unsigned int)time(NULL),
-            parent_cid,
-            fmt,
-            parent_desc_line,
-            ext_desc_lines,
-            (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
-            total_size / (int64_t)(63 * 16 * 512));
+    snprintf(desc, sizeof(desc),
+             "# Disk DescriptorFile\n"
+             "version=1\n"
+             "CID=%x\n"
+             "parentCID=%x\n"
+             "createType=\"%s\"\n"
+             "%s"
+             "\n"
+             "# Extent description\n"
+             "%s"
+             "\n"
+             "# The Disk Data Base\n"
+             "#DDB\n"
+             "\n"
+             "ddb.virtualHWVersion = \"%d\"\n"
+             "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
+             "ddb.geometry.heads = \"16\"\n"
+             "ddb.geometry.sectors = \"63\"\n"
+             "ddb.adapterType = \"ide\"\n",
+             (unsigned int)time(NULL),
+             parent_cid,
+             fmt,
+             parent_desc_line,
+             ext_desc_lines,
+             (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
+             total_size / (int64_t)(63 * 16 * 512));
     if (split || flat) {
         fd = open(
                 filename,
diff --git a/bsd-user/strace.c b/bsd-user/strace.c
index d73bbca..73ae567 100644
--- a/bsd-user/strace.c
+++ b/bsd-user/strace.c
@@ -90,6 +90,9 @@ static const struct syscallname openbsd_scnames[] = {
 #include "openbsd/strace.list"
 };
 
+
+GCC_WARNINGS_SAVE
+GCC_WARNINGS_IGNORE("-Wformat-nonliteral")
 static void
 print_syscall(int num, const struct syscallname *scnames, unsigned int 
nscnames,
               abi_long arg1, abi_long arg2, abi_long arg3,
@@ -119,6 +122,7 @@ print_syscall(int num, const struct syscallname *scnames, 
unsigned int nscnames,
         }
     gemu_log("Unknown syscall %d\n", num);
 }
+GCC_WARNINGS_RESTORE
 
 static void
 print_syscall_ret(int num, abi_long ret, const struct syscallname *scnames,
diff --git a/cmd.c b/cmd.c
index 0806e18..0490d23 100644
--- a/cmd.c
+++ b/cmd.c
@@ -414,36 +414,41 @@ cvtnum(
 
 void
 cvtstr(
-       double          value,
-       char            *str,
-       size_t          size)
+        double                value,
+        char                *str,
+        size_t                size)
 {
-       const char      *fmt;
-       int             precise;
-
-       precise = ((double)value * 1000 == (double)(int)value * 1000);
-
-       if (value >= EXABYTES(1)) {
-               fmt = precise ? "%.f EiB" : "%.3f EiB";
-               snprintf(str, size, fmt, TO_EXABYTES(value));
-       } else if (value >= PETABYTES(1)) {
-               fmt = precise ? "%.f PiB" : "%.3f PiB";
-               snprintf(str, size, fmt, TO_PETABYTES(value));
-       } else if (value >= TERABYTES(1)) {
-               fmt = precise ? "%.f TiB" : "%.3f TiB";
-               snprintf(str, size, fmt, TO_TERABYTES(value));
-       } else if (value >= GIGABYTES(1)) {
-               fmt = precise ? "%.f GiB" : "%.3f GiB";
-               snprintf(str, size, fmt, TO_GIGABYTES(value));
-       } else if (value >= MEGABYTES(1)) {
-               fmt = precise ? "%.f MiB" : "%.3f MiB";
-               snprintf(str, size, fmt, TO_MEGABYTES(value));
-       } else if (value >= KILOBYTES(1)) {
-               fmt = precise ? "%.f KiB" : "%.3f KiB";
-               snprintf(str, size, fmt, TO_KILOBYTES(value));
-       } else {
-               snprintf(str, size, "%f bytes", value);
-       }
+        int                precise;
+
+        precise = ((double)value * 1000 == (double)(int)value * 1000);
+
+        if (value >= EXABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f EiB" : "%.3f EiB",
+                         TO_EXABYTES(value));
+        } else if (value >= PETABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f PiB" : "%.3f PiB",
+                         TO_PETABYTES(value));
+        } else if (value >= TERABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f TiB" : "%.3f TiB",
+                         TO_TERABYTES(value));
+        } else if (value >= GIGABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f GiB" : "%.3f GiB",
+                         TO_GIGABYTES(value));
+        } else if (value >= MEGABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f MiB" : "%.3f MiB",
+                         TO_MEGABYTES(value));
+        } else if (value >= KILOBYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f KiB" : "%.3f KiB",
+                         TO_KILOBYTES(value));
+        } else {
+                snprintf(str, size, "%f bytes", value);
+        }
 }
 
 struct timeval
diff --git a/compiler.h b/compiler.h
index 736e770..145d848 100644
--- a/compiler.h
+++ b/compiler.h
@@ -50,4 +50,15 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+#if defined __GNUC__
+# define GCC_WARNINGS_SAVE      _Pragma("GCC diagnostic push")
+# define GCC_WARNINGS_RESTORE   _Pragma("GCC diagnostic pop")
+# define DO_PRAGMA(x)           _Pragma(#x)
+# define GCC_WARNINGS_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored x)
+#else
+# define GCC_WARNINGS_SAVE
+# define GCC_WARNINGS_RESTORE
+# define GCC_WARNINGS_IGNORE(x)
+#endif
+
 #endif /* COMPILER_H */
diff --git a/configure b/configure
index 3d47440..175901f 100755
--- a/configure
+++ b/configure
@@ -1194,6 +1194,10 @@ gcc_flags="$gcc_flags -Wmissing-parameter-type"
 gcc_flags="$gcc_flags -Wuninitialized"
 gcc_flags="$gcc_flags -Wlogical-op"
 gcc_flags="$gcc_flags -Wmissing-format-attribute"
+gcc_flags="$gcc_flags -Wformat-contains-nul"
+gcc_flags="$gcc_flags -Wformat-extra-args"
+gcc_flags="$gcc_flags -Wformat-zero-length"
+gcc_flags="$gcc_flags -Wformat-nonliteral"
 
 cat > $TMPC << EOF
 int main(void) { return 0; }
diff --git a/linux-user/strace.c b/linux-user/strace.c
index 05a0d3e..75fd70b 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -627,6 +627,8 @@ print_string(abi_long addr, int last)
  * Prints out raw parameter using given format.  Caller needs
  * to do byte swapping if needed.
  */
+GCC_WARNINGS_SAVE
+GCC_WARNINGS_IGNORE("-Wformat-nonliteral")
 static void
 print_raw_param(const char *fmt, abi_long param, int last)
 {
@@ -635,6 +637,7 @@ print_raw_param(const char *fmt, abi_long param, int last)
     (void) snprintf(format, sizeof (format), "%s%s", fmt, get_comma(last));
     gemu_log(format, param);
 }
+GCC_WARNINGS_RESTORE
 
 static void
 print_pointer(abi_long p, int last)
@@ -1488,6 +1491,8 @@ static int nsyscalls = ARRAY_SIZE(scnames);
 /*
  * The public interface to this module.
  */
+GCC_WARNINGS_SAVE
+GCC_WARNINGS_IGNORE("-Wformat-nonliteral")
 void
 print_syscall(int num,
               abi_long arg1, abi_long arg2, abi_long arg3,
@@ -1513,6 +1518,7 @@ print_syscall(int num,
         }
     gemu_log("Unknown syscall %d\n", num);
 }
+GCC_WARNINGS_RESTORE
 
 
 void
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 8fba770..07c4f5a 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -500,10 +500,13 @@ void start_auth_sasl(VncState *vs)
     VNC_DEBUG("Initialize SASL auth %d\n", vs->csock);
 
     /* Get local & remote client addresses in form  IPADDR;PORT */
-    if (!(localAddr = vnc_socket_local_addr("%s;%s", vs->csock)))
+    localAddr = vnc_socket_local_addr(';', vs->csock);
+    if (!localAddr) {
         goto authabort;
+    }
 
-    if (!(remoteAddr = vnc_socket_remote_addr("%s;%s", vs->csock))) {
+    remoteAddr = vnc_socket_remote_addr(';', vs->csock);
+    if (!remoteAddr) {
         g_free(localAddr);
         goto authabort;
     }
diff --git a/ui/vnc.c b/ui/vnc.c
index deb9ecd..86c5c3b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -71,7 +71,7 @@ static void vnc_set_share_mode(VncState *vs, VncShareMode 
mode)
     }
 }
 
-static char *addr_to_string(const char *format,
+static char *addr_to_string(char separator,
                             struct sockaddr_storage *sa,
                             socklen_t salen) {
     char *addr;
@@ -89,18 +89,19 @@ static char *addr_to_string(const char *format,
         return NULL;
     }
 
-    /* Enough for the existing format + the 2 vars we're
+    /* Enough for the separator + the 2 vars we're
      * substituting in. */
-    addrlen = strlen(format) + strlen(host) + strlen(serv);
+    addrlen = strlen(host) + 1 + strlen(serv);
     addr = g_malloc(addrlen + 1);
-    snprintf(addr, addrlen, format, host, serv);
+    snprintf(addr, addrlen, "%s%c%s", host, separator, serv);
     addr[addrlen] = '\0';
 
     return addr;
 }
 
 
-char *vnc_socket_local_addr(const char *format, int fd) {
+char *vnc_socket_local_addr(char separator, int fd)
+{
     struct sockaddr_storage sa;
     socklen_t salen;
 
@@ -108,10 +109,11 @@ char *vnc_socket_local_addr(const char *format, int fd) {
     if (getsockname(fd, (struct sockaddr*)&sa, &salen) < 0)
         return NULL;
 
-    return addr_to_string(format, &sa, salen);
+    return addr_to_string(separator, &sa, salen);
 }
 
-char *vnc_socket_remote_addr(const char *format, int fd) {
+char *vnc_socket_remote_addr(char separator, int fd)
+{
     struct sockaddr_storage sa;
     socklen_t salen;
 
@@ -119,7 +121,7 @@ char *vnc_socket_remote_addr(const char *format, int fd) {
     if (getpeername(fd, (struct sockaddr*)&sa, &salen) < 0)
         return NULL;
 
-    return addr_to_string(format, &sa, salen);
+    return addr_to_string(separator, &sa, salen);
 }
 
 static int put_addr_qdict(QDict *qdict, struct sockaddr_storage *sa,
@@ -2857,7 +2859,7 @@ char *vnc_display_local_addr(DisplayState *ds)
 {
     VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
     
-    return vnc_socket_local_addr("%s:%s", vs->lsock);
+    return vnc_socket_local_addr(':', vs->lsock);
 }
 
 int vnc_display_open(DisplayState *ds, const char *display)
diff --git a/ui/vnc.h b/ui/vnc.h
index a851ebd..fc5be66 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -533,8 +533,8 @@ void buffer_append(Buffer *buffer, const void *data, size_t 
len);
 
 /* Misc helpers */
 
-char *vnc_socket_local_addr(const char *format, int fd);
-char *vnc_socket_remote_addr(const char *format, int fd);
+char *vnc_socket_local_addr(char separator, int fd);
+char *vnc_socket_remote_addr(char separator, int fd);
 
 static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
     return (vs->features & (1 << feature));
-- 
1.7.7.6