From 3702304359d3c3882db434d3f6f98ae2d84b6a75 Mon Sep 17 00:00:00 2001 From: Henrique de Moraes Holschuh Date: Tue, 5 Jan 2021 11:06:12 -0300 Subject: [PATCH 1/3] lmapd: protect against event_free(NULL) Internal errors (bugs) could cause lmapd to call event_free() on a NULL pointer. event_free() is *not* documented to handle NULL pointers, unlike free(), and in fact it does not handle them well at all (at least on libevent2 2.1). That could cause a NULL pointer derreference, and a crash. It is better to cope with it: log the problem, and keep the daemon alive. This issue has been triggered in production, it can happen. CEPTRO.br-issue: MDC-1016 --- src/runner.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/runner.c b/src/runner.c index f19338e..aafb783 100644 --- a/src/runner.c +++ b/src/runner.c @@ -52,6 +52,15 @@ event_gaga(struct event *event, struct event **ev, } #endif +static void +safe_event_free(struct event * event) +{ + if (event) + event_free(event); + else + lmap_wrn("internal error: tried to free an event twice"); +} + /** * @brief Generate a uniformly distributed random number. * @@ -787,7 +796,7 @@ fire_cb(evutil_socket_t fd, short events, void *context) suppress_cb(event->lmapd, event); execute_cb(event->lmapd, event); - event_free(event->fire_event); + safe_event_free(event->fire_event); event->fire_event = NULL; } @@ -808,7 +817,7 @@ trigger_periodic_cb(evutil_socket_t fd, short events, void *context) if (t.tv_sec > event->end) { /* XXX disable related schedules / suppressions */ lmap_wrn("event '%s' ending", event->name); - event_free(event->trigger_event); + safe_event_free(event->trigger_event); event->trigger_event = NULL; return; } @@ -836,7 +845,7 @@ trigger_calendar_cb(evutil_socket_t fd, short events, void *context) if (t.tv_sec > event->end) { /* XXX disable related schedules / suppressions */ lmap_wrn("event '%s' ending", event->name); - event_free(event->trigger_event); + safe_event_free(event->trigger_event); event->trigger_event = NULL; return; } @@ -845,7 +854,7 @@ trigger_calendar_cb(evutil_socket_t fd, short events, void *context) match = lmap_event_calendar_match(event, &t.tv_sec); if (match < 0) { lmap_err("shutting down '%s'", event->name); - event_free(event->trigger_event); + safe_event_free(event->trigger_event); event->trigger_event = NULL; return; } @@ -885,7 +894,7 @@ startup_cb(evutil_socket_t fd, short events, void *context) break; } - event_free(event->start_event); + safe_event_free(event->start_event); event->start_event = NULL; } From a5677c77664553a224040611f861ea0188aec323 Mon Sep 17 00:00:00 2001 From: Henrique de Moraes Holschuh Date: Mon, 4 Jan 2021 18:01:15 -0300 Subject: [PATCH 2/3] lmapd: do not abort due to too large a random spread Should the random spread mechanism cause two firings of an event to accumulate, we'd call event_gaga() on an event that already has event->fire_event set and active. This either causes an assertion failure on event_gaga() when trying to add the second event while the first is still active, or if assertions are disabled (production build), it overwrites event->fire_event, forgetting about the already active event. This causes two "fire" events to be active, something the current code is not prepared to support. The obvious issue is that fire_cb() will run twice for the same (lmapd) struct event, which would crash before the change of that codepath to safe_event_free(). But there could be other issues too, such as incorrect event timestamp metadata. Explicitly detect that an event is already active in event_gaga() and output an error message (and cope gracefully, by losing the new event). This ensures that the event that does trigger will be handled correctly and have correct metadata, etc. --- src/runner.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/runner.c b/src/runner.c index aafb783..19d6bcc 100644 --- a/src/runner.c +++ b/src/runner.c @@ -43,11 +43,15 @@ static void event_gaga(struct event *event, struct event **ev, short what, event_callback_fn func, struct timeval *tv) { - assert(event && event->lmapd && ev && *ev == NULL); + assert(event && event->lmapd && ev); - *ev = event_new(event->lmapd->base, -1, what, func, event); - if (!*ev || event_add(*ev, tv) < 0) { - lmap_err("failed to create/add event for '%s'", event->name); + if (!(*ev)) { + *ev = event_new(event->lmapd->base, -1, what, func, event); + if (!*ev || event_add(*ev, tv) < 0) { + lmap_err("failed to create/add event for '%s'", event->name); + } + } else { + lmap_err("failed to create/add event for '%s': event already pending, maybe due to too large a random spread?", event->name); } } #endif From 1ba164370fcbe285ba2753b2e6ef63661559a586 Mon Sep 17 00:00:00 2001 From: Henrique de Moraes Holschuh Date: Mon, 4 Jan 2021 18:18:15 -0300 Subject: [PATCH 3/3] lmapd: refuse too large random spreads for periodic events Refuse configurations that would knowingly cause periodic event reordering due to a too large random spread. It would be nice to detect this also for the calendar event type, but that's non-trivial, and not worth the hassle now that lmapd has been changed to handle this gracefully (instead of triggering a crash or assertion failure). CEPTRO.br-issue: MDC-1016 --- src/data.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/data.c b/src/data.c index 59b491b..b222a15 100644 --- a/src/data.c +++ b/src/data.c @@ -1162,13 +1162,21 @@ lmap_event_valid(struct lmap *lmap, struct event *event) valid = 0; } - if (event->type == LMAP_EVENT_TYPE_PERIODIC - && ! (event->flags & LMAP_EVENT_FLAG_INTERVAL_SET)) { - lmap_err("event %s%s%srequires an interval", - event->name ? "'" : "", - event->name ? event->name : "", - event->name ? "' " : ""); - valid = 0; + if (event->type == LMAP_EVENT_TYPE_PERIODIC) { + if (!(event->flags & LMAP_EVENT_FLAG_INTERVAL_SET)) { + lmap_err("event %s%s%srequires an interval", + event->name ? "'" : "", + event->name ? event->name : "", + event->name ? "' " : ""); + valid = 0; + } else if (event->flags & LMAP_EVENT_FLAG_RANDOM_SPREAD_SET + && event->random_spread >= event->interval) { + lmap_err("event %s%s%shas a random spread too large for its interval", + event->name ? "'" : "", + event->name ? event->name : "", + event->name ? "' " : ""); + valid = 0; + } } if (event->type == LMAP_EVENT_TYPE_CALENDAR) { if (event->months == 0) {