|SUCCESS| pw159226 [PATCH v5 15/17] ethdev: avoid possible overflow in xstat names

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

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

_AI Review Comments Available_

**TodoWrite Items:**

- [x] git sha of the commit: (from patch series v5 15/17)
- [x] Author: Stephen Hemminger <stephen@networkplumber.org>
- [x] One line subject: ethdev: avoid possible overflow in xstat names
- [x] Brief summary of the commit
- [x] Unified diff of the commit, quoted as email reply
- [x] Questions about regressions placed alongside relevant code
- [x] Snipped portions of quoted content unrelated to review
- [x] Check every hunk for relevance to review comments
- [x] Retain necessary diff headers
- [x] Replace snipped content with [ ... ]
- [x] Only keep quoted material needed for review to make sense

---

Author: Stephen Hemminger <stephen@networkplumber.org>
Subject: ethdev: avoid possible overflow in xstat names

This commit adds overflow checking to snprintf operations when
constructing ethernet device statistic names. It replaces silent
truncation with explicit error logging when statistic names exceed
the available buffer space.

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index c6fe0d5165..05de3335e5 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3489,9 +3489,10 @@ eth_basic_stats_get_names(struct rte_eth_dev *dev,
>  	uint16_t num_q;
>  
>  	for (idx = 0; idx < RTE_NB_STATS; idx++) {
> -		strlcpy(xstats_names[cnt_used_entries].name,
> -			eth_dev_stats_strings[idx].name,
> -			sizeof(xstats_names[0].name));
> +		if (strlcpy(xstats_names[cnt_used_entries].name, eth_dev_stats_strings[idx].name,
> +			    sizeof(xstats_names[0].name)) >= sizeof(xstats_names[0].name))
> +			RTE_ETHDEV_LOG_LINE(ERR, "statistic name '%s' will be truncated",
> +					    xstats_names[cnt_used_entries].name);

Does this error message print the correct value? After strlcpy truncates
the source string, xstats_names[cnt_used_entries].name contains the
truncated version rather than the original problematic string from
eth_dev_stats_strings[idx].name.

> @@ -3501,10 +3502,17 @@ eth_basic_stats_get_names(struct rte_eth_dev *dev,
>  	num_q = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
>  	for (id_queue = 0; id_queue < num_q; id_queue++) {
>  		for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
> -			snprintf(xstats_names[cnt_used_entries].name,
> -				sizeof(xstats_names[0].name),
> -				"rx_q%u_%s",
> -				id_queue, eth_dev_rxq_stats_strings[idx].name);
> +			unsigned int cc;
> +
> +			cc = snprintf(xstats_names[cnt_used_entries].name,
> +				      sizeof(xstats_names[0].name),
> +				      "rx_q%u_%s",
> +				      id_queue, eth_dev_rxq_stats_strings[idx].name);
> +
> +			/* could only happen if a long string was added */
> +			if (cc >= sizeof(xstats_names[0].name))
> +				RTE_ETHDEV_LOG_LINE(ERR, "truncated rxq stat string '%s'",
> +						    eth_dev_rxq_stats_strings[idx].name);

Should this error message include the queue ID to help identify which
specific queue statistic caused the truncation? The current message
only shows the base statistic name without the queue context.

> @@ -3512,10 +3520,15 @@ eth_basic_stats_get_names(struct rte_eth_dev *dev,
>  	num_q = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
>  	for (id_queue = 0; id_queue < num_q; id_queue++) {
>  		for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
> -			snprintf(xstats_names[cnt_used_entries].name,
> -				sizeof(xstats_names[0].name),
> -				"tx_q%u_%s",
> -				id_queue, eth_dev_txq_stats_strings[idx].name);
> +			unsigned int cc;
> +
> +			cc = snprintf(xstats_names[cnt_used_entries].name,
> +				      sizeof(xstats_names[0].name),
> +				      "tx_q%u_%s",
> +				      id_queue, eth_dev_txq_stats_strings[idx].name);
> +			if (cc >= sizeof(xstats_names[0].name))
> +				RTE_ETHDEV_LOG_LINE(ERR, "truncated txq stat string '%s'",
> +						    eth_dev_txq_stats_strings[idx].name);

Would it be more consistent to include the same comment about "could
only happen if a long string was added" here as in the rx queue
handling above?