From fe1f029be72404a07a64bc8e30393634de3fa466 Mon Sep 17 00:00:00 2001 From: Arran Cudbard-Bell Date: Thu, 2 Oct 2025 17:37:36 -0600 Subject: [PATCH] Remove UNLANG_ACTION_STOP_PROCESSING It's not needed anymore. Fatal errors should use `RETURN_UNLANG_ACTION_FATAL` to signal the request to stop. Fix issue where finally-timeout test wasn't actually running because the dummy request got cancalled during the virtual server call (oops). Add regression tests for module calls being immediately cancelled in finally sections. --- src/lib/io/control.h | 2 +- src/lib/unlang/action.h | 7 ++++- src/lib/unlang/function.c | 14 +--------- src/lib/unlang/interpret.c | 14 ++-------- src/lib/unlang/load_balance.c | 6 ++--- src/lib/unlang/map.c | 4 +-- src/lib/unlang/module.c | 27 +++---------------- src/lib/unlang/switch.c | 4 +-- src/lib/unlang/timeout.c | 2 +- src/lib/unlang/tmpl.c | 2 +- src/lib/unlang/try.c | 2 +- src/lib/unlang/xlat.c | 6 ++--- src/tests/keywords/finally | 3 +-- src/tests/keywords/finally-timeout | 4 +++ src/tests/keywords/finally-timeout-module | 4 +++ .../keywords/finally-timeout-module.attrs | 6 +++++ .../keywords/finally-timeout-module.servers | 22 +++++++++++++++ src/tests/keywords/finally-timeout.attrs | 6 +++++ src/tests/keywords/finally-timeout.servers | 18 +++++++++++++ src/tests/keywords/finally.servers | 19 ------------- 20 files changed, 88 insertions(+), 84 deletions(-) create mode 100644 src/tests/keywords/finally-timeout create mode 100644 src/tests/keywords/finally-timeout-module create mode 100644 src/tests/keywords/finally-timeout-module.attrs create mode 100644 src/tests/keywords/finally-timeout-module.servers create mode 100644 src/tests/keywords/finally-timeout.attrs create mode 100644 src/tests/keywords/finally-timeout.servers diff --git a/src/lib/io/control.h b/src/lib/io/control.h index c82782bbbf..fde6a55b1b 100644 --- a/src/lib/io/control.h +++ b/src/lib/io/control.h @@ -72,7 +72,7 @@ ssize_t fr_control_message_pop(fr_atomic_queue_t *aq, uint32_t *p_id, void *data int fr_control_callback_add(fr_control_t *c, uint32_t id, void *ctx, fr_control_callback_t callback) CC_HINT(nonnull(1,4)); int fr_control_callback_delete(fr_control_t *c, uint32_t id) CC_HINT(nonnull); -int fr_control_same_thread(fr_control_t *c); +int fr_control_same_thread(fr_control_t *c) CC_HINT(nonnull); #ifdef __cplusplus } diff --git a/src/lib/unlang/action.h b/src/lib/unlang/action.h index 6212cfd017..c88e16ddf7 100644 --- a/src/lib/unlang/action.h +++ b/src/lib/unlang/action.h @@ -39,9 +39,14 @@ typedef enum { UNLANG_ACTION_PUSHED_CHILD, //!< #unlang_t pushed a new child onto the stack, //!< execute it instead of continuing. UNLANG_ACTION_YIELD, //!< Temporarily pause execution until an event occurs. - UNLANG_ACTION_STOP_PROCESSING //!< Break out of processing the current request (unwind). } unlang_action_t; +#define RETURN_UNLANG_ACTION_FATAL \ +{ \ + unlang_interpret_signal(request, FR_SIGNAL_CANCEL); \ + return UNLANG_ACTION_FAIL; \ +} + #ifdef __cplusplus } #endif diff --git a/src/lib/unlang/function.c b/src/lib/unlang/function.c index 7c8dc1174b..17489614ea 100644 --- a/src/lib/unlang/function.c +++ b/src/lib/unlang/function.c @@ -125,9 +125,6 @@ again: ua = func(p_result, request, state->uctx); if (REPEAT(state)) { /* set again by func */ switch (ua) { - case UNLANG_ACTION_STOP_PROCESSING: - break; - case UNLANG_ACTION_CALCULATE_RESULT: goto again; @@ -161,9 +158,6 @@ static unlang_action_t call_with_result(unlang_result_t *p_result, request_t *re state->func_name = NULL; if (REPEAT(state)) { switch (ua) { - case UNLANG_ACTION_STOP_PROCESSING: - break; - case UNLANG_ACTION_CALCULATE_RESULT: ua = call_with_result_repeat(p_result, request, frame); break; @@ -208,9 +202,6 @@ again: ua = func(request, state->uctx); if (REPEAT(state)) { /* set again by func */ switch (ua) { - case UNLANG_ACTION_STOP_PROCESSING: - break; - case UNLANG_ACTION_CALCULATE_RESULT: goto again; @@ -253,9 +244,6 @@ static unlang_action_t call_no_result(UNUSED unlang_result_t *p_result, request_ state->func_name = NULL; if (REPEAT(state)) { switch (ua) { - case UNLANG_ACTION_STOP_PROCESSING: - break; - case UNLANG_ACTION_CALCULATE_RESULT: ua = call_no_result_repeat(p_result, request, frame); break; @@ -568,7 +556,7 @@ void unlang_function_init(void) .dump = unlang_function_dump, .unlang_size = sizeof(unlang_group_t), - .unlang_name = "unlang_group_t", + .unlang_name = "unlang_group_t", .frame_state_size = sizeof(unlang_frame_state_func_t), .frame_state_type = "unlang_frame_state_func_t", diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index c2ebfb2f39..4d84f689eb 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -46,7 +46,6 @@ static fr_table_num_ordered_t const unlang_action_table[] = { { L("calculate-result"), UNLANG_ACTION_CALCULATE_RESULT }, { L("next"), UNLANG_ACTION_EXECUTE_NEXT }, { L("pushed-child"), UNLANG_ACTION_PUSHED_CHILD }, - { L("stop"), UNLANG_ACTION_STOP_PROCESSING }, { L("yield"), UNLANG_ACTION_YIELD } }; static size_t unlang_action_table_len = NUM_ELEMENTS(unlang_action_table); @@ -379,7 +378,7 @@ static int _local_variables_free(unlang_variable_ref_t *ref) * @return * - UNLANG_ACTION_PUSHED_CHILD on success. * - UNLANG_ACTION_EXECUTE_NEXT do nothing, but just go to the next sibling instruction - * - UNLANG_ACTION_STOP_PROCESSING, fatal error, usually stack overflow. + * - UNLANG_ACTION_FAIL, fatal error, usually stack overflow. */ unlang_action_t unlang_interpret_push_children(unlang_result_t *p_result, request_t *request, rlm_rcode_t default_rcode, bool do_next_sibling) @@ -405,7 +404,7 @@ unlang_action_t unlang_interpret_push_children(unlang_result_t *p_result, reques if (unlang_interpret_push(p_result, request, unlang_list_head(&g->children), FRAME_CONF(default_rcode, UNLANG_SUB_FRAME), do_next_sibling) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } if (!g->variables) return UNLANG_ACTION_PUSHED_CHILD; @@ -831,15 +830,6 @@ unlang_frame_action_t frame_eval(request_t *request, unlang_stack_frame_t *frame if (is_unwinding(frame)) goto calculate_result; switch (ua) { - case UNLANG_ACTION_STOP_PROCESSING: - /* - * This marks all the cancellable - * frames with the unwind flag, - * and starts popping them. - */ - unlang_interpret_signal(request, FR_SIGNAL_CANCEL); - return UNLANG_FRAME_ACTION_POP; - /* * The operation resulted in additional frames * being pushed onto the stack, execution should diff --git a/src/lib/unlang/load_balance.c b/src/lib/unlang/load_balance.c index 2b723d3a6e..94e7d86874 100644 --- a/src/lib/unlang/load_balance.c +++ b/src/lib/unlang/load_balance.c @@ -93,7 +93,7 @@ push: */ if (unlang_interpret_push(&redundant->result, request, redundant->child, FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), UNLANG_NEXT_STOP) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; @@ -123,7 +123,7 @@ static unlang_action_t unlang_load_balance(unlang_result_t *p_result, request_t uint32_t count = 0; #ifdef STATIC_ANALYZER - if (!g || unlang_list_empty(&g->children)) return UNLANG_ACTION_STOP_PROCESSING; + if (!g || unlang_list_empty(&g->children)) return UNLANG_ACTION_FAIL; #else fr_assert(g != NULL); fr_assert(!unlang_list_empty(&g->children)); @@ -220,7 +220,7 @@ static unlang_action_t unlang_load_balance(unlang_result_t *p_result, request_t if (frame->instruction->type == UNLANG_TYPE_LOAD_BALANCE) { if (unlang_interpret_push(p_result, request, redundant->start, FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), UNLANG_NEXT_STOP) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; } diff --git a/src/lib/unlang/map.c b/src/lib/unlang/map.c index a25cb00f29..2256577dce 100644 --- a/src/lib/unlang/map.c +++ b/src/lib/unlang/map.c @@ -186,14 +186,14 @@ static unlang_action_t unlang_map_state_init(unlang_result_t *p_result, request_ case TMPL_TYPE_EXEC: if (unlang_tmpl_push(map_proc_state, NULL, &map_proc_state->src_result, request, inst->src, NULL, UNLANG_SUB_FRAME) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; case TMPL_TYPE_XLAT: if (unlang_xlat_push(map_proc_state, NULL, &map_proc_state->src_result, request, tmpl_xlat(inst->src), false) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c index 22e706f2bd..6ab8e0068c 100644 --- a/src/lib/unlang/module.c +++ b/src/lib/unlang/module.c @@ -178,7 +178,7 @@ unlang_action_t unlang_module_yield_to_xlat(TALLOC_CTX *ctx, unlang_result_t *p_ /* * Push the xlat function */ - if (unlang_xlat_push(ctx, p_result, out, request, exp, false) < 0) return UNLANG_ACTION_STOP_PROCESSING; + if (unlang_xlat_push(ctx, p_result, out, request, exp, false) < 0) RETURN_UNLANG_ACTION_FATAL; return UNLANG_ACTION_PUSHED_CHILD; } @@ -285,7 +285,9 @@ unlang_action_t unlang_module_yield_to_section(unlang_result_t *p_result, (void) unlang_module_yield(request, resume, signal, sigmask, rctx); if (unlang_interpret_push_section(p_result, request, subcs, - FRAME_CONF(default_rcode, UNLANG_SUB_FRAME)) < 0) return UNLANG_ACTION_STOP_PROCESSING; + FRAME_CONF(default_rcode, UNLANG_SUB_FRAME)) < 0) { + RETURN_UNLANG_ACTION_FATAL; + } return UNLANG_ACTION_PUSHED_CHILD; } @@ -615,17 +617,7 @@ static unlang_action_t unlang_module_resume(unlang_result_t *p_result, request_t state->env_data, state->rctx), request); safe_unlock(m->mmc.mi); - if (request->master_state == REQUEST_STOP_PROCESSING) ua = UNLANG_ACTION_STOP_PROCESSING; - switch (ua) { - case UNLANG_ACTION_STOP_PROCESSING: - RWARN("Module %s or worker signalled to stop processing request", m->mmc.mi->name); - state->thread->active_callers--; - request->module = state->previous_module; - p_result->rcode = RLM_MODULE_NOT_SET; - p_result->priority = MOD_ACTION_NOT_SET; - return UNLANG_ACTION_STOP_PROCESSING; - case UNLANG_ACTION_YIELD: /* * The module yielded but didn't set a @@ -888,18 +880,7 @@ static unlang_action_t unlang_module(unlang_result_t *p_result, request_t *reque request); safe_unlock(m->mmc.mi); - if (request->master_state == REQUEST_STOP_PROCESSING) ua = UNLANG_ACTION_STOP_PROCESSING; - switch (ua) { - /* - * It is now marked as "stop" when it wasn't before, we - * must have been blocked. - */ - case UNLANG_ACTION_STOP_PROCESSING: - RWARN("Module %s became unblocked", m->mmc.mi->name); - request->module = state->previous_module; - return UNLANG_ACTION_STOP_PROCESSING; - case UNLANG_ACTION_YIELD: state->thread->active_callers++; diff --git a/src/lib/unlang/switch.c b/src/lib/unlang/switch.c index df8ab6fc5e..b0783e8720 100644 --- a/src/lib/unlang/switch.c +++ b/src/lib/unlang/switch.c @@ -113,7 +113,7 @@ do_null_case: if (!found) return UNLANG_ACTION_EXECUTE_NEXT; if (unlang_interpret_push(NULL, request, found, FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), UNLANG_NEXT_STOP) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; @@ -524,7 +524,7 @@ void unlang_switch_init(void) .compile = unlang_compile_switch, .interpret = unlang_switch, - + .unlang_size = sizeof(unlang_switch_t), .unlang_name = "unlang_switch_t", diff --git a/src/lib/unlang/timeout.c b/src/lib/unlang/timeout.c index 4d259bb333..664e339ff1 100644 --- a/src/lib/unlang/timeout.c +++ b/src/lib/unlang/timeout.c @@ -117,7 +117,7 @@ static unlang_action_t unlang_timeout_set(UNUSED unlang_result_t *p_result, requ if (fr_timer_in(state, unlang_interpret_event_list(request)->tl, &state->ev, state->timeout, false, unlang_timeout_handler, state) < 0) { RPEDEBUG("Failed inserting event"); - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } frame_repeat(frame, unlang_timeout_resume_done); diff --git a/src/lib/unlang/tmpl.c b/src/lib/unlang/tmpl.c index c1bf1062c1..7a7d7dbeaa 100644 --- a/src/lib/unlang/tmpl.c +++ b/src/lib/unlang/tmpl.c @@ -252,7 +252,7 @@ static unlang_action_t unlang_tmpl(unlang_result_t *p_result, request_t *request push: if (unlang_xlat_push(state->ctx, &state->xlat_result, &state->list, request, tmpl_xlat(ut->tmpl), UNLANG_SUB_FRAME) < 0) { fail: - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; diff --git a/src/lib/unlang/try.c b/src/lib/unlang/try.c index aa1be92f8d..e38ca69def 100644 --- a/src/lib/unlang/try.c +++ b/src/lib/unlang/try.c @@ -42,7 +42,7 @@ static unlang_action_t skip_to_catch(UNUSED unlang_result_t *p_result, request_t if (gext->catch[rcode]) { if (unlang_interpret_push(NULL, request, gext->catch[rcode], FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), false) < 0) { - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; diff --git a/src/lib/unlang/xlat.c b/src/lib/unlang/xlat.c index 07fddf0537..4008a1060b 100644 --- a/src/lib/unlang/xlat.c +++ b/src/lib/unlang/xlat.c @@ -334,7 +334,7 @@ static unlang_action_t unlang_xlat_repeat(unlang_result_t *p_result, request_t * fr_value_box_list_talloc_free(&state->out); if (unlang_xlat_push(state->ctx, p_result, &state->out, request, child, false) < 0) { REXDENT(); - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; @@ -396,7 +396,7 @@ static unlang_action_t unlang_xlat(UNUSED unlang_result_t *p_result, request_t * fr_value_box_list_talloc_free(&state->out); if (unlang_xlat_push(state->ctx, p_result, &state->out, request, child, false) < 0) { RINDENT_RESTORE(request, state); - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; @@ -510,7 +510,7 @@ static unlang_action_t unlang_xlat_resume(unlang_result_t *p_result, request_t * fr_value_box_list_talloc_free(&state->out); if (unlang_xlat_push(state->ctx, state->p_result, &state->out, request, child, false) < 0) { RINDENT_RESTORE(request, state); - return UNLANG_ACTION_STOP_PROCESSING; + RETURN_UNLANG_ACTION_FATAL; } return UNLANG_ACTION_PUSHED_CHILD; diff --git a/src/tests/keywords/finally b/src/tests/keywords/finally index 56933d56a6..ca019a79a0 100644 --- a/src/tests/keywords/finally +++ b/src/tests/keywords/finally @@ -13,5 +13,4 @@ if Tmp-String-0 != 'bye-from-packet-fin' { test_fail } -# This should timeout, so it must call success in the finally section -call timeout-fin { } +success diff --git a/src/tests/keywords/finally-timeout b/src/tests/keywords/finally-timeout new file mode 100644 index 0000000000..663a2260c7 --- /dev/null +++ b/src/tests/keywords/finally-timeout @@ -0,0 +1,4 @@ +# This should timeout, so it must call success in the finally section +# Reply attributes are set in the finally section of the server we're calling +# and checked with the response filter, as this request will get cancelled too. +call timeout-fin { } diff --git a/src/tests/keywords/finally-timeout-module b/src/tests/keywords/finally-timeout-module new file mode 100644 index 0000000000..fa2dd49e85 --- /dev/null +++ b/src/tests/keywords/finally-timeout-module @@ -0,0 +1,4 @@ +# This should timeout, so it must call success in the finally section +# Reply attributes are set in the finally section of the server we're calling +# and checked with the response filter, as this request will get cancelled too. +call module-timeout-fin { } diff --git a/src/tests/keywords/finally-timeout-module.attrs b/src/tests/keywords/finally-timeout-module.attrs new file mode 100644 index 0000000000..cbbbdc9d1d --- /dev/null +++ b/src/tests/keywords/finally-timeout-module.attrs @@ -0,0 +1,6 @@ +Packet-Type = Access-Request + +# Should probably be Do-Not-Respond +Result-Status == success +Packet-Type == 0 +Reply-Message == bye-from-timeout-module diff --git a/src/tests/keywords/finally-timeout-module.servers b/src/tests/keywords/finally-timeout-module.servers new file mode 100644 index 0000000000..ee07bc728d --- /dev/null +++ b/src/tests/keywords/finally-timeout-module.servers @@ -0,0 +1,22 @@ +# Regression test We'd set the request master state to "Stop Processing" and this +# would result in module calls in finally being skipped. +server module-timeout-fin { + namespace = radius + + recv Access-Request { + # Trigger the overall timeout for the test binary + %time.advance(61s) + ok + } + + finally { + if (timeout) { + # This previously resulted in the request processing terminating early + reschedule + reply.Reply-Message := 'bye-from-timeout-module' + success + } else { + fail + } + } +} diff --git a/src/tests/keywords/finally-timeout.attrs b/src/tests/keywords/finally-timeout.attrs new file mode 100644 index 0000000000..71e88752b7 --- /dev/null +++ b/src/tests/keywords/finally-timeout.attrs @@ -0,0 +1,6 @@ +Packet-Type = Access-Request + +# Should probably be Do-Not-Respond +Result-Status == success +Packet-Type == 0 +Reply-Message == bye-from-timeout-fin diff --git a/src/tests/keywords/finally-timeout.servers b/src/tests/keywords/finally-timeout.servers new file mode 100644 index 0000000000..9009da1f6f --- /dev/null +++ b/src/tests/keywords/finally-timeout.servers @@ -0,0 +1,18 @@ +server timeout-fin { + namespace = radius + + recv Access-Request { + # Trigger the overall timeout for the test binary + %time.advance(61s) + ok + } + + finally { + if (timeout) { + reply.Reply-Message := 'bye-from-timeout-fin' + success + } else { + fail + } + } +} diff --git a/src/tests/keywords/finally.servers b/src/tests/keywords/finally.servers index 7c6bcee18c..9224decb87 100644 --- a/src/tests/keywords/finally.servers +++ b/src/tests/keywords/finally.servers @@ -26,22 +26,3 @@ server packet-fin { fail } } - -server timeout-fin { - namespace = radius - - recv Access-Request { - # Trigger the overall timeout for the test binary - %time.advance(61s) - ok - } - - finally { - if (timeout) { - Tmp-String-0 := 'bye-from-timeout-fin' - success - } else { - fail - } - } -} -- 2.47.3