diff mbox series

hash: fix possible uninitialized variable

Message ID 20181222121100.52827-1-haiyangtan@tencent.com (mailing list archive)
State Rejected, archived
Headers
Series hash: fix possible uninitialized variable |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Haiyang Tan Dec. 22, 2018, 12:10 p.m. UTC 
  The uninitialized field 'extra_flag' of hash_cuckoo_params may enable
certain feature silently. Typically, if bit0 of 'extra_flag' set, the
hardware transactional memory support will be enabled unexpectedly.

Signed-off-by: Haiyang Tan <haiyangtan@tencent.com>
---
 lib/librte_table/rte_table_hash_cuckoo.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Stephen Hemminger Dec. 23, 2018, 7:43 a.m. UTC | #1 
On Sat, 22 Dec 2018 04:10:59 -0800
Haiyang Tan <haiyangtan@tencent.com> wrote:

> The uninitialized field 'extra_flag' of hash_cuckoo_params may enable
> certain feature silently. Typically, if bit0 of 'extra_flag' set, the
> hardware transactional memory support will be enabled unexpectedly.
> 
> Signed-off-by: Haiyang Tan <haiyangtan@tencent.com>

This is not necessary. Structure initializations will fill in the other
elements with zero.

https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Designated-Inits.html
  Omitted field members are implicitly initialized the same as objects that have static storage duration.
Cristian Dumitrescu Dec. 24, 2018, 10:30 a.m. UTC | #2 
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Sunday, December 23, 2018 7:44 AM
> To: Haiyang Tan <haiyangtan@tencent.com>
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] hash: fix possible uninitialized variable
> 
> On Sat, 22 Dec 2018 04:10:59 -0800
> Haiyang Tan <haiyangtan@tencent.com> wrote:
> 
> > The uninitialized field 'extra_flag' of hash_cuckoo_params may enable
> > certain feature silently. Typically, if bit0 of 'extra_flag' set, the
> > hardware transactional memory support will be enabled unexpectedly.
> >
> > Signed-off-by: Haiyang Tan <haiyangtan@tencent.com>
> 
> This is not necessary. Structure initializations will fill in the other
> elements with zero.
> 
> https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Designated-Inits.html
>   Omitted field members are implicitly initialized the same as objects that
> have static storage duration.

Agree with Stephen, this is part of the C language.

Haiyang, are you experiencing a real issue in your app or is your proposal triggered purely by code review?

Haiyang, it seems that the extra_flags was recently added in librte_hash, but left behind in librte_table. For better readability, I suggest you send a quick patch that explicitly initializes the extra_flag with 0, what do you think?

	struct rte_hash_parameters hash_cuckoo_params = {
		...
		.extra_flag = 0,
	};

Regards,
Cristian
diff mbox series

Patch

diff --git a/lib/librte_table/rte_table_hash_cuckoo.c b/lib/librte_table/rte_table_hash_cuckoo.c
index f02430333..25fc3a10b 100644
--- a/lib/librte_table/rte_table_hash_cuckoo.c
+++ b/lib/librte_table/rte_table_hash_cuckoo.c
@@ -82,6 +82,7 @@  rte_table_hash_cuckoo_create(void *params,
 			uint32_t entry_size)
 {
 	struct rte_table_hash_cuckoo_params *p = params;
+	struct rte_hash_parameters hash_cuckoo_params = { 0 };
 	struct rte_hash *h_table;
 	struct rte_table_hash *t;
 	uint32_t total_size;
@@ -103,14 +104,12 @@  rte_table_hash_cuckoo_create(void *params,
 	}
 
 	/* Create cuckoo hash table */
-	struct rte_hash_parameters hash_cuckoo_params = {
-		.entries = p->n_keys,
-		.key_len = p->key_size,
-		.hash_func = p->f_hash,
-		.hash_func_init_val = p->seed,
-		.socket_id = socket_id,
-		.name = p->name
-	};
+	hash_cuckoo_params.entries = p->n_keys;
+	hash_cuckoo_params.key_len = p->key_size;
+	hash_cuckoo_params.hash_func = p->f_hash;
+	hash_cuckoo_params.hash_func_init_val = p->seed;
+	hash_cuckoo_params.socket_id = socket_id;
+	hash_cuckoo_params.name = p->name;
 
 	h_table = rte_hash_find_existing(p->name);
 	if (h_table == NULL) {