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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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 <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().

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