From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [PATCH v2 2/2] eal: fix signed integers in fbarray
Date: Fri, 13 Apr 2018 17:09:01 +0100
Message-ID: <25801a3b-8c0d-26fb-aa72-40bbd02adbb4@intel.com>
References: <20180413153749.28208-1-adrien.mazarguil@6wind.com>
 <20180413155417.29643-1-adrien.mazarguil@6wind.com>
 <20180413155417.29643-2-adrien.mazarguil@6wind.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: dev@dpdk.org
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by dpdk.org (Postfix) with ESMTP id 102241C686
 for <dev@dpdk.org>; Fri, 13 Apr 2018 18:09:03 +0200 (CEST)
In-Reply-To: <20180413155417.29643-2-adrien.mazarguil@6wind.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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