From c30420200bf9e0a8c076da5348a0c538de87640c Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 26 Jan 2017 05:59:50 +0100 Subject: [PATCH] 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 Cc: Quentin Young --- lib/graph.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/graph.c b/lib/graph.c index 9ca0a3152c..0992059ef1 100644 --- 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 -- 2.39.5