stats: missing dimension in stat_set_simple_counter

A simple counter is a two dimensional array by threads and
counter index. 28017 introduced an error missing the first
dimension.

If a vector is updated at the same time as a client reads,
an invalid pointer my result. This will be caught by the
optimistic locking after copying out the data, but if
following a pointer outside of the stat segment then
the stat client would crash. Add suitable boundary checks
for access to stat memory segment.

Fixes: 7d29e320fb2855a1ddb7a6af09078b8ed636de01
Type: fix
Signed-off-by: Ole Troan <ot@cisco.com>
Change-Id: I94f124ec71d98218c4eda5d124ac5594743d93d6

diff --git a/src/vpp-api/client/stat_client.c b/src/vpp-api/client/stat_client.c
index 56ff387..018cce3 100644 (file)
--- a/src/vpp-api/client/stat_client.c
+++ b/src/vpp-api/client/stat_client.c
@@ -192,6 +192,16 @@ stat_segment_heartbeat (void)
   return stat_segment_heartbeat_r (sm);
 }
 
+#define stat_vec_dup(S,V)                             \
+  ({                                                  \
+  __typeof__ ((V)[0]) * _v(v) = 0;                    \
+  if (V && ((void *)V > (void *)S->shared_header) &&  \
+      (((void*)V + vec_bytes(V)) <                    \
+       ((void *)S->shared_header + S->memory_size)))  \
+    _v(v) = vec_dup(V);                               \
+   _v(v);                                             \
+})
+
 static stat_segment_data_t
 copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
 {
@@ -213,21 +223,21 @@ copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
 
     case STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE:
       simple_c = stat_segment_adjust (sm, ep->data);
-      result.simple_counter_vec = vec_dup (simple_c);
+      result.simple_counter_vec = stat_vec_dup (sm, simple_c);
       for (i = 0; i < vec_len (simple_c); i++)
        {
          counter_t *cb = stat_segment_adjust (sm, simple_c[i]);
-         result.simple_counter_vec[i] = vec_dup (cb);
+         result.simple_counter_vec[i] = stat_vec_dup (sm, cb);
        }
       break;
 
     case STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED:
       combined_c = stat_segment_adjust (sm, ep->data);
-      result.combined_counter_vec = vec_dup (combined_c);
+      result.combined_counter_vec = stat_vec_dup (sm, combined_c);
       for (i = 0; i < vec_len (combined_c); i++)
        {
          vlib_counter_t *cb = stat_segment_adjust (sm, combined_c[i]);
-         result.combined_counter_vec[i] = vec_dup (cb);
+         result.combined_counter_vec[i] = stat_vec_dup (sm, cb);
        }
       break;
 
@@ -246,11 +256,11 @@ copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
     case STAT_DIR_TYPE_NAME_VECTOR:
       {
        uint8_t **name_vector = stat_segment_adjust (sm, ep->data);
-       result.name_vector = vec_dup (name_vector);
+       result.name_vector = stat_vec_dup (sm, name_vector);
        for (i = 0; i < vec_len (name_vector); i++)
          {
            u8 *name = stat_segment_adjust (sm, name_vector[i]);
-           result.name_vector[i] = vec_dup (name);
+           result.name_vector[i] = stat_vec_dup (sm, name);
          }
       }
       break;
@@ -290,6 +300,8 @@ stat_segment_data_free (stat_segment_data_t * res)
        case STAT_DIR_TYPE_ERROR_INDEX:
          vec_free (res[i].error_vector);
          break;
+       case STAT_DIR_TYPE_SCALAR_INDEX:
+         break;
        default:
          assert (0);
        }

diff --git a/src/vpp-api/client/stat_client.h b/src/vpp-api/client/stat_client.h
index c5fa559..f8473ef 100644 (file)
--- a/src/vpp-api/client/stat_client.h
+++ b/src/vpp-api/client/stat_client.h
@@ -101,8 +101,12 @@ _time_now_nsec (void)
 static inline void *
 stat_segment_adjust (stat_client_main_t * sm, void *data)
 {
-  return (void *) ((char *) sm->shared_header +
-                  ((char *) data - (char *) sm->shared_header->base));
+  void *p = (void *) ((char *) sm->shared_header +
+                     ((char *) data - (char *) sm->shared_header->base));
+  if (p > (void *) sm->shared_header &&
+      ((p + sizeof (p)) < ((void *) sm->shared_header + sm->memory_size)))
+    return p;
+  return 0;
 }
 
 /*

diff --git a/src/vpp/stats/stat_segment.c b/src/vpp/stats/stat_segment.c
index b060969..4244acb 100644 (file)
--- a/src/vpp/stats/stat_segment.c
+++ b/src/vpp/stats/stat_segment.c
@@ -281,7 +281,8 @@ stat_set_simple_counter (stat_segment_directory_entry_t * ep,
                         u32 thread_index, u32 index, u64 value)
 {
   ASSERT (ep->data);
-  counter_t *cb = ep->data;
+  counter_t **counters = ep->data;
+  counter_t *cb = counters[thread_index];
   cb[index] = value;
 }