From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2 2/2] eal: fix signed integers in fbarray Date: Fri, 13 Apr 2018 19:54:03 +0200 Message-ID: <20180413175403.GT4957@6wind.com> References: <20180413153749.28208-1-adrien.mazarguil@6wind.com> <20180413155417.29643-1-adrien.mazarguil@6wind.com> <20180413155417.29643-2-adrien.mazarguil@6wind.com> <25801a3b-8c0d-26fb-aa72-40bbd02adbb4@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: "Burakov, Anatoly" Return-path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id E8EB31C6D5 for ; Fri, 13 Apr 2018 19:54:17 +0200 (CEST) Received: by mail-wr0-f193.google.com with SMTP id z73so10181522wrb.0 for ; Fri, 13 Apr 2018 10:54:17 -0700 (PDT) Content-Disposition: inline In-Reply-To: <25801a3b-8c0d-26fb-aa72-40bbd02adbb4@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Apr 13, 2018 at 05:09:01PM +0100, Burakov, Anatoly wrote: > 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 > > > > Signed-off-by: Adrien Mazarguil > > > > -- > > > > 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(). Indeed, also I just received a Coverity report about a bunch of other details due to these changes (now it's obvious that calc_data_size() doesn't support negative page sizes), I'll update the patch accordingly and submit v3. Thanks. > > > 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 -- Adrien Mazarguil 6WIND