|SUCCESS| pw159213 [PATCH v5 02/17] lpm: restrict name size

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

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

_AI Review Comments Available_

Looking at this patch, I have a few concerns about the implementation
and approach.

commit: (not provided in patch headers)
Author: Stephen Hemminger <stephen@networkplumber.org>
lpm: restrict name size

> Subject: [PATCH v5 02/17] lpm: restrict name size
> 
> Since LPM uses ring internally with a prefix, limit the
> maximum allowed LPM name.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/lpm/rte_lpm.c | 25 +++++++++++++++++++------
>  lib/lpm/rte_lpm.h |  2 +-
>  2 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/lpm/rte_lpm.c b/lib/lpm/rte_lpm.c
> index 6dab86a05e..2666d4f208 100644
> --- a/lib/lpm/rte_lpm.c
> +++ b/lib/lpm/rte_lpm.c
> @@ -5,6 +5,7 @@
>  
>  #include <string.h>
>  #include <stdint.h>
> +#include <stdlib.h>
>  #include <errno.h>
>  #include <stdio.h>
>  #include <sys/queue.h>

[ ... ]

> @@ -152,7 +153,6 @@ struct rte_lpm *
>  rte_lpm_create(const char *name, int socket_id,
>  		const struct rte_lpm_config *config)
>  {
> -	char mem_name[RTE_LPM_NAMESIZE];
>  	struct __rte_lpm *i_lpm;
>  	struct rte_lpm *lpm = NULL;
>  	struct rte_tailq_entry *te;
> @@ -170,7 +170,15 @@ rte_lpm_create(const char *name, int socket_id,
>  		return NULL;
>  	}
>  
> -	snprintf(mem_name, sizeof(mem_name), "LPM_%s", name);
> +	if (strlen(name) >= RTE_LPM_NAMESIZE) {
> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}
> +
> +	/* This is only used for tracing, ok to be NULL */
> +	char *mem_name = NULL;
> +	int unused __rte_unused;
> +	unused = asprintf(&mem_name, "LPM_%s", name);

Does this code leak mem_name when asprintf fails? The asprintf function
allocates memory that needs to be freed, but if it fails and returns -1,
mem_name becomes undefined and might not be NULL.

Would it be more robust to check the return value and set mem_name to
NULL on failure rather than using the __rte_unused annotation?

> 
>  	rte_mcfg_tailq_write_lock();
> 

[ ... ]

> @@ -247,6 +255,7 @@ rte_lpm_create(const char *name, int socket_id,
>  
>  exit:
>  	rte_mcfg_tailq_write_unlock();
> +	free(mem_name);

Can this code safely call free on mem_name if asprintf failed? When
asprintf returns -1, the state of mem_name is undefined according to
POSIX - it might not be safe to pass to free().

> 
>  	return lpm;
>  }

[ ... ]

> @@ -326,9 +334,13 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
>  	if (cfg->mode == RTE_LPM_QSBR_MODE_SYNC) {
>  		/* No other things to do. */
>  	} else if (cfg->mode == RTE_LPM_QSBR_MODE_DQ) {
> +		char rcu_dq_name[RTE_RCU_QSBR_DQ_NAMESIZE];
> +
>  		/* Init QSBR defer queue. */
> -		snprintf(rcu_dq_name, sizeof(rcu_dq_name),
> -				"LPM_RCU_%s", i_lpm->name);
> +		if (snprintf(rcu_dq_name, sizeof(rcu_dq_name),
> +			     "LPM_RCU_%s", i_lpm->name) >= (int)sizeof(rcu_dq_name))
> +			LPM_LOG(NOTICE, "LPM rcu defer queue name truncated");

Should this function return an error when the RCU defer queue name gets
truncated? Operating with a truncated name might cause collisions or
other unexpected behavior.

> diff --git a/lib/lpm/rte_lpm.h b/lib/lpm/rte_lpm.h
> index edfe77b458..8e70603ace 100644
> --- a/lib/lpm/rte_lpm.h
> +++ b/lib/lpm/rte_lpm.h
> @@ -26,7 +26,7 @@ extern "C" {
>  #endif
>  
>  /** Max number of characters in LPM name. */
> -#define RTE_LPM_NAMESIZE                32
> +#define RTE_LPM_NAMESIZE  RTE_RING_NAMESIZE

Is it correct to assume RTE_RING_NAMESIZE is always available when this
header is included? This creates a dependency that might not be obvious
to users of the LPM library.