Re: [dpdk-dev] [PATCH v2 2/2] eal: fix signed integers in fbarray

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 13-Apr-18 4:56 PM, Adrien Mazarguil wrote:
> While debugging startup issues encountered with Clang (see "eal: fix
> undefined behavior in fbarray"), I noticed that fbarray stores indices,
> sizes and masks on signed integers involved in bitwise operations.
> 
> Such operations almost invariably cause undefined behavior with values that
> cannot be represented by the result type, as is often the case with
> bit-masks and left-shifts.
> 
> This patch replaces them with unsigned integers as a safety measure and
> promotes a few internal variables to larger types for consistency.
> 
> Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> --
> 
> v2 changes:
> 
> Removed unnecessary "(unsigned int)" cast leftovers.

Thanks for figuring this out! In general, i'm OK with the change, however...

> ---
>   lib/librte_eal/common/eal_common_fbarray.c  | 97 ++++++++++++------------
>   lib/librte_eal/common/include/rte_fbarray.h | 33 ++++----
>   2 files changed, 68 insertions(+), 62 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
> index 11aa3f22a..368290654 100644
> --- a/lib/librte_eal/common/eal_common_fbarray.c
> +++ b/lib/librte_eal/common/eal_common_fbarray.c
> @@ -21,7 +21,7 @@
>   #include "rte_fbarray.h"
>   
>   #define MASK_SHIFT 6ULL
> -#define MASK_ALIGN (1 << MASK_SHIFT)
> +#define MASK_ALIGN (1ULL << MASK_SHIFT)
>   #define MASK_LEN_TO_IDX(x) ((x) >> MASK_SHIFT)
>   #define MASK_LEN_TO_MOD(x) ((x) - RTE_ALIGN_FLOOR(x, MASK_ALIGN))
>   #define MASK_GET_IDX(idx, mod) ((idx << MASK_SHIFT) + mod)
> @@ -32,12 +32,12 @@
>    */

<...>

>   
>   int __rte_experimental
> -rte_fbarray_find_next_free(struct rte_fbarray *arr, int start)
> +rte_fbarray_find_next_free(struct rte_fbarray *arr, unsigned int start)
>   {

This leads to inconsistency here. As it is, we can specify len and start 
value up to UINT32_MAX, but this (and others like it) function will only 
return values up to INT32_MAX.

One way to fix this would be to prohibit len being >= INT32_MAX on array 
creation. The place to fix this would probably be fully_validate().

>   	int ret = -1;
>   
> -	if (arr == NULL || start < 0 || start >= arr->len) {
> +	if (arr == NULL || start >= arr->len) {
>   		rte_errno = EINVAL;
>   		return -1;
>   	}
> @@ -683,11 +686,11 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int start)


-- 
Thanks,
Anatoly