vppinfra: fix memcpy undefined behaviour

Calling mem{cpy,move} with NULL pointers results in undefined behaviour.
This in turns is exploited by GCC. For example, the sequence:
    memcpy (dst, src, n);
    if (!src)
      return;
    src[0] = 0xcafe;
will be optimized as
    memcpy (dst, src, n);
    src[0] = 0xcafe;
IOW the test for NULL is gone.

vec_*() functions sometime call memcpy with NULL pointers and 0 length,
triggering this optimization. For example, the sequence:
    vec_append(v1, v2);
    len = vec_len(v2);
will crash if v2 is NULL, because the test for NULL pointer in vec_len()
has been optimized out.

This commit fixes occurrences of such undefined behaviour, and also
introduces a memcpy wrapper to catch those in debug mode.

Type: fix

Change-Id: I175e2dd726a883f97cf7de3b15f66d4b237ddefd
Signed-off-by: Benoît Ganne <bganne@cisco.com>

diff --git a/src/vppinfra/hash.c b/src/vppinfra/hash.c
index 220c169..da37b6e 100644 (file)
--- a/src/vppinfra/hash.c
+++ b/src/vppinfra/hash.c
@@ -548,6 +548,7 @@ lookup (void *v, uword key, enum lookup_opcode op,
   hash_t *h = hash_header (v);
   hash_pair_union_t *p = 0;
   uword found_key = 0;
+  uword value_bytes;
   uword i;
 
   if (!v)
@@ -555,6 +556,7 @@ lookup (void *v, uword key, enum lookup_opcode op,
 
   i = key_sum (h, key) & (_vec_len (v) - 1);
   p = get_pair (v, i);
+  value_bytes = hash_value_bytes (h);
 
   if (hash_is_user (v, i))
     {
@@ -564,9 +566,8 @@ lookup (void *v, uword key, enum lookup_opcode op,
          if (op == UNSET)
            {
              set_is_user (v, i, 0);
-             if (old_value)
-               clib_memcpy_fast (old_value, p->direct.value,
-                                 hash_value_bytes (h));
+             if (old_value && value_bytes)
+               clib_memcpy_fast (old_value, p->direct.value, value_bytes);
              zero_pair (h, &p->direct);
            }
        }
@@ -598,9 +599,8 @@ lookup (void *v, uword key, enum lookup_opcode op,
          found_key = p != 0;
          if (found_key && op == UNSET)
            {
-             if (old_value)
-               clib_memcpy_fast (old_value, &p->direct.value,
-                                 hash_value_bytes (h));
+             if (old_value && value_bytes)
+               clib_memcpy_fast (old_value, &p->direct.value, value_bytes);
 
              unset_indirect (v, i, &p->direct);
 
@@ -611,12 +611,12 @@ lookup (void *v, uword key, enum lookup_opcode op,
        }
     }
 
-  if (op == SET && p != 0)
+  if (op == SET && p != 0 && value_bytes)
     {
       /* Save away old value for caller. */
       if (old_value && found_key)
-       clib_memcpy_fast (old_value, &p->direct.value, hash_value_bytes (h));
-      clib_memcpy_fast (&p->direct.value, new_value, hash_value_bytes (h));
+       clib_memcpy_fast (old_value, &p->direct.value, value_bytes);
+      clib_memcpy_fast (&p->direct.value, new_value, value_bytes);
     }
 
   if (op == SET)

diff --git a/src/vppinfra/memcpy_avx2.h b/src/vppinfra/memcpy_avx2.h
index f3f0d3f..f7a36f0 100644 (file)
--- a/src/vppinfra/memcpy_avx2.h
+++ b/src/vppinfra/memcpy_avx2.h
@@ -114,7 +114,7 @@ clib_mov128blocks (u8 * dst, const u8 * src, size_t n)
 }
 
 static inline void *
-clib_memcpy_fast (void *dst, const void *src, size_t n)
+clib_memcpy_fast_avx2 (void *dst, const void *src, size_t n)
 {
   uword dstu = (uword) dst;
   uword srcu = (uword) src;

diff --git a/src/vppinfra/memcpy_avx512.h b/src/vppinfra/memcpy_avx512.h
index 1444c27..98dac75 100644 (file)
--- a/src/vppinfra/memcpy_avx512.h
+++ b/src/vppinfra/memcpy_avx512.h
@@ -144,7 +144,7 @@ clib_mov512blocks (u8 * dst, const u8 * src, size_t n)
 }
 
 static inline void *
-clib_memcpy_fast (void *dst, const void *src, size_t n)
+clib_memcpy_fast_avx512 (void *dst, const void *src, size_t n)
 {
   uword dstu = (uword) dst;
   uword srcu = (uword) src;

diff --git a/src/vppinfra/memcpy_sse3.h b/src/vppinfra/memcpy_sse3.h
index d9e4ac6..aea2005 100644 (file)
--- a/src/vppinfra/memcpy_sse3.h
+++ b/src/vppinfra/memcpy_sse3.h
@@ -188,7 +188,7 @@ clib_mov256 (u8 * dst, const u8 * src)
 })
 
 static inline void *
-clib_memcpy_fast (void *dst, const void *src, size_t n)
+clib_memcpy_fast_sse3 (void *dst, const void *src, size_t n)
 {
   __m128i xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7, xmm8;
   uword dstu = (uword) dst;

diff --git a/src/vppinfra/string.h b/src/vppinfra/string.h
index 4f96450..fb46a0c 100644 (file)
--- a/src/vppinfra/string.h
+++ b/src/vppinfra/string.h
@@ -46,6 +46,7 @@
 
 #include <vppinfra/clib.h>     /* for CLIB_LINUX_KERNEL */
 #include <vppinfra/vector.h>
+#include <vppinfra/error_bootstrap.h>
 
 #ifdef CLIB_LINUX_KERNEL
 #include <linux/string.h>
@@ -73,16 +74,30 @@ void clib_memswap (void *_a, void *_b, uword bytes);
 #ifndef __COVERITY__
 #if __AVX512BITALG__
 #include <vppinfra/memcpy_avx512.h>
+#define clib_memcpy_fast_arch(a, b, c) clib_memcpy_fast_avx512 (a, b, c)
 #elif __AVX2__
 #include <vppinfra/memcpy_avx2.h>
+#define clib_memcpy_fast_arch(a, b, c) clib_memcpy_fast_avx2 (a, b, c)
 #elif __SSSE3__
 #include <vppinfra/memcpy_sse3.h>
-#else
-#define clib_memcpy_fast(a,b,c) memcpy(a,b,c)
-#endif
-#else /* __COVERITY__ */
-#define clib_memcpy_fast(a,b,c) memcpy(a,b,c)
-#endif
+#define clib_memcpy_fast_arch(a, b, c) clib_memcpy_fast_sse3 (a, b, c)
+#endif /* __AVX512BITALG__ */
+#endif /* __COVERITY__ */
+
+#ifndef clib_memcpy_fast_arch
+#define clib_memcpy_fast_arch(a, b, c) memcpy (a, b, c)
+#endif /* clib_memcpy_fast_arch */
+
+static_always_inline void *
+clib_memcpy_fast (void *restrict dst, const void *restrict src, size_t n)
+{
+  ASSERT (dst && src &&
+         "memcpy(src, dst, n) with src == NULL or dst == NULL is undefined "
+         "behaviour");
+  return clib_memcpy_fast_arch (dst, src, n);
+}
+
+#undef clib_memcpy_fast_arch
 
 /* c-11 string manipulation variants */
 

diff --git a/src/vppinfra/vec.h b/src/vppinfra/vec.h
index e8146af..d19ff99 100644 (file)
--- a/src/vppinfra/vec.h
+++ b/src/vppinfra/vec.h
@@ -665,13 +665,19 @@ do {                                                                              \
     @param A alignment (may be zero)
     @return V (value-result macro parameter)
 */
-#define vec_add_ha(V,E,N,H,A)                                                  \
-do {                                                                           \
-  word _v(n) = (N);                                                            \
-  word _v(l) = vec_len (V);                                                    \
-  V = _vec_resize ((V), _v(n), (_v(l) + _v(n)) * sizeof ((V)[0]), (H), (A));   \
-  clib_memcpy_fast ((V) + _v(l), (E), _v(n) * sizeof ((V)[0]));                        \
-} while (0)
+#define vec_add_ha(V, E, N, H, A)                                             \
+  do                                                                          \
+    {                                                                         \
+      word _v (n) = (N);                                                      \
+      if (PREDICT_TRUE (_v (n) > 0))                                          \
+       {                                                                     \
+         word _v (l) = vec_len (V);                                          \
+         V = _vec_resize ((V), _v (n), (_v (l) + _v (n)) * sizeof ((V)[0]),  \
+                          (H), (A));                                         \
+         clib_memcpy_fast ((V) + _v (l), (E), _v (n) * sizeof ((V)[0]));     \
+       }                                                                     \
+    }                                                                         \
+  while (0)
 
 /** \brief Add N elements to end of vector V (no header, unspecified alignment)
 
@@ -819,22 +825,23 @@ do {                                                      \
     @return V (value-result macro parameter)
 */
 
-#define vec_insert_elts_ha(V,E,N,M,H,A)                        \
-do {                                                   \
-  word _v(l) = vec_len (V);                            \
-  word _v(n) = (N);                                    \
-  word _v(m) = (M);                                    \
-  V = _vec_resize ((V),                                        \
-                  _v(n),                               \
-                  (_v(l) + _v(n))*sizeof((V)[0]),      \
-                  (H), (A));                           \
-  ASSERT (_v(m) <= _v(l));                             \
-  memmove ((V) + _v(m) + _v(n),                                \
-          (V) + _v(m),                                 \
-          (_v(l) - _v(m)) * sizeof ((V)[0]));          \
-  clib_memcpy_fast ((V) + _v(m), (E),                  \
-              _v(n) * sizeof ((V)[0]));                \
-} while (0)
+#define vec_insert_elts_ha(V, E, N, M, H, A)                                  \
+  do                                                                          \
+    {                                                                         \
+      word _v (n) = (N);                                                      \
+      if (PREDICT_TRUE (_v (n) > 0))                                          \
+       {                                                                     \
+         word _v (l) = vec_len (V);                                          \
+         word _v (m) = (M);                                                  \
+         V = _vec_resize ((V), _v (n), (_v (l) + _v (n)) * sizeof ((V)[0]),  \
+                          (H), (A));                                         \
+         ASSERT (_v (m) <= _v (l));                                          \
+         memmove ((V) + _v (m) + _v (n), (V) + _v (m),                       \
+                  (_v (l) - _v (m)) * sizeof ((V)[0]));                      \
+         clib_memcpy_fast ((V) + _v (m), (E), _v (n) * sizeof ((V)[0]));     \
+       }                                                                     \
+    }                                                                         \
+  while (0)
 
 /** \brief Insert N vector elements starting at element M,
     insert given elements (no header, unspecified alignment)
@@ -902,15 +909,21 @@ do {                                              \
     @param V2 vector to append
 */
 
-#define vec_append(v1,v2)                                              \
-do {                                                                   \
-  uword _v(l1) = vec_len (v1);                                         \
-  uword _v(l2) = vec_len (v2);                                         \
-                                                                       \
-  v1 = _vec_resize ((v1), _v(l2),                                      \
-                   (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, 0);        \
-  clib_memcpy_fast ((v1) + _v(l1), (v2), _v(l2) * sizeof ((v2)[0]));           \
-} while (0)
+#define vec_append(v1, v2)                                                    \
+  do                                                                          \
+    {                                                                         \
+      uword _v (l1) = vec_len (v1);                                           \
+      uword _v (l2) = vec_len (v2);                                           \
+                                                                              \
+      if (PREDICT_TRUE (_v (l2) > 0))                                         \
+       {                                                                     \
+         v1 = _vec_resize ((v1), _v (l2),                                    \
+                           (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, 0);    \
+         clib_memcpy_fast ((v1) + _v (l1), (v2),                             \
+                           _v (l2) * sizeof ((v2)[0]));                      \
+       }                                                                     \
+    }                                                                         \
+  while (0)
 
 /** \brief Append v2 after v1. Result in v1. Specified alignment.
     @param V1 target vector
@@ -918,31 +931,42 @@ do {                                                                      \
     @param align required alignment
 */
 
-#define vec_append_aligned(v1,v2,align)                                        \
-do {                                                                   \
-  uword _v(l1) = vec_len (v1);                                         \
-  uword _v(l2) = vec_len (v2);                                         \
-                                                                       \
-  v1 = _vec_resize ((v1), _v(l2),                                      \
-                   (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, align);    \
-  clib_memcpy_fast ((v1) + _v(l1), (v2), _v(l2) * sizeof ((v2)[0]));           \
-} while (0)
+#define vec_append_aligned(v1, v2, align)                                     \
+  do                                                                          \
+    {                                                                         \
+      uword _v (l1) = vec_len (v1);                                           \
+      uword _v (l2) = vec_len (v2);                                           \
+                                                                              \
+      if (PREDICT_TRUE (_v (l2) > 0))                                         \
+       {                                                                     \
+         v1 = _vec_resize (                                                  \
+           (v1), _v (l2), (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, align); \
+         clib_memcpy_fast ((v1) + _v (l1), (v2),                             \
+                           _v (l2) * sizeof ((v2)[0]));                      \
+       }                                                                     \
+    }                                                                         \
+  while (0)
 
 /** \brief Prepend v2 before v1. Result in v1.
     @param V1 target vector
     @param V2 vector to prepend
 */
 
-#define vec_prepend(v1,v2)                                              \
-do {                                                                    \
-  uword _v(l1) = vec_len (v1);                                          \
-  uword _v(l2) = vec_len (v2);                                          \
-                                                                        \
-  v1 = _vec_resize ((v1), _v(l2),                                       \
-                   (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, 0);        \
-  memmove ((v1) + _v(l2), (v1), _v(l1) * sizeof ((v1)[0]));             \
-  clib_memcpy_fast ((v1), (v2), _v(l2) * sizeof ((v2)[0]));                  \
-} while (0)
+#define vec_prepend(v1, v2)                                                   \
+  do                                                                          \
+    {                                                                         \
+      uword _v (l1) = vec_len (v1);                                           \
+      uword _v (l2) = vec_len (v2);                                           \
+                                                                              \
+      if (PREDICT_TRUE (_v (l2) > 0))                                         \
+       {                                                                     \
+         v1 = _vec_resize ((v1), _v (l2),                                    \
+                           (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, 0);    \
+         memmove ((v1) + _v (l2), (v1), _v (l1) * sizeof ((v1)[0]));         \
+         clib_memcpy_fast ((v1), (v2), _v (l2) * sizeof ((v2)[0]));          \
+       }                                                                     \
+    }                                                                         \
+  while (0)
 
 /** \brief Prepend v2 before v1. Result in v1. Specified alignment
     @param V1 target vector
@@ -950,17 +974,21 @@ do {                                                                    \
     @param align required alignment
 */
 
-#define vec_prepend_aligned(v1,v2,align)                                \
-do {                                                                    \
-  uword _v(l1) = vec_len (v1);                                          \
-  uword _v(l2) = vec_len (v2);                                          \
-                                                                        \
-  v1 = _vec_resize ((v1), _v(l2),                                       \
-                   (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, align);    \
-  memmove ((v1) + _v(l2), (v1), _v(l1) * sizeof ((v1)[0]));             \
-  clib_memcpy_fast ((v1), (v2), _v(l2) * sizeof ((v2)[0]));                  \
-} while (0)
-
+#define vec_prepend_aligned(v1, v2, align)                                    \
+  do                                                                          \
+    {                                                                         \
+      uword _v (l1) = vec_len (v1);                                           \
+      uword _v (l2) = vec_len (v2);                                           \
+                                                                              \
+      if (PREDICT_TRUE (_v (l2) > 0))                                         \
+       {                                                                     \
+         v1 = _vec_resize (                                                  \
+           (v1), _v (l2), (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, align); \
+         memmove ((v1) + _v (l2), (v1), _v (l1) * sizeof ((v1)[0]));         \
+         clib_memcpy_fast ((v1), (v2), _v (l2) * sizeof ((v2)[0]));          \
+       }                                                                     \
+    }                                                                         \
+  while (0)
 
 /** \brief Zero all vector elements. Null-pointer tolerant.
     @param var Vector to zero