Libguestfs - Feb 2017 - [PATCH v2 1/2] GCC 7: Add __attribute__((noreturn)) to some usage functions which call exit.

v1 -> v2:

 - Use intprops macro suggested by danpb.

Rich.

This happens with GCC 7.0.1.  The errors were all of the form:

qemu-speed-test.c: In function 'main':
qemu-speed-test.c:153:7: error: this statement may fall through
[-Werror=implicit-fallthrough=]
       usage (EXIT_SUCCESS);
       ^~~~~~~~~~~~~~~~~~~~
qemu-speed-test.c:155:5: note: here
     default:
     ^~~~~~~
---
 builder/index-validate.c                | 2 +-
 utils/boot-analysis/boot-analysis.c     | 2 +-
 utils/boot-benchmark/boot-benchmark.c   | 2 +-
 utils/qemu-boot/qemu-boot.c             | 2 +-
 utils/qemu-speed-test/qemu-speed-test.c | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builder/index-validate.c b/builder/index-validate.c
index d3912f3..33086b2 100644
--- a/builder/index-validate.c
+++ b/builder/index-validate.c
@@ -38,7 +38,7 @@
 
 extern int do_parse (struct parse_context *context, FILE *in);
 
-static void
+static void __attribute__((noreturn))
 usage (int exit_status)
 {
   printf ("%s index\n", getprogname ());
diff --git a/utils/boot-analysis/boot-analysis.c
b/utils/boot-analysis/boot-analysis.c
index 04b3bdd..1bec9a5 100644
--- a/utils/boot-analysis/boot-analysis.c
+++ b/utils/boot-analysis/boot-analysis.c
@@ -88,7 +88,7 @@ static void print_longest_to_shortest (void);
 static void free_pass_data (void);
 static void free_final_timeline (void);
 
-static void
+static void __attribute__((noreturn))
 usage (int exitcode)
 {
   guestfs_h *g;
diff --git a/utils/boot-benchmark/boot-benchmark.c
b/utils/boot-benchmark/boot-benchmark.c
index 73da722..4af3943 100644
--- a/utils/boot-benchmark/boot-benchmark.c
+++ b/utils/boot-benchmark/boot-benchmark.c
@@ -51,7 +51,7 @@ static void run_test (void);
 static guestfs_h *create_handle (void);
 static void add_drive (guestfs_h *g);
 
-static void
+static void __attribute__((noreturn))
 usage (int exitcode)
 {
   guestfs_h *g;
diff --git a/utils/qemu-boot/qemu-boot.c b/utils/qemu-boot/qemu-boot.c
index 1551a1a..6881835 100644
--- a/utils/qemu-boot/qemu-boot.c
+++ b/utils/qemu-boot/qemu-boot.c
@@ -74,7 +74,7 @@ static void run_test (size_t P);
 static void *start_thread (void *thread_data_vp);
 static void message_callback (guestfs_h *g, void *opaque, uint64_t event, int
event_handle, int flags, const char *buf, size_t buf_len, const uint64_t *array,
size_t array_len);
 
-static void
+static void __attribute__((noreturn))
 usage (int exitcode)
 {
   fprintf (stderr,
diff --git a/utils/qemu-speed-test/qemu-speed-test.c
b/utils/qemu-speed-test/qemu-speed-test.c
index 54875fa..5aa663c 100644
--- a/utils/qemu-speed-test/qemu-speed-test.c
+++ b/utils/qemu-speed-test/qemu-speed-test.c
@@ -66,7 +66,7 @@ reset_default_tests (int *flag)
   }
 }
 
-static void
+static void __attribute__((noreturn))
 usage (int exitcode)
 {
   fprintf (stderr,
-- 
2.10.2

GCC 7.0.1 can determine if there is likely to be sufficient space in
the output buffer when using sprintf/snprintf, based on the format
string.

The errors were all either of this form:

bindtests.c:717:29: error: '%zu' directive output may be truncated
writing between 1 and 19 bytes into a region of size 16
[-Werror=format-truncation=]
     snprintf (strs[i], 16, "%zu", i);
                             ^~~
bindtests.c:717:28: note: directive argument in the range [0,
2305843009213693951]
     snprintf (strs[i], 16, "%zu", i);
                             ^~~~~

or this form:

sync.c: In function 'fsync_devices':
sync.c:108:50: error: '%s' directive output may be truncated writing up
to 255 bytes into a region of size 251 [-Werror=format-truncation=]
       snprintf (dev_path, sizeof dev_path, "/dev/%s", d->d_name);
                                                  ^~

Fixed by converting these into dynamic allocation, or making the
output buffer larger, whichever was easier.

There is a gnulib macro we can use to make this simpler for integers.
It requires a new gnulib module (intprops), but it turns out that we
were already pulling that in through dependencies, so the change to
bootstrap is a no-op.  (thanks: Dan Berrange)
---
 bootstrap              |  1 +
 cat/filesystems.c      |  3 ++-
 daemon/9p.c            | 10 +++++++---
 daemon/debug.c         | 12 ++++++++++--
 daemon/devsparts.c     | 16 ++++++++++++----
 daemon/sync.c          |  7 +++++--
 generator/bindtests.ml | 14 ++++++++------
 7 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/bootstrap b/bootstrap
index 071682c..037d07e 100755
--- a/bootstrap
+++ b/bootstrap
@@ -66,6 +66,7 @@ hash-pjw
 human
 iconv
 ignore-value
+intprops
 lock
 maintainer-makefile
 manywarnings
diff --git a/cat/filesystems.c b/cat/filesystems.c
index 1036c6f..0c7748d 100644
--- a/cat/filesystems.c
+++ b/cat/filesystems.c
@@ -33,6 +33,7 @@
 
 #include "c-ctype.h"
 #include "human.h"
+#include "intprops.h"
 #include "getprogname.h"
 
 #include "guestfs.h"
@@ -866,7 +867,7 @@ write_row (const char *name, const char *type,
   size_t len = 0;
   char hum[LONGEST_HUMAN_READABLE];
   char num[256];
-  char mbr_id_str[3];
+  char mbr_id_str[INT_BUFSIZE_BOUND (mbr_id)];
 
   if ((columns & COLUMN_NAME))
     strings[len++] = name;
diff --git a/daemon/9p.c b/daemon/9p.c
index a9e36d1..f72c8dd 100644
--- a/daemon/9p.c
+++ b/daemon/9p.c
@@ -71,9 +71,13 @@ do_list_9p (void)
     if (d == NULL) break;
 
     if (STRPREFIX (d->d_name, "virtio")) {
-      char mount_tag_path[256];
-      snprintf (mount_tag_path, sizeof mount_tag_path,
-                BUS_PATH "/%s/mount_tag", d->d_name);
+      CLEANUP_FREE char *mount_tag_path;
+      if (asprintf (&mount_tag_path, BUS_PATH "/%s/mount_tag",
+                    d->d_name) == -1) {
+        reply_with_perror ("asprintf");
+        closedir (dir);
+        return NULL;
+      }
 
       /* A bit unclear, but it looks like the virtio transport allows
        * the mount tag length to be unlimited (or up to 65536 bytes).
diff --git a/daemon/debug.c b/daemon/debug.c
index 06f0729..b18d87c 100644
--- a/daemon/debug.c
+++ b/daemon/debug.c
@@ -161,7 +161,7 @@ debug_fds (const char *subcmd, size_t argc, char *const
*const argv)
   FILE *fp;
   DIR *dir;
   struct dirent *d;
-  char fname[256], link[256];
+  char link[256];
   struct stat statbuf;
 
   fp = open_memstream (&out, &size);
@@ -178,10 +178,18 @@ debug_fds (const char *subcmd, size_t argc, char *const
*const argv)
   }
 
   while ((d = readdir (dir)) != NULL) {
+    CLEANUP_FREE char *fname = NULL;
+
     if (STREQ (d->d_name, ".") || STREQ (d->d_name,
".."))
       continue;
 
-    snprintf (fname, sizeof fname, "/proc/self/fd/%s", d->d_name);
+    if (asprintf (&fname, "/proc/self/fd/%s", d->d_name) ==
-1) {
+      reply_with_perror ("asprintf");
+      fclose (fp);
+      free (out);
+      closedir (dir);
+      return NULL;
+    }
 
     r = lstat (fname, &statbuf);
     if (r == -1) {
diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index f15d2c3..96e867f 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -43,7 +43,6 @@ foreach_block_device (block_dev_func_t func, bool return_md)
   DIR *dir;
   int err = 0;
   struct dirent *d;
-  char dev_path[256];
   int fd;
 
   dir = opendir ("/sys/block");
@@ -64,7 +63,12 @@ foreach_block_device (block_dev_func_t func, bool return_md)
         STREQLEN (d->d_name, "sr", 2) ||
         (return_md &&
          STREQLEN (d->d_name, "md", 2) && c_isdigit
(d->d_name[2]))) {
-      snprintf (dev_path, sizeof dev_path, "/dev/%s", d->d_name);
+      CLEANUP_FREE char *dev_path;
+      if (asprintf (&dev_path, "/dev/%s", d->d_name) == -1) {
+        reply_with_perror ("asprintf");
+        closedir (dir);
+        return NULL;
+      }
 
       /* Ignore the root device. */
       if (is_root_device (dev_path))
@@ -167,8 +171,12 @@ add_partitions (const char *device, struct stringsbuf *r)
   struct dirent *d;
   while ((d = readdir (dir)) != NULL) {
     if (STREQLEN (d->d_name, device, strlen (device))) {
-      char part[256];
-      snprintf (part, sizeof part, "/dev/%s", d->d_name);
+      CLEANUP_FREE char *part;
+      if (asprintf (&part, "/dev/%s", d->d_name) == -1) {
+        perror ("asprintf");
+        closedir (dir);
+        return -1;
+      }
 
       if (add_string (r, part) == -1) {
         closedir (dir);
diff --git a/daemon/sync.c b/daemon/sync.c
index 581fa0f..b64f6e3 100644
--- a/daemon/sync.c
+++ b/daemon/sync.c
@@ -86,7 +86,6 @@ fsync_devices (void)
 {
   DIR *dir;
   struct dirent *d;
-  char dev_path[256];
   int fd;
 
   dir = opendir ("/sys/block");
@@ -105,7 +104,11 @@ fsync_devices (void)
         STREQLEN (d->d_name, "ubd", 3) ||
         STREQLEN (d->d_name, "vd", 2) ||
         STREQLEN (d->d_name, "sr", 2)) {
-      snprintf (dev_path, sizeof dev_path, "/dev/%s", d->d_name);
+      CLEANUP_FREE char *dev_path;
+      if (asprintf (&dev_path, "/dev/%s", d->d_name) == -1) {
+        perror ("asprintf");
+        continue;
+      }
 
       /* Ignore the root device. */
       if (is_root_device (dev_path))
diff --git a/generator/bindtests.ml b/generator/bindtests.ml
index 22d0c37..8ac5031 100644
--- a/generator/bindtests.ml
+++ b/generator/bindtests.ml
@@ -48,6 +48,8 @@ let rec generate_bindtests ()  #include
\"guestfs-internal-actions.h\"
 #include \"guestfs_protocol.h\"
 
+#include \"intprops.h\"
+
 int
 guestfs_impl_internal_test_set_output (guestfs_h *g, const char *filename)
 {
@@ -258,8 +260,8 @@ fill_lvm_pv (guestfs_h *g, struct guestfs_lvm_pv *pv, size_t
i)
              pr "  }\n";
              pr "  strs = safe_malloc (g, (n+1) * sizeof (char
*));\n";
              pr "  for (i = 0; i < n; ++i) {\n";
-             pr "    strs[i] = safe_malloc (g, 16);\n";
-             pr "    snprintf (strs[i], 16, \"%%zu\",
i);\n";
+             pr "    strs[i] = safe_malloc (g, INT_BUFSIZE_BOUND
(i));\n";
+             pr "    snprintf (strs[i], INT_BUFSIZE_BOUND (i),
\"%%zu\", i);\n";
              pr "  }\n";
              pr "  strs[n] = NULL;\n";
              pr "  return strs;\n"
@@ -290,10 +292,10 @@ fill_lvm_pv (guestfs_h *g, struct guestfs_lvm_pv *pv,
size_t i)
              pr "  }\n";
              pr "  strs = safe_malloc (g, (n*2+1) * sizeof
(*strs));\n";
              pr "  for (i = 0; i < n; ++i) {\n";
-             pr "    strs[i*2] = safe_malloc (g, 16);\n";
-             pr "    strs[i*2+1] = safe_malloc (g, 16);\n";
-             pr "    snprintf (strs[i*2], 16, \"%%zu\",
i);\n";
-             pr "    snprintf (strs[i*2+1], 16, \"%%zu\",
i);\n";
+             pr "    strs[i*2] = safe_malloc (g, INT_BUFSIZE_BOUND
(i));\n";
+             pr "    strs[i*2+1] = safe_malloc (g, INT_BUFSIZE_BOUND
(i));\n";
+             pr "    snprintf (strs[i*2], INT_BUFSIZE_BOUND (i),
\"%%zu\", i);\n";
+             pr "    snprintf (strs[i*2+1], INT_BUFSIZE_BOUND (i),
\"%%zu\", i);\n";
              pr "  }\n";
              pr "  strs[n*2] = NULL;\n";
              pr "  return strs;\n"
-- 
2.10.2

On Tuesday, 14 February 2017 16:51:23 CET Richard W.M. Jones
wrote:
> diff --git a/daemon/9p.c b/daemon/9p.c
> index a9e36d1..f72c8dd 100644
> --- a/daemon/9p.c
> +++ b/daemon/9p.c
> @@ -71,9 +71,13 @@ do_list_9p (void)
>      if (d == NULL) break;
>  
>      if (STRPREFIX (d->d_name, "virtio")) {
> -      char mount_tag_path[256];
> -      snprintf (mount_tag_path, sizeof mount_tag_path,
> -                BUS_PATH "/%s/mount_tag", d->d_name);
> +      CLEANUP_FREE char *mount_tag_path;
I'd initialize this (and all the other occurrences in this patch) to
NULL anyway, so if in the future more code is added between the
declaration and the initialization then there's no risk.

LGTM with the above changes.

-- 
Pino Toscano

On Tuesday, 14 February 2017 16:51:22 CET Richard W.M. Jones
wrote:
> This happens with GCC 7.0.1.  The errors were all of the form:
> 
> qemu-speed-test.c: In function 'main':
> qemu-speed-test.c:153:7: error: this statement may fall through
[-Werror=implicit-fallthrough=]
>        usage (EXIT_SUCCESS);
>        ^~~~~~~~~~~~~~~~~~~~
> qemu-speed-test.c:155:5: note: here
>      default:
>      ^~~~~~~
> ---
LGTM.

Thanks,
-- 
Pino Toscano