[PATCH 1/2] GCC 7: Add __attribute__((noreturn)) to some usage functions which call exit.
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. --- cat/filesystems.c | 2 +- daemon/9p.c | 10 +++++++--- daemon/debug.c | 12 ++++++++++-- daemon/devsparts.c | 16 ++++++++++++---- daemon/sync.c | 7 +++++-- generator/bindtests.ml | 12 ++++++------ 6 files changed, 41 insertions(+), 18 deletions(-) diff --git a/cat/filesystems.c b/cat/filesystems.c index 1036c6f..4264b0f 100644 --- a/cat/filesystems.c +++ b/cat/filesystems.c @@ -866,7 +866,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[9]; 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..2fde425 100644 --- a/generator/bindtests.ml +++ b/generator/bindtests.ml @@ -258,8 +258,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, 20);\n"; + pr " snprintf (strs[i], 20, \"%%zu\", i);\n"; pr " }\n"; pr " strs[n] = NULL;\n"; pr " return strs;\n" @@ -290,10 +290,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, 20);\n"; + pr " strs[i*2+1] = safe_malloc (g, 20);\n"; + pr " snprintf (strs[i*2], 20, \"%%zu\", i);\n"; + pr " snprintf (strs[i*2+1], 20, \"%%zu\", i);\n"; pr " }\n"; pr " strs[n*2] = NULL;\n"; pr " return strs;\n" -- 2.10.2
On Tue, Feb 14, 2017 at 03:02:14PM +0000, Richard W.M. Jones wrote:
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. --- cat/filesystems.c | 2 +- daemon/9p.c | 10 +++++++--- daemon/debug.c | 12 ++++++++++-- daemon/devsparts.c | 16 ++++++++++++---- daemon/sync.c | 7 +++++-- generator/bindtests.ml | 12 ++++++------ 6 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/cat/filesystems.c b/cat/filesystems.c index 1036c6f..4264b0f 100644 --- a/cat/filesystems.c +++ b/cat/filesystems.c @@ -866,7 +866,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[9];
[snip]
@@ -258,8 +258,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, 20);\n"; + pr " snprintf (strs[i], 20, \"%%zu\", i);\n"; pr " }\n"; pr " strs[n] = NULL;\n"; pr " return strs;\n" @@ -290,10 +290,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, 20);\n"; + pr " strs[i*2+1] = safe_malloc (g, 20);\n"; + pr " snprintf (strs[i*2], 20, \"%%zu\", i);\n"; + pr " snprintf (strs[i*2+1], 20, \"%%zu\", i);\n"; pr " }\n"; pr " strs[n*2] = NULL;\n"; pr " return strs;\n"
FWIW, for cases where you really do want to print an integer into a fixed length buffer, then gnulib has a useful macro to let you automatically determine the correct buffer size at compile time. size_t someval; char buf[INT_BUFSIZE_BOUND(someval)] snprintf(buf, ARRAY_CARDINALITY(buf), "%zu", someval) Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
participants (2)
-
Daniel P. Berrange -
Richard W.M. Jones