From 9a8e46790fe1c2460244e3c6d25152218720d7b8 Mon Sep 17 00:00:00 2001 From: Tony Hutter Date: Tue, 20 Jun 2017 11:47:31 -0700 Subject: [PATCH] GCC 7.1 fixes GCC 7.1 with will warn when we're not checking the snprintf() return code in cases where the buffer could be truncated. This patch either checks the snprintf return code (where applicable), or converts the snprintfs to a new SNPRINTF_TRUNC() macro, which silently truncates without the compiler error. Signed-off-by: Tony Hutter --- cmd/ztest/ztest.c | 26 ++++++++++++++------------ include/zfs_comutil.h | 21 +++++++++++++++++++++ lib/libspl/include/assert.h | 28 +++++++++++++++++++++------- lib/libzfs/libzfs_dataset.c | 22 ++++++++++++++-------- lib/libzfs/libzfs_iter.c | 7 +++++-- lib/libzfs/libzfs_sendrecv.c | 8 ++++++-- module/zfs/zfs_ioctl.c | 6 ++++-- module/zfs/zvol.c | 3 ++- 8 files changed, 87 insertions(+), 34 deletions(-) diff --git a/cmd/ztest/ztest.c b/cmd/ztest/ztest.c index 39fbfd993adf..3a72901114da 100644 --- a/cmd/ztest/ztest.c +++ b/cmd/ztest/ztest.c @@ -126,6 +126,7 @@ #include #include #include +#include #ifdef __GLIBC__ #include /* for backtrace() */ #endif @@ -3540,8 +3541,9 @@ ztest_snapshot_destroy(char *osname, uint64_t id) char snapname[ZFS_MAX_DATASET_NAME_LEN]; int error; - (void) snprintf(snapname, sizeof (snapname), "%s@%llu", osname, - (u_longlong_t)id); + if (snprintf(snapname, sizeof (snapname), "%s@%llu", osname, + (u_longlong_t)id) >= sizeof (snapname)) + return (B_FALSE); error = dsl_destroy_snapshot(snapname, B_FALSE); if (error != 0 && error != ENOENT) @@ -3565,8 +3567,8 @@ ztest_dmu_objset_create_destroy(ztest_ds_t *zd, uint64_t id) (void) rw_rdlock(&ztest_name_lock); - (void) snprintf(name, sizeof (name), "%s/temp_%llu", - ztest_opts.zo_pool, (u_longlong_t)id); + SNPRINTF_TRUNC(name, sizeof (name), "%s/temp_%llu", ztest_opts.zo_pool, + (u_longlong_t)id); /* * If this dataset exists from a previous run, process its replay log @@ -3744,15 +3746,15 @@ ztest_dsl_dataset_promote_busy(ztest_ds_t *zd, uint64_t id) ztest_dsl_dataset_cleanup(osname, id); - (void) snprintf(snap1name, ZFS_MAX_DATASET_NAME_LEN, + SNPRINTF_TRUNC(snap1name, ZFS_MAX_DATASET_NAME_LEN, "%s@s1_%llu", osname, (u_longlong_t)id); - (void) snprintf(clone1name, ZFS_MAX_DATASET_NAME_LEN, + SNPRINTF_TRUNC(clone1name, ZFS_MAX_DATASET_NAME_LEN, "%s/c1_%llu", osname, (u_longlong_t)id); - (void) snprintf(snap2name, ZFS_MAX_DATASET_NAME_LEN, + SNPRINTF_TRUNC(snap2name, ZFS_MAX_DATASET_NAME_LEN, "%s@s2_%llu", clone1name, (u_longlong_t)id); - (void) snprintf(clone2name, ZFS_MAX_DATASET_NAME_LEN, + SNPRINTF_TRUNC(clone2name, ZFS_MAX_DATASET_NAME_LEN, "%s/c2_%llu", osname, (u_longlong_t)id); - (void) snprintf(snap3name, ZFS_MAX_DATASET_NAME_LEN, + SNPRINTF_TRUNC(snap3name, ZFS_MAX_DATASET_NAME_LEN, "%s@s3_%llu", clone1name, (u_longlong_t)id); error = dmu_objset_snapshot_one(osname, strchr(snap1name, '@') + 1); @@ -6120,7 +6122,7 @@ ztest_thread(void *arg) static void ztest_dataset_name(char *dsname, char *pool, int d) { - (void) snprintf(dsname, ZFS_MAX_DATASET_NAME_LEN, "%s/ds_%d", pool, d); + SNPRINTF_TRUNC(dsname, ZFS_MAX_DATASET_NAME_LEN, "%s/ds_%d", pool, d); } static void @@ -6420,7 +6422,7 @@ ztest_run(ztest_shared_t *zs) */ if (ztest_random(2) == 0) { char name[ZFS_MAX_DATASET_NAME_LEN]; - (void) snprintf(name, sizeof (name), "%s_import", + SNPRINTF_TRUNC(name, sizeof (name), "%s_import", ztest_opts.zo_pool); ztest_spa_import_export(ztest_opts.zo_pool, name); ztest_spa_import_export(name, ztest_opts.zo_pool); @@ -7000,7 +7002,7 @@ main(int argc, char **argv) char tmpname[ZFS_MAX_DATASET_NAME_LEN]; kernel_fini(); kernel_init(FREAD | FWRITE); - (void) snprintf(tmpname, sizeof (tmpname), "%s_tmp", + SNPRINTF_TRUNC(tmpname, sizeof (tmpname), "%s_tmp", ztest_opts.zo_pool); (void) spa_rename(tmpname, ztest_opts.zo_pool); } diff --git a/include/zfs_comutil.h b/include/zfs_comutil.h index f89054388a4d..497035946f05 100644 --- a/include/zfs_comutil.h +++ b/include/zfs_comutil.h @@ -41,6 +41,27 @@ extern int zfs_spa_version_map(int zpl_version); #define ZFS_NUM_LEGACY_HISTORY_EVENTS 41 extern const char *zfs_history_event_names[ZFS_NUM_LEGACY_HISTORY_EVENTS]; +/* + * Truncating snprintf() without compiler warnings + * + * GCC 7.1 with "-Wall" will warn in cases where we're not checking the + * snprintf() return code when the buffer could be truncated: + * + * libzfs_iter.c: In function ‘zfs_iter_bookmarks’: + * libzfs_iter.c:207:40: error: ‘snprintf’ output may be truncated + * before the last format character [-Werror=format-truncation=] + * (void) snprintf(name, sizeof (name), "%s#%s", zhp->zfs_name, + * + * In many cases we don't care and just want to silently truncate the buffer. + * + * This hack fools the compiler by making the size volatile, so that it isn't + * able to compute the truncation at compile time, and thus no error. + */ +#define SNPRINTF_TRUNC(str, size, ...) do { \ + volatile int s = size; \ + snprintf(str, s, ##__VA_ARGS__); \ +} while (0) + #ifdef __cplusplus } #endif diff --git a/lib/libspl/include/assert.h b/lib/libspl/include/assert.h index bd89ad94fa1c..27e9427264db 100644 --- a/lib/libspl/include/assert.h +++ b/lib/libspl/include/assert.h @@ -40,6 +40,24 @@ libspl_assert(const char *buf, const char *file, const char *func, int line) abort(); } +/* printf version of libspl_assert */ +static inline int +libspl_assertf(const char *file, const char *func, int line, char *format, ...) +{ + char *buf = NULL; + int size; + va_list args; + va_start(args, format); + size = vsnprintf(NULL, 0, format, args); + buf = alloca(size + 1); + if (buf == NULL) + buf = ""; + else + vsnprintf(buf, size, format, args); + va_end(args); + return (libspl_assert(buf, file, func, line)); +} + #ifdef verify #undef verify #endif @@ -55,13 +73,9 @@ libspl_assert(const char *buf, const char *file, const char *func, int line) do { \ const TYPE __left = (TYPE)(LEFT); \ const TYPE __right = (TYPE)(RIGHT); \ - if (!(__left OP __right)) { \ - char *__buf = alloca(256); \ - (void) snprintf(__buf, 256, \ - "%s %s %s (0x%llx %s 0x%llx)", #LEFT, #OP, #RIGHT, \ - (u_longlong_t)__left, #OP, (u_longlong_t)__right); \ - libspl_assert(__buf, __FILE__, __FUNCTION__, __LINE__); \ - } \ + if (!(__left OP __right)) \ + libspl_assertf(__FILE__, __FUNCTION__, __LINE__, \ + "%s %s %s (0x%llx %s 0x%llx)", #LEFT, #OP, #RIGHT); \ } while (0) #define VERIFY3S(x, y, z) VERIFY3_IMPL(x, y, z, int64_t) diff --git a/lib/libzfs/libzfs_dataset.c b/lib/libzfs/libzfs_dataset.c index f1346b69c6f4..46a40f7896e5 100644 --- a/lib/libzfs/libzfs_dataset.c +++ b/lib/libzfs/libzfs_dataset.c @@ -3566,8 +3566,9 @@ zfs_check_snap_cb(zfs_handle_t *zhp, void *arg) char name[ZFS_MAX_DATASET_NAME_LEN]; int rv = 0; - (void) snprintf(name, sizeof (name), - "%s@%s", zhp->zfs_name, dd->snapname); + if (snprintf(name, sizeof (name), "%s@%s", zhp->zfs_name, + dd->snapname) >= sizeof (name)) + return (EINVAL); if (lzc_exists(name)) verify(nvlist_add_boolean(dd->nvl, name) == 0); @@ -3788,8 +3789,9 @@ zfs_snapshot_cb(zfs_handle_t *zhp, void *arg) int rv = 0; if (zfs_prop_get_int(zhp, ZFS_PROP_INCONSISTENT) == 0) { - (void) snprintf(name, sizeof (name), - "%s@%s", zfs_get_name(zhp), sd->sd_snapname); + if (snprintf(name, sizeof (name), "%s@%s", zfs_get_name(zhp), + sd->sd_snapname) >= sizeof (name)) + return (EINVAL); fnvlist_add_boolean(sd->sd_nvl, name); @@ -4533,8 +4535,9 @@ zfs_hold_one(zfs_handle_t *zhp, void *arg) char name[ZFS_MAX_DATASET_NAME_LEN]; int rv = 0; - (void) snprintf(name, sizeof (name), - "%s@%s", zhp->zfs_name, ha->snapname); + if (snprintf(name, sizeof (name), "%s@%s", zhp->zfs_name, + ha->snapname) >= sizeof (name)) + return (EINVAL); if (lzc_exists(name)) fnvlist_add_string(ha->nvl, name, ha->tag); @@ -4653,8 +4656,11 @@ zfs_release_one(zfs_handle_t *zhp, void *arg) int rv = 0; nvlist_t *existing_holds; - (void) snprintf(name, sizeof (name), - "%s@%s", zhp->zfs_name, ha->snapname); + if (snprintf(name, sizeof (name), "%s@%s", zhp->zfs_name, + ha->snapname) >= sizeof (name)) { + ha->error = EINVAL; + rv = EINVAL; + } if (lzc_get_holds(name, &existing_holds) != 0) { ha->error = ENOENT; diff --git a/lib/libzfs/libzfs_iter.c b/lib/libzfs/libzfs_iter.c index d78c757a58b7..57ebdd89d1b8 100644 --- a/lib/libzfs/libzfs_iter.c +++ b/lib/libzfs/libzfs_iter.c @@ -204,8 +204,11 @@ zfs_iter_bookmarks(zfs_handle_t *zhp, zfs_iter_f func, void *data) bmark_name = nvpair_name(pair); bmark_props = fnvpair_value_nvlist(pair); - (void) snprintf(name, sizeof (name), "%s#%s", zhp->zfs_name, - bmark_name); + if (snprintf(name, sizeof (name), "%s#%s", zhp->zfs_name, + bmark_name) >= sizeof (name)) { + err = EINVAL; + goto out; + } nzhp = make_bookmark_handle(zhp, name, bmark_props); if (nzhp == NULL) diff --git a/lib/libzfs/libzfs_sendrecv.c b/lib/libzfs/libzfs_sendrecv.c index 71ee8faaeaa3..ff909f1e3574 100644 --- a/lib/libzfs/libzfs_sendrecv.c +++ b/lib/libzfs/libzfs_sendrecv.c @@ -1867,9 +1867,13 @@ zfs_send(zfs_handle_t *zhp, const char *fromsnap, const char *tosnap, drr_versioninfo, DMU_COMPOUNDSTREAM); DMU_SET_FEATUREFLAGS(drr.drr_u.drr_begin. drr_versioninfo, featureflags); - (void) snprintf(drr.drr_u.drr_begin.drr_toname, + if (snprintf(drr.drr_u.drr_begin.drr_toname, sizeof (drr.drr_u.drr_begin.drr_toname), - "%s@%s", zhp->zfs_name, tosnap); + "%s@%s", zhp->zfs_name, tosnap) >= + sizeof (drr.drr_u.drr_begin.drr_toname)) { + err = EINVAL; + goto stderr_out; + } drr.drr_payloadlen = buflen; err = dump_record(&drr, packbuf, buflen, &zc, outfd); diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index 4fda36c7f047..ec1754dab397 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -3627,8 +3627,10 @@ zfs_ioc_destroy(zfs_cmd_t *zc) */ char namebuf[ZFS_MAX_DATASET_NAME_LEN + 6]; - (void) snprintf(namebuf, sizeof (namebuf), - "%s/%s", zc->zc_name, recv_clone_name); + if (snprintf(namebuf, sizeof (namebuf), "%s/%s", + zc->zc_name, recv_clone_name) >= + sizeof (namebuf)) + return (EINVAL); /* * Try to remove the hidden child (%recv) and after diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c index 8890aaaf78a3..6b25037c44b5 100644 --- a/module/zfs/zvol.c +++ b/module/zfs/zvol.c @@ -79,6 +79,7 @@ #include #include #include +#include #include unsigned int zvol_inhibit_dev = 0; @@ -2086,7 +2087,7 @@ zvol_rename_minors_impl(const char *oldname, const char *newname) } else if (strncmp(zv->zv_name, oldname, oldnamelen) == 0 && (zv->zv_name[oldnamelen] == '/' || zv->zv_name[oldnamelen] == '@')) { - snprintf(name, MAXNAMELEN, "%s%c%s", newname, + SNPRINTF_TRUNC(name, MAXNAMELEN, "%s%c%s", newname, zv->zv_name[oldnamelen], zv->zv_name + oldnamelen + 1); zvol_rename_minor(zv, name);