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>

2 files changed, 19 insertions, 11 deletions

diff --git a/lib/event.c b/lib/event.c
index d95b3021a7..6081ba4727 100644
--- 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 44776b29a7..c35b39a147 100644
--- 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;
 };