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(-) 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) { -- 2.14.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.
> -----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