lib: fix use after free in `clear event cpu`

Freeing any item here means freeing someone's `event->hist`, leaving a
dangling pointer there.  Which will immediately be written to because
we're executing in a CLI function under the `vty_read` event, whose
`event->hist` is then updated.

Deallocating `event->hist` anywhere other than shutting down the whole
event loop is a bad idea to begin with, just zero out the stats instead.

Fixes: FRRouting/frr#16419
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

diff --git a/lib/event.c b/lib/event.c
index d95b3021a7688ab07c7581ac049b1fba4c7c8734..6081ba47271d06f8a854aebeafacdb419ae4aac2 100644 (file)
--- a/lib/event.c
+++ b/lib/event.c
@@ -111,6 +111,11 @@ static struct cpu_event_history *cpu_records_get(struct event_loop *loop,
        return res;
 }
 
+static void cpu_records_clear(struct cpu_event_history *p)
+{
+       memset(p->_clear_begin, 0, p->_clear_end - p->_clear_begin);
+}
+
 static void cpu_records_free(struct cpu_event_history **p)
 {
        XFREE(MTYPE_EVENT_STATS, *p);
@@ -250,20 +255,15 @@ static void cpu_record_clear(uint8_t filter)
                for (ALL_LIST_ELEMENTS_RO(masters, ln, m)) {
                        frr_with_mutex (&m->mtx) {
                                struct cpu_event_history *item;
-                               struct cpu_records_head old[1];
 
-                               cpu_records_init(old);
-                               cpu_records_swap_all(old, m->cpu_records);
-
-                               while ((item = cpu_records_pop(old))) {
+                               /* it isn't possible to free the memory here
+                                * because some of these will be in use (e.g.
+                                * the one we're currently running in!)
+                                */
+                               frr_each (cpu_records, m->cpu_records, item) {
                                        if (item->types & filter)
-                                               cpu_records_free(&item);
-                                       else
-                                               cpu_records_add(m->cpu_records,
-                                                               item);
+                                               cpu_records_clear(item);
                                }
-
-                               cpu_records_fini(old);
                        }
                }
        }

diff --git a/lib/frrevent.h b/lib/frrevent.h
index 44776b29a7e189ee6e67a73ecca8ea3306d1da4f..c35b39a1473d314dd1af25eb52db25979e01a14f 100644 (file)
--- a/lib/frrevent.h
+++ b/lib/frrevent.h
@@ -139,6 +139,10 @@ struct cpu_event_history {
        struct cpu_records_item item;
 
        void (*func)(struct event *e);
+
+       /* fields between the pair of these two are nulled on "clear event cpu" */
+       char _clear_begin[0];
+
        atomic_size_t total_cpu_warn;
        atomic_size_t total_wall_warn;
        atomic_size_t total_starv_warn;
@@ -149,6 +153,10 @@ struct cpu_event_history {
        } real;
        struct time_stats cpu;
        atomic_uint_fast32_t types;
+
+       /* end of cleared region */
+       char _clear_end[0];
+
        const char *funcname;
 };