From c696a226ea5410da48677b47b21256987fe8ecd4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Dec 2017 16:38:42 +0100 Subject: [PATCH 1/6] all: don't use NM_FLAGS_HAS() with non-constant argument NM_FLAGS_HAS() uses a static-assert that the second argument is a single flag (power of two). With a single flag, NM_FLAGS_HAS(), NM_FLAGS_ANY() and NM_FLAGS_ALL() are all identical. The second argument must be a compile time constant, and if that is not the case, one must not use NM_FLAGS_HAS(). Use NM_FLAGS_ANY() in these cases. --- libnm-core/nm-setting-bond.c | 2 +- src/devices/nm-device.c | 2 +- src/nm-audit-manager.c | 2 +- src/nm-logging.c | 4 ++-- src/platform/nm-linux-platform.c | 2 +- src/platform/nmp-netns.c | 4 ++-- src/platform/tests/test-link.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 13bb4a8a72..829cda2ef2 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -537,7 +537,7 @@ _nm_setting_bond_option_supported (const char *option, NMBondMode mode) for (i = 0; i < G_N_ELEMENTS (bond_unsupp_modes); i++) { if (nm_streq (option, bond_unsupp_modes[i].option)) - return !NM_FLAGS_HAS (bond_unsupp_modes[i].unsupp_modes, BIT (mode)); + return !NM_FLAGS_ANY (bond_unsupp_modes[i].unsupp_modes, BIT (mode)); } return TRUE; diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index fda2126410..62e4a67cff 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -12246,7 +12246,7 @@ nm_device_check_connection_available (NMDevice *self, for (i = 0; i <= NM_DEVICE_CHECK_CON_AVAILABLE_ALL; i++) { for (j = 1; j <= NM_DEVICE_CHECK_CON_AVAILABLE_ALL; j <<= 1) { - if (NM_FLAGS_HAS (i, j)) { + if (NM_FLAGS_ANY (i, j)) { k = i & ~j; nm_assert ( available_all[i] == available_all[k] || available_all[i]); diff --git a/src/nm-audit-manager.c b/src/nm-audit-manager.c index 2d75c0e715..45d068f9ae 100644 --- a/src/nm-audit-manager.c +++ b/src/nm-audit-manager.c @@ -123,7 +123,7 @@ build_message (GPtrArray *fields, AuditBackend backend) for (i = 0; i < fields->len; i++) { field = fields->pdata[i]; - if (!NM_FLAGS_HAS (field->backends, backend)) + if (!NM_FLAGS_ANY (field->backends, backend)) continue; if (first) diff --git a/src/nm-logging.c b/src/nm-logging.c index f19f0de194..4ef786f6e9 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -663,7 +663,7 @@ _nm_log_impl (const char *file, NMLogDomain dom = dom_all & _nm_logging_enabled_state[level]; for (diter = &global.domain_desc[0]; diter->name; diter++) { - if (!NM_FLAGS_HAS (dom_all, diter->num)) + if (!NM_FLAGS_ANY (dom_all, diter->num)) continue; /* construct a list of all domains (not only the enabled ones). @@ -681,7 +681,7 @@ _nm_log_impl (const char *file, g_string_append (s_domain_all, diter->name); } - if (NM_FLAGS_HAS (dom, diter->num)) { + if (NM_FLAGS_ANY (dom, diter->num)) { if (i_domain > 0) { /* SYSLOG_FACILITY is specified multiple times for each domain that is actually enabled. */ _iovec_set_format_a (iov++, _MAX_LEN (30, diter->name), "SYSLOG_FACILITY=%s", diter->name); diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index eac829ff5b..76285dd7d7 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -283,7 +283,7 @@ typedef enum { #define FOR_EACH_DELAYED_ACTION(iflags, flags_all) \ for ((iflags) = (DelayedActionType) 0x1LL; (iflags) <= DELAYED_ACTION_TYPE_MAX; (iflags) <<= 1) \ - if (NM_FLAGS_HAS (flags_all, iflags)) + if (NM_FLAGS_ANY (flags_all, iflags)) typedef enum { /* Negative values are errors from kernel. Add dummy member to diff --git a/src/platform/nmp-netns.c b/src/platform/nmp-netns.c index bc305f0170..e31c881d66 100644 --- a/src/platform/nmp-netns.c +++ b/src/platform/nmp-netns.c @@ -200,8 +200,8 @@ _stack_current_ns_types (NMPNetns *netns, int ns_types) } for (i = 0; i < G_N_ELEMENTS (ns_types_check); i++) { - if ( NM_FLAGS_HAS (ns_types, ns_types_check[i]) - && NM_FLAGS_HAS (info->ns_types, ns_types_check[i])) { + if ( NM_FLAGS_ANY (ns_types, ns_types_check[i]) + && NM_FLAGS_ANY (info->ns_types, ns_types_check[i])) { res = NM_FLAGS_SET (res, ns_types_check[i]); ns_types = NM_FLAGS_UNSET (ns_types, ns_types_check[i]); } diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index aedf4a49c2..ad25ea577b 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -2230,7 +2230,7 @@ test_netns_push (gpointer fixture, gconstpointer test_data) p = pl_base; for (j = nstack; j >= 1; ) { j--; - if (NM_FLAGS_HAS (stack[j].ns_types, ns_type)) { + if (NM_FLAGS_ANY (stack[j].ns_types, ns_type)) { p = stack[j].pl; break; } From ea1f77f91163e86265a1e1a34577ce2db8ca57ea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Dec 2017 18:21:34 +0100 Subject: [PATCH 2/6] core: don't use variable length arrays Let's compile with -Wvla, so that G_STATIC_ASSERT() is really static. But then we cannot use variable length arrays. --- libnm-core/tests/test-general.c | 4 +++- src/nm-ip4-config.c | 9 +++++---- src/nm-ip6-config.c | 9 +++++---- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 88eb4bc85f..da7ea7b1f8 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -6206,7 +6206,9 @@ _test_find_binary_search_do_uint32 (const int *int_array, gsize len) const int OFFSET = 100; const int NEEDLE = 0 + OFFSET; gssize expected_result = -1; - guint32 array[len]; + guint32 array[30]; + + g_assert (len <= G_N_ELEMENTS (array)); /* the test data has negative values. Shift them... */ for (idx = 0; idx < len; idx++) { diff --git a/src/nm-ip4-config.c b/src/nm-ip4-config.c index 1f98ed08c4..33d1f27507 100644 --- a/src/nm-ip4-config.c +++ b/src/nm-ip4-config.c @@ -2879,9 +2879,9 @@ nm_ip4_config_equal (const NMIP4Config *a, const NMIP4Config *b) { GChecksum *a_checksum = g_checksum_new (G_CHECKSUM_SHA1); GChecksum *b_checksum = g_checksum_new (G_CHECKSUM_SHA1); - gsize a_len = g_checksum_type_get_length (G_CHECKSUM_SHA1); - gsize b_len = g_checksum_type_get_length (G_CHECKSUM_SHA1); - guchar a_data[a_len], b_data[b_len]; + guchar a_data[20], b_data[20]; + gsize a_len = sizeof (a_data); + gsize b_len = sizeof (b_data); gboolean equal; if (a) @@ -2892,7 +2892,8 @@ nm_ip4_config_equal (const NMIP4Config *a, const NMIP4Config *b) g_checksum_get_digest (a_checksum, a_data, &a_len); g_checksum_get_digest (b_checksum, b_data, &b_len); - g_assert (a_len == b_len); + nm_assert (a_len == sizeof (a_data)); + nm_assert (b_len == sizeof (b_data)); equal = !memcmp (a_data, b_data, a_len); g_checksum_free (a_checksum); diff --git a/src/nm-ip6-config.c b/src/nm-ip6-config.c index 7c6b0ee604..d2a08d0ceb 100644 --- a/src/nm-ip6-config.c +++ b/src/nm-ip6-config.c @@ -2477,9 +2477,9 @@ nm_ip6_config_equal (const NMIP6Config *a, const NMIP6Config *b) { GChecksum *a_checksum = g_checksum_new (G_CHECKSUM_SHA1); GChecksum *b_checksum = g_checksum_new (G_CHECKSUM_SHA1); - gsize a_len = g_checksum_type_get_length (G_CHECKSUM_SHA1); - gsize b_len = g_checksum_type_get_length (G_CHECKSUM_SHA1); - guchar a_data[a_len], b_data[b_len]; + guchar a_data[20], b_data[20]; + gsize a_len = sizeof (a_data); + gsize b_len = sizeof (b_data); gboolean equal; if (a) @@ -2490,7 +2490,8 @@ nm_ip6_config_equal (const NMIP6Config *a, const NMIP6Config *b) g_checksum_get_digest (a_checksum, a_data, &a_len); g_checksum_get_digest (b_checksum, b_data, &b_len); - g_assert (a_len == b_len); + nm_assert (a_len == sizeof (a_data)); + nm_assert (b_len == sizeof (b_data)); equal = !memcmp (a_data, b_data, a_len); g_checksum_free (a_checksum); From 08d41ada7a69af90d3d685c4a426a71d42dd4ca2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Dec 2017 20:29:42 +0100 Subject: [PATCH 3/6] tests: fix invalid static-asserts with non-const expression The expression is not const. Use a run-time assert instead --- src/platform/tests/test-nmp-object.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/src/platform/tests/test-nmp-object.c b/src/platform/tests/test-nmp-object.c index 3228de8347..047d7a854b 100644 --- a/src/platform/tests/test-nmp-object.c +++ b/src/platform/tests/test-nmp-object.c @@ -54,27 +54,20 @@ test_obj_base (void) gs_unref_object GCancellable *obj_cancellable = g_cancellable_new (); nm_auto_nmpobj NMPObject *obj_link = nmp_object_new_link (10); -#define STATIC_ASSERT(cond) \ - G_STMT_START { \ - G_STATIC_ASSERT (cond); \ - G_STATIC_ASSERT_EXPR (cond); \ - g_assert (cond); \ - } G_STMT_END + g_assert (&g->g_type_instance == (void *) &o->_class); + g_assert (&g->g_type_instance.g_class == (void *) &o->_class); - STATIC_ASSERT (&g->g_type_instance == (void *) &o->_class); - STATIC_ASSERT (&g->g_type_instance.g_class == (void *) &o->_class); + g_assert (sizeof (o->parent.parent) == sizeof (GTypeInstance)); - STATIC_ASSERT (sizeof (o->parent.parent) == sizeof (GTypeInstance)); + g_assert (&c->parent == (void *) c); + g_assert (&c->parent.parent.g_type_class == (void *) c); + g_assert (&c->parent.parent.g_type == (void *) c); + g_assert (&c->parent.parent.g_type == &k->g_type); - STATIC_ASSERT (&c->parent == (void *) c); - STATIC_ASSERT (&c->parent.parent.g_type_class == (void *) c); - STATIC_ASSERT (&c->parent.parent.g_type == (void *) c); - STATIC_ASSERT (&c->parent.parent.g_type == &k->g_type); + g_assert (sizeof (c->parent.parent) == sizeof (GTypeClass)); - STATIC_ASSERT (sizeof (c->parent.parent) == sizeof (GTypeClass)); - - STATIC_ASSERT (&o->parent == (void *) o); - STATIC_ASSERT (&o->parent.klass == (void *) &o->_class); + g_assert (&o->parent == (void *) o); + g_assert (&o->parent.klass == (void *) &o->_class); obj = (NMObjBaseInst *) obj_cancellable; g_assert (!NMP_CLASS_IS_VALID ((NMPClass *) obj->klass)); From 96ae9309e7204757987263a5ba49e5a7a1b7c031 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 15:49:39 +0100 Subject: [PATCH 4/6] core: don't use const integers where const expression is needed --- src/main.c | 6 ++++-- src/nm-core-utils.c | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main.c b/src/main.c index d59da0522a..6984d52dda 100644 --- a/src/main.c +++ b/src/main.c @@ -93,8 +93,10 @@ static void _init_nm_debug (NMConfig *config) { gs_free char *debug = NULL; - const guint D_RLIMIT_CORE = 1; - const guint D_FATAL_WARNINGS = 2; + enum { + D_RLIMIT_CORE = (1 << 0), + D_FATAL_WARNINGS = (1 << 1), + }; GDebugKey keys[] = { { "RLIMIT_CORE", D_RLIMIT_CORE }, { "fatal-warnings", D_FATAL_WARNINGS }, diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index f6b33a1490..197fa49ba9 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -1246,8 +1246,8 @@ typedef struct { static gboolean match_device_s390_subchannels_parse (const char *s390_subchannels, guint32 *out_a, guint32 *out_b, guint32 *out_c) { - const int BUFSIZE = 30; - char buf[BUFSIZE + 1]; + char buf[30 + 1]; + const int BUFSIZE = G_N_ELEMENTS (buf) - 1; guint i = 0; char *pa = NULL, *pb = NULL, *pc = NULL; gint64 a, b, c; From 9c3402aa1e145420b868359e993b0a38ae9a6408 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Dec 2017 16:24:42 +0100 Subject: [PATCH 5/6] build: add "-Wvla" to warn about uses of variable-length arrays We don't use them, so add a compiler warning about their use. I think they are annoying, because sizeof(x) and typeof(x) might evaluate at runtime. Especially the typeof() extension is useful for macros, but behaves badly with variable-length arrays due to running code at runtime. But the worst is, G_STATIC_ASSERT() is implemented by declaring an array of negative length. Usually, the checked condition should be a compile time constant, but with VLAs the compiler would not evaluate the static-assert at compile time and instead accept it silently. It's easy to mess up static asserts to wrongly have a non-constant condition, especially when the static-assert is used inside a macro. Just say no. --- m4/compiler_options.m4 | 1 + 1 file changed, 1 insertion(+) diff --git a/m4/compiler_options.m4 b/m4/compiler_options.m4 index ccb51f5e6e..c0b07e5b4b 100644 --- a/m4/compiler_options.m4 +++ b/m4/compiler_options.m4 @@ -79,6 +79,7 @@ if test "$GCC" = "yes" -a "$set_more_warnings" != "no"; then -Wshift-negative-value \ -Wstrict-prototypes \ -Wundef \ + -Wvla \ -Wno-duplicate-decl-specifier \ -Wno-format-truncation \ -Wno-format-y2k \ From 974501fdcfa91547539ad1049c0cb3339c08efb3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Dec 2017 12:31:55 +0100 Subject: [PATCH 6/6] shared: add static assert for nm_g_slice_free_fcn() argument --- libnm-core/tests/test-general.c | 7 +++++++ shared/nm-utils/nm-shared-utils.h | 10 ++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index da7ea7b1f8..ba34e6e9e7 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -210,6 +210,10 @@ static void test_nm_g_slice_free_fcn (void) { gpointer p; + struct { + char a1; + char a2; + } xx; p = g_slice_new (gint64); (nm_g_slice_free_fcn (gint64)) (p); @@ -222,6 +226,9 @@ test_nm_g_slice_free_fcn (void) p = g_slice_new (gint64); nm_g_slice_free_fcn_gint64 (p); + + p = g_slice_alloc (sizeof (xx)); + (nm_g_slice_free_fcn (xx)) (p); } /*****************************************************************************/ diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 89ad8b8d48..17024d591f 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -326,12 +326,18 @@ _nm_g_slice_free_fcn_define (16) /* If mem_size is a compile time constant, the compiler * will be able to optimize this. Hence, you don't want * to call this with a non-constant size argument. */ \ - switch (mem_size) { \ + G_STATIC_ASSERT_EXPR ( ((mem_size) == 1) \ + || ((mem_size) == 2) \ + || ((mem_size) == 4) \ + || ((mem_size) == 8) \ + || ((mem_size) == 12) \ + || ((mem_size) == 16)); \ + switch ((mem_size)) { \ case 1: _fcn = _nm_g_slice_free_fcn_1; break; \ case 2: _fcn = _nm_g_slice_free_fcn_2; break; \ case 4: _fcn = _nm_g_slice_free_fcn_4; break; \ case 8: _fcn = _nm_g_slice_free_fcn_8; break; \ - case 12: _fcn = _nm_g_slice_free_fcn_12; break; \ + case 12: _fcn = _nm_g_slice_free_fcn_12; break; \ case 16: _fcn = _nm_g_slice_free_fcn_16; break; \ default: g_assert_not_reached (); _fcn = NULL; break; \ } \