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.bura...@intel.com>
> >
> > Signed-off-by: Adrien Mazarguil <adrien.mazarg...@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
