|SUCCESS| pw159214 [PATCH v5 03/17] hash: add checks for hash name length

[0-day Robot <robot@bytheb.org>]

Test-Label: 0day Robot - Claude SONNET
Test-Status: SUCCESS
http://dpdk.org/patch/159214

_AI Review Comments Available_

git sha: [Could not determine from patch content]
Author: Stephen Hemminger <stephen@networkplumber.org>
Subject: [PATCH v5 03/17] hash: add checks for hash name length

This commit adds name length validation to hash creation functions and 
improves error handling for ring creation failures. It also adds tests 
for the new validation logic.

> diff --git a/app/test/test_hash.c b/app/test/test_hash.c
> index 5791fd7f4c..0b608a3d74 100644
> --- a/app/test/test_hash.c
> +++ b/app/test/test_hash.c
> @@ -1120,6 +1120,14 @@ fbk_hash_unit_test(void)
>  		.socket_id = RTE_MAX_NUMA_NODES + 1, /* invalid socket */
>  	};
>  
> +	/* try and create hash with an excessively long name */
> +	struct rte_fbk_hash_params invalid_params_long_name = {
> +		.name = "four_byte_key_hash_name_length_32",
> +		.entries = 4,
> +		.entries_per_bucket = 2,
> +		.socket_id = 0,
> +	};
> +

Is this test name actually long enough to trigger the validation? The
test name appears to be exactly 32 characters but the test should verify
it exceeds RTE_FBK_HASH_NAMESIZE.

[ ... ]

> diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> index 2c92c51624..f9c4a0e302 100644
> --- a/lib/hash/rte_cuckoo_hash.c
> +++ b/lib/hash/rte_cuckoo_hash.c
> @@ -278,12 +278,16 @@ rte_hash_create(const struct rte_hash_parameters *params)
>  	else
>  		num_key_slots = params->entries + 1;
>  
> -	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
> +	/* Ring name may get truncated, conflict detected on ring creation */
> +	if (snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name)
> +	    >= (int)sizeof(ring_name))
> +		HASH_LOG(NOTICE, "ring name truncated to '%s'", ring_name);
> +

Can this truncation check be reached given the earlier name length
validation in rte_hash_create()? The validation should prevent names
that would cause truncation when prefixed with "HT_".

> @@ -1621,8 +1639,9 @@ rte_hash_rcu_qsbr_add(struct rte_hash *h, struct rte_hash_rcu_config *cfg)
>  		h->dq = rte_rcu_qsbr_dq_create(&params);
>  		if (h->dq == NULL) {
>  			rte_free(hash_rcu_cfg);
> -			HASH_LOG(ERR, "HASH defer queue creation failed");
> +			HASH_LOG(ERR, "HASH defer queue creation failed: %s",
> +				 rte_strerror(rte_errno));

Does rte_rcu_qsbr_dq_create() guarantee that rte_errno is set on failure?
Without this guarantee, the error message might be misleading.

> diff --git a/lib/hash/rte_fbk_hash.c b/lib/hash/rte_fbk_hash.c
> index 38b15a14d1..2304db7551 100644
> --- a/lib/hash/rte_fbk_hash.c
> +++ b/lib/hash/rte_fbk_hash.c
> @@ -83,7 +83,7 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
>  {
>  	struct rte_fbk_hash_table *ht = NULL;
>  	struct rte_tailq_entry *te;
> -	char hash_name[RTE_FBK_HASH_NAMESIZE];
> +	char *hash_name = NULL;
>  	const uint32_t mem_size =
>  			sizeof(*ht) + (sizeof(ht->t[0]) * params->entries);
>  	uint32_t i;
> @@ -105,7 +107,14 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
>  		return NULL;
>  	}
>  
> -	snprintf(hash_name, sizeof(hash_name), "FBK_%s", params->name);
> +	if (strlen(params->name) >= RTE_FBK_HASH_NAMESIZE) {
> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}
> +
> +	/* don't care if hash_name is NULL */
> +	int unused __rte_unused;
> +	unused = asprintf(&hash_name, "FBK_%s", params->name);

Does this code handle asprintf() failure correctly? If asprintf() fails
and returns -1 with hash_name set to NULL, the subsequent code using
hash_name could crash or behave incorrectly.

> @@ -170,6 +179,7 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
>  
>  exit:
>  	rte_mcfg_tailq_write_unlock();
> +	free(hash_name);

Does this code have a memory leak on the success path? The hash_name is
allocated with asprintf() but only freed at the exit label, which
appears to be used for both success and failure cases.