GCC 7.1 fixes #6253

tonyhutter · 2017-06-20T19:10:30Z

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 simply disables the warnings (ztest.c).

Signed-off-by: Tony Hutter hutter2@llnl.gov

Motivation and Context

Fixes #6221

How Has This Been Tested?

Built on Fedora 26 alpha with gcc 7.1.1

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

dinatale2

This may be an oversimplification, but couldn't you provide a #define TRUNC(func) if (func) {; } then all your snprintf calls can become something like TRUNC(snprintf(....));. Thoughts?

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 simply disables the warnings (ztest.c). Signed-off-by: Tony Hutter <hutter2@llnl.gov>

tonyhutter · 2017-06-22T18:38:58Z

@dinatale2 unfortunately I get the same build errors with that since the gcc still knows the size at compile time. This hack seems to work though:

#define SNPRINTF_TRUNC(str, size, ...) {volatile int s = size; if (snprintf(str, s, __VA_ARGS__)) {; }}

I've updated the code with this in my latest push.

tonyhutter · 2017-06-22T19:04:14Z

Looks like I don't need the if() in SNPRINTF_TRUNC anymore - removed in latest push.

tuxoko · 2017-06-23T18:49:24Z

lib/libspl/include/assert.h

-		char *__buf = alloca(256);				\
-		(void) snprintf(__buf, 256,				\
+		char *__buf = NULL;					\
+		int rc = asprintf(&__buf,				\


asprintf will require a free. But I don't see why we need to do this. Why not pass the format strings and variable all the way, or pass va_list?

The other four callers of libspl_assert() just pass it a const string, so it makes sense to have VERIFY3_IMPL pass in a string as well (rather than a format string + vars).

I'll get rid of the asprintf and use a "snprintf to get size + alloca(size)" to allocate __buf.

Then how about adding a libspl_assertf?

Works for me

tuxoko · 2017-06-23T18:50:15Z

include/zfs_comutil.h

+ */
+#define	SNPRINTF_TRUNC(str, size, ...)	{				\
+	volatile int s = size;						\
+	snprintf(str, s, __VA_ARGS__);					\


I think it should be ##__VA_ARGS__ to allow zero arguments.
Also, wrap this in do { } while (0)

Good catch, will fix

What's the purpose for the volatile?
Shouldn't it be fine if you pass in size directly, as it's only used once?

volatile fools gcc into not computing the buffer size at compile time, and thus prevents the compiler warning about snprintf truncation.

tuxoko · 2017-06-23T22:34:04Z

lib/libspl/include/assert.h

+	va_list args;
+	va_start(args, format);
+	size = vsnprintf(NULL, 0, format, args);
+	buf = alloca(size + 1);


What I meant was feed the format string directly to fprintf, so we don't need to allocate any thing.
Edit, or I should say vfprintf.

tuxoko · 2017-06-23T22:35:20Z

include/zfs_comutil.h

+ * able to compute the truncation at compile time, and thus no error.
+ */
+#define	SNPRINTF_TRUNC(str, size, ...) do {				\
+	volatile int s = size;						\


If this is required then you should change it to __s to prevent name collision.

tonyhutter · 2017-06-23T22:50:34Z

@tuxoko my latest push should have all the fixes you were looking for

behlendorf · 2017-06-26T21:12:40Z

include/zfs_comutil.h

+#define	SNPRINTF_TRUNC(str, size, ...) do {				\
+	volatile int __s = size;					\
+	snprintf(str, __s, ##__VA_ARGS__);				\
+} while (0)


With the single exception in zvol.c it looks like SNPRINTF_TRUNC is only needed by ztest.c. In order to avoid tempting developers to use the wrapper in the core ZFS code, where there needs to be proper error handling and we want gcc to warn us, let's move this macro in to ztest.c. The usage in zvol_rename_minors_impl() can be fixed by using kmem_asprintf() to always dynamically allocate a long enough buffer.

And with the only semi-legitimate usage of this now occurring in ztest.c we should be able to eliminate the need for this macro entirely by building ztest with -Wnoformat-truncation. Here's an untested patch, you may need to structure this like the ZFS_AC_CONFIG_USER_FRAME_LARGER_THAN to ensure the version of gcc being used supports the option. Alternately a pragma could be used for this.

diff --git a/cmd/ztest/Makefile.am b/cmd/ztest/Makefile.am index a2f3b5a..c7ac681 100644 --- a/cmd/ztest/Makefile.am +++ b/cmd/ztest/Makefile.am @@ -1,6 +1,6 @@ include $(top_srcdir)/config/Rules.am -AM_CFLAGS += $(DEBUG_STACKFLAGS) $(FRAME_LARGER_THAN) +AM_CFLAGS += $(DEBUG_STACKFLAGS) $(FRAME_LARGER_THAN) -Wnoformat-truncation AM_CPPFLAGS += -DDEBUG DEFAULT_INCLUDES += \

tonyhutter · 2017-06-27T00:16:36Z

@behlendorf all comments fixed in latest push

tonyhutter · 2017-06-27T00:40:21Z

Updated commit message to remove the mention of snprintf_trunc

behlendorf · 2017-06-27T01:37:11Z

config/no-format-truncation.m4

+	])
+
+	CFLAGS="$saved_flags"
+        AC_SUBST([NO_FORMAT_TRUNCATION])


ctyle: spaces instead of tabs

behlendorf · 2017-06-27T01:49:51Z

module/zfs/zvol.c

