From: Arran Cudbard-Bell Date: Sat, 10 Dec 2022 21:01:45 +0000 (-0600) Subject: Rework our wrapper code around user events, and add a surrogate user event on process... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2e1d6f84c02880b6ef29f43b4888bd5a83137e38;p=thirdparty%2Ffreeradius-server.git Rework our wrapper code around user events, and add a surrogate user event on process early exit --- diff --git a/src/lib/server/exec.c b/src/lib/server/exec.c index 63f3b637b21..0a9d52e9d2a 100644 --- a/src/lib/server/exec.c +++ b/src/lib/server/exec.c @@ -1007,28 +1007,7 @@ int fr_exec_start(TALLOC_CTX *ctx, fr_exec_state_t *exec, request_t *request, /* * Tell the event loop that it needs to wait for this PID */ - switch (fr_event_pid_wait(ctx, el, &exec->ev_pid, exec->pid, exec_reap, exec)) { - /* - * We're actually waiting for the process using the event loop - */ - case 0: - /* - * Setup event to kill the child process after a period of time. - */ - if (fr_time_delta_ispos(timeout) && - (fr_event_timer_in(ctx, el, &exec->ev, timeout, exec_timeout, exec) < 0)) goto fail_and_close; - break; - - /* - * The Process has already exited, so no need to setup timers - */ - case 1: - break; - - /* - * An actual error... - */ - default: + if (fr_event_pid_wait(ctx, el, &exec->ev_pid, exec->pid, exec_reap, exec) < 0) { exec->pid = -1; RPEDEBUG("Failed adding watcher for child process"); @@ -1051,5 +1030,11 @@ int fr_exec_start(TALLOC_CTX *ctx, fr_exec_state_t *exec, request_t *request, goto fail; } + /* + * Setup event to kill the child process after a period of time. + */ + if (fr_time_delta_ispos(timeout) && + (fr_event_timer_in(ctx, el, &exec->ev, timeout, exec_timeout, exec) < 0)) goto fail_and_close; + return 0; } diff --git a/src/lib/util/event.c b/src/lib/util/event.c index 494d08d7c0f..02ad409a926 100644 --- a/src/lib/util/event.c +++ b/src/lib/util/event.c @@ -291,7 +291,7 @@ struct fr_event_fd { ///< this should really go away and we should pass around ///< handles directly. - fr_event_list_t *el; //!< because talloc_parent() is O(N) in number of objects + fr_event_list_t *el; //!< Event list this event belongs to. fr_event_filter_t filter; int fd; //!< File descriptor we're listening for events on. @@ -325,13 +325,24 @@ struct fr_event_fd { }; struct fr_event_pid { - fr_event_list_t *el; //!< because talloc_parent() is O(N) in number of objects + fr_event_list_t *el; //!< Event list this event belongs to. + + bool is_registered; //!< Whether this user event has been registered + ///< with the event loop. + pid_t pid; //!< child to wait for fr_event_pid_t const **parent; fr_event_pid_cb_t callback; //!< callback to run when the child exits void *uctx; //!< Context pointer to pass to each file descriptor callback. + /** Fields that are only used if we're being triggered by a user event + */ + struct { + fr_event_user_t *ev; //!< Fallback user event we use to raise a PID event when + ///< a race occurs with kevent. + int status; //!< Status we got from waitid. + } early_exit; #ifndef NDEBUG char const *file; //!< Source file this event was last updated in. int line; //!< Line this event was last updated on. @@ -341,7 +352,7 @@ struct fr_event_pid { /** Hold additional information for automatically reaped PIDs */ typedef struct { - fr_event_list_t *el; //!< because talloc_parent() is O(N) in number of objects + fr_event_list_t *el; //!< Event list this event belongs to. fr_event_pid_t const *pid_ev; //!< pid_ev this reaper is bound to. fr_dlist_t entry; //!< If the fr_event_pid is in the detached, reap state, @@ -353,6 +364,24 @@ typedef struct { void *uctx; //!< Context pointer to pass to each file descriptor callback. } fr_event_pid_reap_t; +/** Callbacks for kevent() user events + * + */ +struct fr_event_user_s { + fr_event_list_t *el; //!< Event list this event belongs to. + + bool is_registered; //!< Whether this user event has been registered + ///< with the event loop. + + fr_event_user_cb_t callback; //!< The callback to call. + void *uctx; //!< Context for the callback. + +#ifndef NDEBUG + char const *file; //!< Source file this event was last updated in. + int line; //!< Line this event was last updated on. +#endif +}; + /** Callbacks to perform when the event handler is about to check the events * */ @@ -371,17 +400,6 @@ typedef struct { void *uctx; //!< Context for the callback. } fr_event_post_t; -/** Callbacks for kevent() user events - * - */ -typedef struct { - fr_dlist_t entry; //!< Linked list of callback. - uintptr_t ident; //!< The identifier of this event. - fr_event_user_handler_t callback; //!< The callback to call. - void *uctx; //!< Context for the callback. -} fr_event_user_t; - - /** Stores all information relating to an event list * */ @@ -403,7 +421,6 @@ struct fr_event_list { int kq; //!< instance associated with this event list. fr_dlist_head_t pre_callbacks; //!< callbacks when we may be idle... - fr_dlist_head_t user_callbacks; //!< EVFILT_USER callbacks fr_dlist_head_t post_callbacks; //!< post-processing callbacks fr_dlist_head_t pid_to_reap; //!< A list of all orphaned child processes we're @@ -1591,7 +1608,7 @@ static int _event_pid_free(fr_event_pid_t *ev) struct kevent evset; if (ev->parent) *ev->parent = NULL; - if (ev->pid < 0) return 0; /* already deleted from kevent */ + if (!ev->is_registered || (ev->pid < 0)) return 0; /* already deleted from kevent */ EVENT_DEBUG("%p - Disabling event for PID %u - %p was freed", ev->el, (unsigned int)ev->pid, ev); @@ -1602,6 +1619,70 @@ static int _event_pid_free(fr_event_pid_t *ev) return 0; } +/** Evaluate a EVFILT_PROC event + * + */ +static inline CC_HINT(always_inline) +void event_pid_eval(fr_event_list_t *el, struct kevent *kev) +{ + pid_t pid; + fr_event_pid_t *ev; + fr_event_pid_cb_t callback; + void *uctx; + + EVENT_DEBUG("%p - PID %u exited with status %i", + el, (unsigned int)kev->ident, (unsigned int)kev->data); + + ev = talloc_get_type_abort((void *)kev->udata, fr_event_pid_t); + + fr_assert(ev->pid == (pid_t) kev->ident); + fr_assert((kev->fflags & NOTE_EXIT) != 0); + + pid = ev->pid; + callback = ev->callback; + uctx = ev->uctx; + + ev->is_registered = false; /* so we won't hit kevent again when it's freed */ + + /* + * Delete the event before calling it. + * + * This also sets the parent pointer + * to NULL, so the thing that started + * monitoring the process knows the + * handle is no longer valid. + * + * EVFILT_PROC NOTE_EXIT events are always + * oneshot no matter what flags we pass, + * so we're just reflecting the state of + * the kqueue. + */ + talloc_free(ev); + + if (callback) callback(el, pid, (int) kev->data, uctx); +} + +/** Called on the next loop through the event loop when inserting an EVFILT_PROC event fails + * + * This is just a trampoleen function which takes the user event and simulates + * an EVFILT_PROC event from it. + * + * @param[in] el That received the event. + * @param[in] uctx An fr_event_pid_t to process. + */ +static void _fr_event_pid_early_exit(fr_event_list_t *el, void *uctx) +{ + fr_event_pid_t *ev = talloc_get_type_abort(uctx, fr_event_pid_t); + + EVENT_DEBUG("%p - PID %ld exited early, triggered through user event", el, (long)ev->pid); + + /* + * Simulate a real struct kevent with the values we + * recorded in fr_event_pid_wait. + */ + event_pid_eval(el, &(struct kevent){ .ident = ev->pid, .data = ev->early_exit.status, .udata = ev }); +} + /** Insert a PID event into an event list * * @note The talloc parent of the memory returned in ev_p must not be changed. @@ -1643,6 +1724,7 @@ int _fr_event_pid_wait(NDEBUG_LOCATION_ARGS .line = line, #endif }; + talloc_set_destructor(ev, _event_pid_free); /* * macOS only, on FreeBSD NOTE_EXIT always provides @@ -1655,6 +1737,7 @@ int _fr_event_pid_wait(NDEBUG_LOCATION_ARGS EVENT_DEBUG("%p - Adding exit waiter for PID %u", el, (unsigned int)pid); EV_SET(&evset, pid, EVFILT_PROC, EV_ADD | EV_ONESHOT, NOTE_EXIT | NOTE_EXITSTATUS, 0, ev); + ev->is_registered = true; /* * This deals with the race where the process exited @@ -1674,8 +1757,8 @@ int _fr_event_pid_wait(NDEBUG_LOCATION_ARGS */ fr_strerror_clear(); - talloc_free(ev); - + ev->is_registered = false; + /* * If the child exited before kevent() was * called, we need to get its status via @@ -1718,29 +1801,54 @@ int _fr_event_pid_wait(NDEBUG_LOCATION_ARGS el, (long)pid, fr_table_str_by_value(si_codes, info.si_code, ""), info.si_code, info.si_status); - if (callback) callback(el, pid, info.si_status, uctx); - return 1; + /* + * Record the status for later + */ + ev->early_exit.status = info.si_status; + + /* + * The user event acts as a surrogate for + * an EVFILT_PROC event, and will be evaluated + * during the next loop through the event loop. + * + * It will be automatically deleted when the + * fr_event_pid_t is freed. + * + * Previously we tried to evaluate the proc + * callback here directly, but this lead to + * multiple problems, the biggest being that + * setting requests back to resumable failed + * because they were not yet yielded, + * leading to hangs. + */ + if (fr_event_user_insert(ev, &ev->early_exit.ev, el, true, _fr_event_pid_early_exit, ev) < 0) { + fr_strerror_printf_push("Failed adding wait for PID %ld, and failed adding " + "backup user event", (long) pid); + error: + talloc_free(ev); + return -1; + } + break; default: fr_strerror_printf("Unexpected code %s (%u) whilst waiting on PID %ld", fr_table_str_by_value(si_codes, info.si_code, ""), info.si_code, (long) pid); - return -1; - } - } - /* - * Print this error here, so that the caller gets - * the error from kevent(), and not waitpid(). - */ - fr_strerror_printf("Failed adding waiter for PID %ld - kevent %s, waitid %s", - (long) pid, fr_syserror(evset.flags), fr_syserror(errno)); + goto error; + } + } else { + /* + * Print this error here, so that the caller gets + * the error from kevent(), and not waitpid(). + */ + fr_strerror_printf("Failed adding waiter for PID %ld - kevent %s, waitid %s", + (long) pid, fr_syserror(evset.flags), fr_syserror(errno)); - return -1; + goto error; + } } - talloc_set_destructor(ev, _event_pid_free); - /* * Sometimes the caller doesn't care about getting the * PID. But we still want to clean it up. @@ -1968,56 +2076,120 @@ force: return processed; } +/** Memory will not be freed if we fail to remove the event from the kqueue + * + * It's easier to debug memory leaks with modern tooling than it is + * to determine why we get random failures and event leaks inside of kqueue. + * + * @return + * - 0 on success. + * - -1 on failure. + */ +static int _event_user_delete(fr_event_user_t *ev) +{ + if (ev->is_registered) { + struct kevent evset; + + EV_SET(&evset, (uintptr_t)ev, EVFILT_USER, EV_DELETE, 0, 0, 0); + + if (unlikely(kevent(ev->el->kq, &evset, 1, NULL, 0, NULL) < 0)) { + fr_strerror_printf("Failed removing user event - kevent %s", fr_syserror(evset.flags)); + return -1; + } + ev->is_registered = false; + } + + return 0; +} + +static inline CC_HINT(always_inline) +void event_user_eval(fr_event_list_t *el, struct kevent *kev) +{ + fr_event_user_t *ev; + + /* + * This is just a "wakeup" event, which + * is always ignored. + */ + if (kev->ident == 0) return; + + ev = talloc_get_type_abort((void *)kev->ident, fr_event_user_t); + fr_assert((uintptr_t)ev == kev->ident); + + ev->callback(el, ev->uctx); +} + /** Add a user callback to the event list. * + * @param[in] ctx to allocate the event in. + * @param[out] ev_p Where to write a pointer. * @param[in] el Containing the timer events. + * @param[in] trigger Whether the user event is triggered initially. * @param[in] callback for EVFILT_USER. * @param[in] uctx for the callback. * @return - * - 0 on error - * - uintptr_t ident for EVFILT_USER signaling + * - 0 on success. + * - -1 on error. */ -uintptr_t fr_event_user_insert(fr_event_list_t *el, fr_event_user_handler_t callback, void *uctx) +int _fr_event_user_insert(NDEBUG_LOCATION_ARGS + TALLOC_CTX *ctx, fr_event_user_t **ev_p, + fr_event_list_t *el, + bool trigger, fr_event_user_cb_t callback, void *uctx) { - fr_event_user_t *user; + fr_event_user_t *ev; + struct kevent evset; - user = talloc(el, fr_event_user_t); - user->callback = callback; - user->uctx = uctx; - user->ident = (uintptr_t) user; + ev = talloc(ctx, fr_event_user_t); + if (unlikely(ev == NULL)) { + fr_strerror_const("Out of memory"); + return -1; + } + *ev = (fr_event_user_t) { + .el = el, + .callback = callback, + .uctx = uctx, +#ifndef NDEBUG + .file = file, + .line = line, +#endif + }; - fr_dlist_insert_tail(&el->user_callbacks, user); + EV_SET(&evset, (uintptr_t)ev, + EVFILT_USER, EV_ADD | EV_DISPATCH, (trigger * NOTE_TRIGGER), 0, ev); - return user->ident; + if (unlikely(kevent(el->kq, &evset, 1, NULL, 0, NULL) < 0)) { + fr_strerror_printf("Failed adding user event - kevent %s", fr_syserror(evset.flags)); + talloc_free(ev); + return -1; + } + ev->is_registered = true; + talloc_set_destructor(ev, _event_user_delete); + + *ev_p = ev; + + return 0; } -/** Delete a user callback to the event list. +/** Trigger a user event * - * @param[in] el Containing the timer events. - * @param[in] callback for EVFILT_USER. - * @param[in] uctx for the callback. + * @param[in] el containing the user event. + * @param[in] ev Handle for the user event. * @return - * - < 0 on error - * - 0 on success + * - 0 on success. + * - -1 on error. */ -int fr_event_user_delete(fr_event_list_t *el, fr_event_user_handler_t callback, void *uctx) +int fr_event_user_trigger(fr_event_list_t *el, fr_event_user_t *ev) { - fr_event_user_t *user, *next; + struct kevent evset; - for (user = fr_dlist_head(&el->user_callbacks); - user != NULL; - user = next) { - next = fr_dlist_next(&el->user_callbacks, user); + EV_SET(&evset, (uintptr_t)ev, EVFILT_USER, EV_ENABLE, NOTE_TRIGGER, 0, NULL); - if ((user->callback == callback) && - (user->uctx == uctx)) { - fr_dlist_remove(&el->user_callbacks, user); - talloc_free(user); - return 0; - } + if (unlikely(kevent(el->kq, &evset, 1, NULL, 0, NULL) < 0)) { + fr_strerror_printf("Failed triggering user event - kevent %s", fr_syserror(evset.flags)); + return -1; } - return -1; + return 0; } /** Add a pre-event callback to the event list. @@ -2343,63 +2515,14 @@ void fr_event_service(fr_event_list_t *el) */ switch (el->events[i].filter) { case EVFILT_USER: - { - fr_event_user_t *user; - - /* - * This is just a "wakeup" event, which - * is always ignored. - */ - if (el->events[i].ident == 0) continue; - - user = talloc_get_type_abort((void *)el->events[i].ident, fr_event_user_t); - fr_assert(user->ident == el->events[i].ident); - - user->callback(el->kq, &el->events[i], user->uctx); - } + event_user_eval(el, &el->events[i]); continue; /* * Process proc events */ case EVFILT_PROC: - { - pid_t pid; - fr_event_pid_t *pev; - fr_event_pid_cb_t callback; - void *uctx; - - EVENT_DEBUG("%p - PID %u exited with status %i", - el, (unsigned int)el->events[i].ident, (unsigned int)el->events[i].data); - - pev = talloc_get_type_abort((void *)el->events[i].udata, fr_event_pid_t); - - fr_assert(pev->pid == (pid_t) el->events[i].ident); - fr_assert((el->events[i].fflags & NOTE_EXIT) != 0); - - pid = pev->pid; - callback = pev->callback; - uctx = pev->uctx; - - pev->pid = -1; /* so we won't hit kevent again when it's freed */ - - /* - * Delete the event before calling it. - * - * This also sets the parent pointer - * to NULL, so the thing that started - * monitoring the process knows the - * handle is no longer valid. - * - * EVFILT_PROC NOTE_EXIT events are always - * oneshot no matter what flags we pass, - * so we're just reflecting the state of - * the kqueue. - */ - talloc_free(pev); - - if (callback) callback(el, pid, (int) el->events[i].data, uctx); - } + event_pid_eval(el, &el->events[i]); continue; /* @@ -2699,7 +2822,6 @@ fr_event_list_t *fr_event_list_alloc(TALLOC_CTX *ctx, fr_event_status_cb_t statu fr_dlist_talloc_init(&el->pre_callbacks, fr_event_pre_t, entry); fr_dlist_talloc_init(&el->post_callbacks, fr_event_post_t, entry); - fr_dlist_talloc_init(&el->user_callbacks, fr_event_user_t, entry); fr_dlist_talloc_init(&el->ev_to_add, fr_event_timer_t, entry); fr_dlist_talloc_init(&el->pid_to_reap, fr_event_pid_reap_t, entry); fr_dlist_talloc_init(&el->fd_to_free, fr_event_fd_t, entry); diff --git a/src/lib/util/event.h b/src/lib/util/event.h index 496bc21446b..96c1af951ae 100644 --- a/src/lib/util/event.h +++ b/src/lib/util/event.h @@ -52,6 +52,10 @@ typedef struct fr_event_timer fr_event_timer_t; */ typedef struct fr_event_pid fr_event_pid_t; +/** An opaquer user event handle + */ +typedef struct fr_event_user_s fr_event_user_t; + /** The type of filter to install for an FD */ typedef enum { @@ -152,11 +156,10 @@ typedef void (*fr_event_pid_cb_t)(fr_event_list_t *el, pid_t pid, int status, vo /** Called when a user kevent occurs * - * @param[in] kq that received the user kevent. - * @param[in] kev The kevent. + * @param[in] el Event list * @param[in] uctx User ctx passed to #fr_event_user_insert. */ -typedef void (*fr_event_user_handler_t)(int kq, struct kevent const *kev, void *uctx); +typedef void (*fr_event_user_cb_t)(fr_event_list_t *el, void *uctx); /** Alternative time source, useful for testing * @@ -268,8 +271,16 @@ unsigned int fr_event_list_reap_signal(fr_event_list_t *el, fr_time_delta_t time int fr_event_timer_run(fr_event_list_t *el, fr_time_t *when); -uintptr_t fr_event_user_insert(fr_event_list_t *el, fr_event_user_handler_t user, void *uctx) CC_HINT(nonnull(1,2)); -int fr_event_user_delete(fr_event_list_t *el, fr_event_user_handler_t user, void *uctx) CC_HINT(nonnull(1,2)); +int _fr_event_user_insert(NDEBUG_LOCATION_ARGS + TALLOC_CTX *ctx, fr_event_user_t **ev_p, + fr_event_list_t *el, + bool trigger, fr_event_user_cb_t callback, void *uctx) CC_HINT(nonnull(NDEBUG_LOCATION_NONNULL(2))); +#define fr_event_user_insert(_ctx, _ev_p, _el, _trigger, _callback, _uctx) \ + _fr_event_user_insert(NDEBUG_LOCATION_EXP _ctx, _ev_p, _el, _trigger, _callback, _uctx) + +int fr_event_user_trigger(fr_event_list_t *el, fr_event_user_t *ev); + +int fr_event_user_delete(fr_event_list_t *el, fr_event_user_cb_t user, void *uctx) CC_HINT(nonnull(1,2)); int fr_event_pre_insert(fr_event_list_t *el, fr_event_status_cb_t callback, void *uctx) CC_HINT(nonnull(1,2)); int fr_event_pre_delete(fr_event_list_t *el, fr_event_status_cb_t callback, void *uctx) CC_HINT(nonnull(1,2));