diff mbox

[8/9] Add more format string warning flags

Message ID 1333363816-1691-9-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé April 2, 2012, 10:50 a.m. UTC
From: "Daniel P. Berrange" <berrange@redhat.com>

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

Comments

Peter Maydell April 2, 2012, 12:13 p.m. UTC | #1
On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> +#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

Do these pragmas work on all versions of gcc that we support?
Google suggests that the push/pop ones are only gcc 4.6 or better,
for example.

-- PMM
Daniel P. Berrangé April 2, 2012, 12:17 p.m. UTC | #2
On Mon, Apr 02, 2012 at 01:13:56PM +0100, Peter Maydell wrote:
> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> > +#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
> 
> Do these pragmas work on all versions of gcc that we support?
> Google suggests that the push/pop ones are only gcc 4.6 or better,
> for example.

Hmm, the gcc info pages didn't mention any version constraints, but I'll
investigate this


Daniel
Peter Maydell April 2, 2012, 2:04 p.m. UTC | #3
On 2 April 2012 13:17, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Mon, Apr 02, 2012 at 01:13:56PM +0100, Peter Maydell wrote:
>> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
>> > +#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
>>
>> Do these pragmas work on all versions of gcc that we support?
>> Google suggests that the push/pop ones are only gcc 4.6 or better,
>> for example.
>
> Hmm, the gcc info pages didn't mention any version constraints, but I'll
> investigate this

Having thought about it a little more, to be honest I'm not really
convinced that we should have these anyway. Better just to not enable
the format-nonliteral warning. (format-security should catch the cases
we care most about anyway, I think.)

-- PMM
Daniel P. Berrangé April 2, 2012, 2:22 p.m. UTC | #4
On Mon, Apr 02, 2012 at 03:04:54PM +0100, Peter Maydell wrote:
> On 2 April 2012 13:17, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Mon, Apr 02, 2012 at 01:13:56PM +0100, Peter Maydell wrote:
> >> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> >> > +#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
> >>
> >> Do these pragmas work on all versions of gcc that we support?
> >> Google suggests that the push/pop ones are only gcc 4.6 or better,
> >> for example.
> >
> > Hmm, the gcc info pages didn't mention any version constraints, but I'll
> > investigate this
> 
> Having thought about it a little more, to be honest I'm not really
> convinced that we should have these anyway. Better just to not enable
> the format-nonliteral warning. (format-security should catch the cases
> we care most about anyway, I think.)

The -Wformat-security option can only catch problems if the format
string is a literal. eg so it'd miss this:

  void foo(void) {
     int notastring = 1;
     const char *format = "String is %s";

     sprintf(format, notastring);
  }

There are a handful of places in QEMU which do that with non-trivial
format strings & were easy to fix in this patch, which I think is a
worthwhile improvement. The cases in the *-user/strace.c file though
are not practical to fix, without significant re-design of the code
in question.

I'm fine if we just include those easy fixes, while leaving the actual
warning flag disabled for now - someone can revisit later if desired.

Daniel
Peter Maydell April 2, 2012, 2:32 p.m. UTC | #5
On 2 April 2012 15:22, Daniel P. Berrange <berrange@redhat.com> wrote:
> The -Wformat-security option can only catch problems if the format
> string is a literal. eg so it'd miss this:
>
>  void foo(void) {
>     int notastring = 1;
>     const char *format = "String is %s";
>
>     sprintf(format, notastring);
>  }
>
> There are a handful of places in QEMU which do that with non-trivial
> format strings & were easy to fix in this patch, which I think is a
> worthwhile improvement. The cases in the *-user/strace.c file though
> are not practical to fix, without significant re-design of the code
> in question.

To be honest I couldn't tell from your patch whether you'd actually
fixed any bugs or if you were just moving things around to turn non
literals into literals.

(Some of the cleanup looks like a good idea anyway, eg the vnc bits.)

-- PMM
Daniel P. Berrangé April 2, 2012, 2:34 p.m. UTC | #6
On Mon, Apr 02, 2012 at 03:32:51PM +0100, Peter Maydell wrote:
> On 2 April 2012 15:22, Daniel P. Berrange <berrange@redhat.com> wrote:
> > The -Wformat-security option can only catch problems if the format
> > string is a literal. eg so it'd miss this:
> >
> >  void foo(void) {
> >     int notastring = 1;
> >     const char *format = "String is %s";
> >
> >     sprintf(format, notastring);
> >  }
> >
> > There are a handful of places in QEMU which do that with non-trivial
> > format strings & were easy to fix in this patch, which I think is a
> > worthwhile improvement. The cases in the *-user/strace.c file though
> > are not practical to fix, without significant re-design of the code
> > in question.
> 
> To be honest I couldn't tell from your patch whether you'd actually
> fixed any bugs or if you were just moving things around to turn non
> literals into literals.

There were no actual bugs fixed - it was just the change you describe
from non-literal to literal - to protect against future possible bugs.

> (Some of the cleanup looks like a good idea anyway, eg the vnc bits.)

Yep, I don't know why I didn't write that VNC code this way in the
first place now :-)

Daniel
diff mbox

Patch

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));