Re: [dpdk-dev] [PATCH] bond: static analysis issues fix

["Wodkowski, PawelX" <pawelx.wodkowski@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Declan Doherty
> Sent: Friday, December 12, 2014 6:40 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] bond: static analysis issues fix
> 
> Fixes for link bonding library identified by static analysis tool
> 
> - Overflow check for active_slaves array in activate_slave function
> - Allocation check of pci_id_table in rte_eth_bond_create
> - Use of eth_dev pointer in mac_address_get/set before NULL check
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  lib/librte_pmd_bond/rte_eth_bond_api.c | 12 ++++++++----
>  lib/librte_pmd_bond/rte_eth_bond_pmd.c |  8 ++++----
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_api.c
> b/lib/librte_pmd_bond/rte_eth_bond_api.c
> index ef5ddf4..9cb1c1f 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_api.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_api.c
> @@ -115,8 +115,11 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t
> port_id)
>  	if (internals->mode == BONDING_MODE_8023AD)
>  		bond_mode_8023ad_activate_slave(eth_dev, port_id);
> 
> -	internals->active_slaves[internals->active_slave_count] = port_id;
> -	internals->active_slave_count++;
> +	if (internals->active_slave_count <
> +			RTE_DIM(internals->active_slaves) - 1) {
> +		internals->active_slaves[internals->active_slave_count] =
> port_id;
> +		internals->active_slave_count++;
> +	}
>  }
> 
>  void
> @@ -144,7 +147,8 @@ deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t
> port_id)
>  					sizeof(internals->active_slaves[0]));
>  	}
> 
> -	internals->active_slave_count = active_count;
> +	internals->active_slave_count = active_count < RTE_MAX_ETHPORTS ?
> +			active_count : RTE_MAX_ETHPORTS - 1;

Since port might not be added twice and active_slaves array is (should be)
 proper size to contain every port you can add to bonding and in fact is
one element bigger and active_slave_count should newer overflow, those
changes might only mask real problems in user application and/or library itself.
I think if you want to make this static analysis tool happy it should be changed
to RTE_VERIFY(), assert(), rte_panic() or something like that to indicate
undefined state.

Pawel