From: Arran Cudbard-Bell Date: Tue, 6 Apr 2021 10:26:52 +0000 (+0100) Subject: Ensure stack frame state is freed in a deterministic way X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=26a0fc22fe2e38a5ed93d49e181ad054e2e1b051;p=thirdparty%2Ffreeradius-server.git Ensure stack frame state is freed in a deterministic way --- diff --git a/src/lib/io/worker.c b/src/lib/io/worker.c index 5ddfde7cdc..a0459c33a4 100644 --- a/src/lib/io/worker.c +++ b/src/lib/io/worker.c @@ -971,9 +971,10 @@ static void _worker_request_done_external(request_t *request, UNUSED rlm_rcode_t */ fr_assert_msg(request_is_internal(request) || request_is_detached(request) || (request->log.unlang_indent == 0), "Request %s bad log indentation - expected 0 got %u", request->name, request->log.unlang_indent); - fr_assert_msg((request->master_state == REQUEST_STOP_PROCESSING) || !unlang_interpret_is_resumable(request), + fr_assert_msg(!unlang_interpret_is_resumable(request), "Request %s is marked as yielded at end of processing", request->name); - + fr_assert_msg(unlang_interpret_stack_depth(request) == 0, + "Request %s stack depth %u > 0", request->name, unlang_interpret_stack_depth(request)); RDEBUG("Done request"); worker_send_reply(worker, request, request->master_state == REQUEST_STOP_PROCESSING ? 1 : 0, fr_time()); @@ -1069,6 +1070,12 @@ static void _worker_request_stop(request_t *request, void *uctx) fr_time_tracking_resume(&request->async->tracking, fr_time()); } + /* + * Let everyone know the request is being + * stopped. + */ + request->master_state = REQUEST_STOP_PROCESSING; + /* * If the request is in the runnable queue * yank it back out, so it's not "runnable" diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index 8af40cc90c..d1ed246e23 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -791,58 +791,6 @@ void *unlang_interpret_stack_alloc(TALLOC_CTX *ctx) return stack; } -/** Send a signal (usually stop) to a request - * - * This is typically called via an "async" action, i.e. an action - * outside of the normal processing of the request. - * - * If there is no #unlang_module_signal_t callback defined, the action is ignored. - * - * The signaling stops at the "limit" frame. This is so that keywords - * such as "timeout" and "limit" can signal frames *lower* than theirs - * to stop, but then continue with their own work. - * - * @param[in] request The current request. - * @param[in] action to signal. - * @param[in] limit the frame at which to stop signaling. - */ -static inline CC_HINT(always_inline) void frame_signal(request_t *request, fr_state_signal_t action, int limit) -{ - unlang_stack_frame_t *frame; - unlang_stack_t *stack = request->stack; - int i, depth = stack->depth; - - (void)talloc_get_type_abort(request, request_t); /* Check the request hasn't already been freed */ - - fr_assert(stack->depth > 0); - - /* - * Walk back up the stack, calling signal handlers - * to cancel any pending operations and free/release - * any resources. - * - * There may be multiple resumption points in the - * stack, as modules can push xlats and function - * calls. - */ - for (i = depth; i > limit; i--) { - stack->depth = i; /* We could also pass in the frame to the signal function */ - frame = &stack->frame[stack->depth]; - - if (is_top_frame(frame)) continue; /* Skip top frames as they have no instruction */ - - /* - * Be gracious in errors. - */ - if (!is_yielded(frame)) continue; - - if (!frame->signal) continue; - - frame->signal(request, frame, action); - } - stack->depth = depth; /* Reset */ -} - /** Indicate to the caller of the interpreter that this request is complete * */ @@ -901,22 +849,72 @@ void unlang_interpret_request_detach(request_t *request) * * If there is no #unlang_module_signal_t callback defined, the action is ignored. * + * The signaling stops at the "limit" frame. This is so that keywords + * such as "timeout" and "limit" can signal frames *lower* than theirs + * to stop, but then continue with their own work. + * * @param[in] request The current request. * @param[in] action to signal. + * @param[in] limit the frame at which to stop signaling. */ -void unlang_interpret_signal(request_t *request, fr_state_signal_t action) +static inline CC_HINT(always_inline) void frame_signal(request_t *request, fr_state_signal_t action, int limit) { + unlang_stack_frame_t *frame; unlang_stack_t *stack = request->stack; + int i, depth = stack->depth; + + (void)talloc_get_type_abort(request, request_t); /* Check the request hasn't already been freed */ + + fr_assert(stack->depth > 0); - switch (action) { /* - * If we're stopping, then mark the request as stopped. - * Then, call the frame signal handler. + * Destructive signal where we clean each of the + * stack frames up in turn. + * + * We do this to avoid possible free ordering + * issues where memory allocated by modules higher + * in the stack is used by modules lower in the + * stack. */ - case FR_SIGNAL_CANCEL: - request->master_state = REQUEST_STOP_PROCESSING; - break; + if (action == FR_SIGNAL_CANCEL) { + for (i = depth; i > limit; i--) { + frame = &stack->frame[i]; + if (frame->signal) frame->signal(request, frame, action); + frame_pop(request->stack); + } + return; + } + + /* + * Walk back up the stack, calling signal handlers + * to cancel any pending operations and free/release + * any resources. + * + * There may be multiple resumption points in the + * stack, as modules can push xlats and function + * calls. + */ + for (i = depth; i > limit; i--) { + frame = &stack->frame[i]; + if (frame->signal) frame->signal(request, frame, action); + } +} +/** Send a signal (usually stop) to a request + * + * This is typically called via an "async" action, i.e. an action + * outside of the normal processing of the request. + * + * If there is no #unlang_module_signal_t callback defined, the action is ignored. + * + * @param[in] request The current request. + * @param[in] action to signal. + */ +void unlang_interpret_signal(request_t *request, fr_state_signal_t action) +{ + unlang_stack_t *stack = request->stack; + + switch (action) { case FR_SIGNAL_DETACH: /* * Ensure the request is able to be detached @@ -930,7 +928,7 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action) } /* - * Requests that haven't been run through the interpret + * Requests that haven't been run through the interpreter * yet should have a stack depth of zero, so we don't * need to do anything. */