Fixes a tricky bug in route filtering.

Route attributes was used after rta was freed during copy-on-write in
filter code. This causes some random crashes, esp. with multipath
routes.

2 files changed, 77 insertions, 24 deletions

diff --git a/filter/filter.c b/filter/filter.c
index b3b17dcf..d6d338bf 100644
--- a/filter/filter.c
+++ b/filter/filter.c
@@ -485,24 +485,42 @@ val_print(struct f_val v)
   }
 }
 
-static struct rte **f_rte, *f_rte_old;
-static struct linpool *f_pool;
+static struct rte **f_rte;
+static struct rta *f_old_rta;
 static struct ea_list **f_tmp_attrs;
+static struct linpool *f_pool;
 static int f_flags;
 
+static inline void f_rte_cow(void)
+{
+  *f_rte = rte_cow(*f_rte); 
+}
+
 /*
  * rta_cow - prepare rta for modification by filter
  */
 static void
-rta_cow(void)
+f_rta_cow(void)
 {
   if ((*f_rte)->attrs->aflags & RTAF_CACHED) {
-    rta *f_rta_copy = lp_alloc(f_pool, sizeof(rta));
-    memcpy(f_rta_copy, (*f_rte)->attrs, sizeof(rta));
-    f_rta_copy->aflags = 0;
-    *f_rte = rte_cow(*f_rte);
-    rta_free((*f_rte)->attrs);
-    (*f_rte)->attrs = f_rta_copy;
+
+    /* Prepare to modify rte */
+    f_rte_cow();
+
+    /* Store old rta to free it later */
+    f_old_rta = (*f_rte)->attrs;
+
+    /* 
+     * Alloc new rta, do shallow copy and update rte. Fields eattrs
+     * and nexthops of rta are shared with f_old_rta (they will be
+     * copied when the cached rta will be obtained at the end of
+     * f_run()), also the lock of hostentry is inherited (we suppose
+     * hostentry is not changed by filters).
+     */
+    rta *ra = lp_alloc(f_pool, sizeof(rta));
+    memcpy(ra, f_old_rta, sizeof(rta));
+    ra->aflags = 0;
+    (*f_rte)->attrs = ra;
   }
 }
 
@@ -819,7 +837,7 @@ interpret(struct f_inst *what)
     ONEARG;
     if (what->aux != v1.type)
       runtime( "Attempt to set static attribute to incompatible type" );
-    rta_cow();
+    f_rta_cow();
     {
       struct rta *rta = (*f_rte)->attrs;
       switch (what->aux) {
@@ -955,7 +973,7 @@ interpret(struct f_inst *what)
       }
 
       if (!(what->aux & EAF_TEMP) && (!(f_flags & FF_FORCE_TMPATTR))) {
-	rta_cow();
+	f_rta_cow();
 	l->next = (*f_rte)->attrs->eattrs;
 	(*f_rte)->attrs->eattrs = l;
       } else {
@@ -974,7 +992,7 @@ interpret(struct f_inst *what)
       runtime( "Can't set preference to non-integer" );
     if ((v1.val.i < 0) || (v1.val.i > 0xFFFF))
       runtime( "Setting preference value out of bounds" );
-    *f_rte = rte_cow(*f_rte);
+    f_rte_cow();
     (*f_rte)->pref = v1.val.i;
     break;
   case 'L':	/* Get length of */
@@ -1300,29 +1318,64 @@ i_same(struct f_inst *f1, struct f_inst *f2)
 }
 
 /**
- * f_run - external entry point to filters
- * @filter: pointer to filter to run
- * @tmp_attrs: where to store newly generated temporary attributes
- * @rte: pointer to pointer to &rte being filtered. When route is modified, this is changed with rte_cow().
+ * f_run - run a filter for a route
+ * @filter: filter to run
+ * @rte: route being filtered, may be modified
+ * @tmp_attrs: temporary attributes, prepared by caller or generated by f_run()
  * @tmp_pool: all filter allocations go from this pool
  * @flags: flags
+ *
+ * If filter needs to modify the route, there are several
+ * posibilities. @rte might be read-only (with REF_COW flag), in that
+ * case rw copy is obtained by rte_cow() and @rte is replaced. If
+ * @rte is originally rw, it may be directly modified (and it is never
+ * copied).
+ *
+ * The returned rte may reuse the (possibly cached, cloned) rta, or
+ * (if rta was modificied) contains a modified uncached rta, which
+ * uses parts allocated from @tmp_pool and parts shared from original
+ * rta. There is one exception - if @rte is rw but contains a cached
+ * rta and that is modified, rta in returned rte is also cached.
+ *
+ * Ownership of cached rtas is consistent with rte, i.e.
+ * if a new rte is returned, it has its own clone of cached rta
+ * (and cached rta of read-only source rte is intact), if rte is
+ * modified in place, old cached rta is possibly freed.
  */
 int
 f_run(struct filter *filter, struct rte **rte, struct ea_list **tmp_attrs, struct linpool *tmp_pool, int flags)
 {
-  struct f_inst *inst;
-  struct f_val res;
+  int rte_cow = ((*rte)->flags & REF_COW);
   DBG( "Running filter `%s'...", filter->name );
 
-  f_flags = flags;
-  f_tmp_attrs = tmp_attrs;
   f_rte = rte;
-  f_rte_old = *rte;
+  f_old_rta = NULL;
+  f_tmp_attrs = tmp_attrs;
   f_pool = tmp_pool;
-  inst = filter->root;
+  f_flags = flags;
 
   log_reset();
-  res = interpret(inst);
+  struct f_val res = interpret(filter->root);
+
+  if (f_old_rta) {
+    /*
+     * Cached rta was modified and f_rte contains now an uncached one,
+     * sharing some part with the cached one. The cached rta should
+     * be freed (if rte was originally COW, f_old_rta is a clone
+     * obtained during rte_cow()).
+     *
+     * This also implements the exception mentioned in f_run()
+     * description. The reason for this is that rta reuses parts of
+     * f_old_rta, and these may be freed during rta_free(f_old_rta).
+     * This is not the problem if rte was COW, because original rte
+     * also holds the same rta.
+     */
+    if (!rte_cow)
+      (*f_rte)->attrs = rta_lookup((*f_rte)->attrs);
+
+    rta_free(f_old_rta);
+  }
+
 
   if (res.type != T_RETURN) {
     log( L_ERR "Filter %s did not return accept nor reject. Make up your mind", filter->name); 
@@ -1341,7 +1394,6 @@ f_eval_int(struct f_inst *expr)
   f_flags = 0;
   f_tmp_attrs = NULL;
   f_rte = NULL;
-  f_rte_old = NULL;
   f_pool = cfg_mem;
 
   log_reset();
diff --git a/lib/slab.c b/lib/slab.c
index af6b50b0..e236e26e 100644
--- a/lib/slab.c
+++ b/lib/slab.c
@@ -60,6 +60,7 @@ static struct resclass sl_class = {
   sizeof(struct slab),
   slab_free,
   slab_dump,
+  NULL,
   slab_memsize
 };