[dpdk-dev] [PATCH v2 2/2] eal: fix signed integers in fbarray
Burakov, Anatoly
anatoly.burakov at intel.com
Fri Apr 13 18:09:01 CEST 2018
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 at intel.com>
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil at 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
More information about the dev
mailing list