@@ -2086,16 +2085,18 @@ 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,
+			name = kmem_asprintf("%s%c%s", newname,


nit: kmem_asprintf() using KM_SLEEP internally and cannot fail so we can drop the if (name != NULL) checks. What do you think about the following to simplify this.

diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c index 8890aaa..21d4467 100644 --- a/module/zfs/zvol.c +++ b/module/zfs/zvol.c @@ -2063,14 +2063,12 @@ zvol_rename_minors_impl(const char *oldname, const char { zvol_state_t *zv, *zv_next; int oldnamelen, newnamelen; - char *name; if (zvol_inhibit_dev) return; oldnamelen = strlen(oldname); newnamelen = strlen(newname); - name = kmem_alloc(MAXNAMELEN, KM_SLEEP); mutex_enter(&zvol_state_lock); @@ -2086,16 +2084,15 @@ zvol_rename_minors_impl(const char *oldname, const char } else if (strncmp(zv->zv_name, oldname, oldnamelen) == 0 && (zv->zv_name[oldnamelen] == '/' || zv->zv_name[oldnamelen] == '@')) { - snprintf(name, MAXNAMELEN, "%s%c%s", newname, + char *name = kmem_asprintf("%s%c%s", newname, zv->zv_name[oldnamelen], zv->zv_name + oldnamelen + 1); zvol_rename_minor(zv, name); + kmem_free(name, strlen(name + 1)); } } mutex_exit(&zvol_state_lock); - - kmem_free(name, MAXNAMELEN); } typedef struct zvol_snapdev_cb_arg {

behlendorf · 2017-06-27T01:53:52Z

module/zfs/zfs_ioctl.c

+			if (snprintf(namebuf, sizeof (namebuf), "%s/%s",
+			    zc->zc_name, recv_clone_name) >=
+			    sizeof (namebuf))
+				return (EINVAL);


nit: return (SET_ERROR(EINVAL));.

behlendorf

One last thing!

behlendorf · 2017-06-27T18:19:03Z

config/no-format-truncation.m4

@@ -0,0 +1,22 @@
+dnl #


This file should be renamed user-no-format-truncation.m4 since the configure check is only run as part of the user space build.

Ah yes, will rename

tonyhutter · 2017-06-27T21:55:56Z

@behlendorf file renamed in latest push

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 simply disables the warnings (ztest.c). Reviewed-by: Chunwei Chen <david.chen@osnexus.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Closes openzfs#6253

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 simply disables the warnings (ztest.c). Reviewed-by: Chunwei Chen <david.chen@osnexus.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Closes #6253

tonyhutter added the Type: Building Indicates an issue related to building binaries label Jun 20, 2017

dinatale2 reviewed Jun 20, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch from 2586e21 to 131432c Compare June 22, 2017 18:36

tonyhutter force-pushed the gcc7-fix branch from 0a7470a to 023a365 Compare June 22, 2017 19:02

tuxoko suggested changes Jun 23, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch from 023a365 to 9a8e467 Compare June 23, 2017 22:30

tuxoko suggested changes Jun 23, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch from 9a8e467 to bab727e Compare June 23, 2017 22:48

tuxoko approved these changes Jun 23, 2017

View reviewed changes

behlendorf approved these changes Jun 26, 2017

View reviewed changes

behlendorf requested changes Jun 26, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch from bab727e to 39c3d67 Compare June 26, 2017 23:22

behlendorf approved these changes Jun 27, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch from 39c3d67 to e7940d9 Compare June 27, 2017 00:38

behlendorf requested changes Jun 27, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch 2 times, most recently from 1425695 to 0f8523d Compare June 27, 2017 17:39

behlendorf approved these changes Jun 27, 2017

View reviewed changes

behlendorf requested changes Jun 27, 2017

View reviewed changes

tonyhutter force-pushed the gcc7-fix branch from 0f8523d to 4664d80 Compare June 27, 2017 20:00

behlendorf approved these changes Jun 27, 2017

View reviewed changes

behlendorf merged commit 682ce10 into openzfs:master Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCC 7.1 fixes #6253

GCC 7.1 fixes #6253

tonyhutter commented Jun 20, 2017 •

edited

Loading

dinatale2 left a comment •

edited

Loading

tonyhutter commented Jun 22, 2017

tonyhutter commented Jun 22, 2017

tuxoko Jun 23, 2017

tonyhutter Jun 23, 2017

tuxoko Jun 23, 2017

tonyhutter Jun 23, 2017

tuxoko Jun 23, 2017

tonyhutter Jun 23, 2017

tuxoko Jun 23, 2017 •

edited

Loading

tonyhutter Jun 23, 2017 •

edited

Loading

tuxoko Jun 23, 2017 •

edited

Loading

tuxoko Jun 23, 2017

tonyhutter commented Jun 23, 2017

behlendorf Jun 26, 2017

behlendorf Jun 26, 2017

tonyhutter commented Jun 27, 2017

tonyhutter commented Jun 27, 2017

behlendorf Jun 27, 2017

behlendorf Jun 27, 2017

behlendorf Jun 27, 2017

behlendorf left a comment

behlendorf Jun 27, 2017

tonyhutter Jun 27, 2017

tonyhutter commented Jun 27, 2017

GCC 7.1 fixes #6253

GCC 7.1 fixes #6253

Conversation

tonyhutter commented Jun 20, 2017 • edited Loading

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

dinatale2 left a comment • edited Loading

Choose a reason for hiding this comment

tonyhutter commented Jun 22, 2017

tonyhutter commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxoko Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

tonyhutter Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

tuxoko Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhutter commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhutter commented Jun 27, 2017

tonyhutter commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhutter commented Jun 27, 2017

tonyhutter commented Jun 20, 2017 •

edited

Loading

dinatale2 left a comment •

edited

Loading

tuxoko Jun 23, 2017 •

edited

Loading

tonyhutter Jun 23, 2017 •

edited

Loading

tuxoko Jun 23, 2017 •

edited

Loading