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