lib: graph: fix vector_remove()

vector_remove would corrupt the data in the following sequence:
1. assume vector v = [a, b], active = 2
2. vector_unset(v, 0) => v = [NULL, b], active = 2
3. vector_remove(v, 1)

vector_remove calls vector_unset(v, 1), vector_unset notices index #0 is
also NULL and thus sets active to 0.

The equality test in vector_remove() now fails, leading it to decrement
v->active *again*, leading to an underflow that will likely crash the
daemon (and might even be exploitable).

This call sequence does not happen in existing code since vector_unset()
is not used on graph from/to lists.  Nonetheless this is a buried land
mine in the code at best.

Rewrite the function - while we're at it, there's no reason to move the
entire array around, just fill the hole with the last element.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Cc: Quentin Young <qlyoung@cumulusnetworks.com>

diff --git a/lib/graph.c b/lib/graph.c
index 9ca0a3152c34892a6fb2a943a89b098d67b0b6fa..0992059ef1d340954d438511fcec0dd723f2b86a 100644 (file)
--- a/lib/graph.c
+++ b/lib/graph.c
@@ -55,11 +55,16 @@ graph_new_node (struct graph *graph, void *data, void (*del) (void*))
 static void
 vector_remove (vector v, unsigned int ix)
 {
-  vector_unset (v, ix);
-  if (ix == vector_active (v)) return;
-  for (; ix < vector_active (v) - 1; ix++)
-    v->index[ix] = v->index[ix+1];
+  if (ix >= v->active)
+    return;
+
+  /* v->active is guaranteed >= 1 because ix can't be lower than 0
+   * and v->active is > ix. */
   v->active--;
+  /* if ix == v->active--, we set the item to itself, then to NULL...
+   * still correct, no check neccessary. */
+  v->index[ix] = v->index[v->active];
+  v->index[v->active] = NULL;
 }
 
 void