From: Alan T. DeKok Date: Mon, 28 Jul 2025 00:27:42 +0000 (-0400) Subject: catch many many more corner cases with 'try' X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6a125b84b6f8ea1484b78abe538c65baa8ea9e9a;p=thirdparty%2Ffreeradius-server.git catch many many more corner cases with 'try' if the input compile_ctx has REJECT or RETURN for an action, AND there's a "try / catch" for it, we do NOT have the try actions stay "REJECT" or "RETURN" also change the behavior so catch { ... } means "catch ALL" of the rcodes. the error rcodes are special, too :( --- diff --git a/doc/antora/modules/reference/pages/unlang/catch.adoc b/doc/antora/modules/reference/pages/unlang/catch.adoc index 70f43d0977d..9539366951c 100644 --- a/doc/antora/modules/reference/pages/unlang/catch.adoc +++ b/doc/antora/modules/reference/pages/unlang/catch.adoc @@ -11,16 +11,17 @@ catch [ ] { } ---- -The `catch` statement runs a series of substatements in a block, but only if the previous xref:unlang/try.adoc[try] failed. +The `catch` statement runs a series of substatements in a block, but +only if the previous xref:unlang/try.adoc[try] failed. -:: Zero or more xref:unlang/condition/return_codes.adoc[return codes]. Multiple return codes are separated by commas. +:: Zero or more xref:unlang/condition/return_codes.adoc[return codes]. Multiple return codes are separated by spaces. [ statements ]:: The `unlang` commands which will be executed. A `catch` block can be empty. -If no return code is given, then the `catch` statement will match all of the return codes. Otherwise, the `catch` statement matches only the return codes which were listed. - Multiple `catch` statements can be placed one after the other, to `catch` different errors. Only one of the statements will be executed. Once a `catch` statement is finished, the interpreter will skip all trailing `catch` statements, and continue execution with the next statement. +As a special case, the final 'catch' statement can list no return codes. i.e `catch { ... }`. In that case, it will match all return codes which were not listed in previous 'catch' statements. + .Example [source,unlang] @@ -43,6 +44,8 @@ catch invalid { # skipped after "catch fail" is run. } ---- +## try / catch versus redundant + There is some overlap in functionality between xref:unlang/try.adoc[try] / `catch` and xref:unlang/redundant.adoc[redundant]. The main difference is that a xref:unlang/catch.adoc[catch] statement can catch specific failure codes. The xref:unlang/redundant.adoc[redundant] statement should be used to run @@ -52,5 +55,5 @@ In contrast, the `try` / `catch` statements should be used for more complex poli The `try` / `catch` statements can also run different statements for each failure code, which is not possible with xref:unlang/redundant.adoc[redundant]. -// Copyright (C) 2023 Network RADIUS SAS. Licenced under CC-by-NC 4.0. +// Copyright (C) 2025 Network RADIUS SAS. Licenced under CC-by-NC 4.0. // This documentation was developed by Network RADIUS SAS. diff --git a/doc/antora/modules/reference/pages/unlang/try.adoc b/doc/antora/modules/reference/pages/unlang/try.adoc index 31224dfeffd..bbe171375ef 100644 --- a/doc/antora/modules/reference/pages/unlang/try.adoc +++ b/doc/antora/modules/reference/pages/unlang/try.adoc @@ -6,21 +6,24 @@ try { [ statements ] } -catch { +catch [ ] { + ... +} +catch [ ] { ... } ---- The `try` statement runs a series of substatements in a block. If the -block returns an error such as `fail`, `reject`, `invalid`, or -`disallow`, a subsequent xref:unlang/catch.adoc[catch] block is -executed. +block returns an error such as `fail`, `reject`, `invalid`, +`disallow`, or `timeout`, a subsequent xref:unlang/catch.adoc[catch] +block is executed. [ statements ]:: The `unlang` commands which will be executed. A `try` block cannot be empty. -Every `try` block must be followed by a xref:unlang/catch.adoc[catch] -block. +Every `try` block must be followed by at least one +xref:unlang/catch.adoc[catch] block. .Example @@ -29,14 +32,14 @@ block. try { sql } -catch { +catch fail { # ... run only if sql failed ok # over-ride the "fail" code } ---- -There is some overlap in functionality between `try` / xref:unlang/catch.adoc[catch] and xref:unlang/redundant.adoc[redundant]. The main difference (TODO) is that a xref:unlang/catch.adoc[catch] statement can catch specific actions. +There is some overlap in functionality between `try` / xref:unlang/catch.adoc[catch] and xref:unlang/redundant.adoc[redundant]. See the xref:unlang/catch.adoc[catch] documentation for more details. -// Copyright (C) 2023 Network RADIUS SAS. Licenced under CC-by-NC 4.0. +// Copyright (C) 2025 Network RADIUS SAS. Licenced under CC-by-NC 4.0. // This documentation was developed by Network RADIUS SAS. diff --git a/src/lib/unlang/catch.c b/src/lib/unlang/catch.c index 102526e3eb4..d2673ba3b6c 100644 --- a/src/lib/unlang/catch.c +++ b/src/lib/unlang/catch.c @@ -34,24 +34,18 @@ static unlang_action_t unlang_catch(UNUSED unlang_result_t *p_result, request_t } -static int catch_argv(CONF_SECTION *cs, unlang_try_t *gext, char const *name, unlang_t *c) +/* + * Sanity checks have already been done by compile_try(). + */ +static void catch_argv(unlang_try_t *gext, char const *name, unlang_t *c) { - int rcode; + rlm_rcode_t rcode; - rcode = fr_table_value_by_str(mod_rcode_table, name, -1); - if (rcode < 0) { - cf_log_err(cs, "Unknown rcode '%s'.", name); - return -1; - } - - if (gext->catch[rcode]) { - cf_log_err(cs, "Duplicate rcode '%s'.", name); - return -1; - } + rcode = fr_table_value_by_str(mod_rcode_table, name, RLM_MODULE_NOT_SET); + fr_assert(rcode != RLM_MODULE_NOT_SET); + fr_assert(!gext->catch[rcode]); gext->catch[rcode] = c; - - return 0; } static unlang_t *unlang_compile_catch(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx, CONF_ITEM const *ci) @@ -90,30 +84,28 @@ static unlang_t *unlang_compile_catch(unlang_t *parent, unlang_compile_ctx_t *un */ c->debug_name = c->name = talloc_typed_asprintf(c, "%s %s", cf_section_name1(cs), cf_section_name2(cs)); + /* + * catch { ... } has to be the last one, and will catch _all_ rcodes that weren't mentioned + * before. + */ if (!cf_section_name2(cs)) { - /* - * No arg2: catch errors - */ - gext->catch[RLM_MODULE_REJECT] = c; - gext->catch[RLM_MODULE_FAIL] = c; - gext->catch[RLM_MODULE_INVALID] = c; - gext->catch[RLM_MODULE_DISALLOW] = c; + int i; + + for (i = 0; i < RLM_MODULE_NUMCODES; i++) { + if (gext->catch[i]) continue; + + gext->catch[i] = c; + } } else { int i; name = cf_section_name2(cs); - if (catch_argv(cs, gext, name, c) < 0) { - talloc_free(c); - return NULL; - } + catch_argv(gext, name, c); for (i = 0; (name = cf_section_argv(cs, i)) != NULL; i++) { - if (catch_argv(cs, gext, name, c) < 0) { - talloc_free(c); - return NULL; - } + catch_argv(gext, name, c); } } diff --git a/src/lib/unlang/try.c b/src/lib/unlang/try.c index 4f47f0a45a5..3d60ced7fda 100644 --- a/src/lib/unlang/try.c +++ b/src/lib/unlang/try.c @@ -74,6 +74,10 @@ static unlang_t *unlang_compile_try(unlang_t *parent, unlang_compile_ctx_t *unla unlang_group_t *g; unlang_t *c; CONF_SECTION *parent_cs, *next; + int i; + CONF_SECTION *default_catch = NULL; + unlang_compile_ctx_t unlang_ctx2; + CONF_SECTION *catcher[RLM_MODULE_NUMCODES] = {}; /* * The transaction is empty, ignore it. @@ -105,7 +109,144 @@ static unlang_t *unlang_compile_try(unlang_t *parent, unlang_compile_ctx_t *unla c = unlang_group_to_generic(g); c->debug_name = c->name = cf_section_name1(cs); - return unlang_compile_children(g, unlang_ctx); + /* + * Whatever the input compile ctx is, we over-ride the default actions. + */ + unlang_compile_ctx_copy(&unlang_ctx2, unlang_ctx); + + /* + * Loop over all of the "catch" sections, figuring out which rcodes are being "catch"ed. + */ + for (default_catch = NULL; + next && (strcmp(cf_section_name1(next), "catch") == 0); + next = cf_section_next(parent_cs, next)) { + rlm_rcode_t rcode; + char const *name; + + /* + * catch { ... } is the default, and we can't + * have anything after it. + */ + if (default_catch) { + cf_log_err(default_catch, "Invalid 'catch' - cannot have another 'catch' after a default 'catch { ... }'"); + cs = default_catch; + print_catch_url: + cf_log_err(cs, DOC_KEYWORD_REF(catch)); + talloc_free(g); + return NULL; + } + + name = cf_section_name2(next); + if (!name) { + default_catch = next; + continue; + } + + rcode = fr_table_value_by_str(mod_rcode_table, name, RLM_MODULE_NOT_SET); + if (rcode == RLM_MODULE_NOT_SET) { + cf_log_err(cs, "Invalid argument to 'catch' - unknown rcode '%s'.", name); + goto print_catch_url; + } + + if (catcher[rcode]) { + cf_log_err(next, "Duplicate rcode '%s'", name); + cf_log_err(catcher[rcode], "First instance is here"); + goto print_catch_url; + } + catcher[rcode] = next; + + for (i = 0; (name = cf_section_argv(next, i)) != NULL; i++) { + rcode = fr_table_value_by_str(mod_rcode_table, name, RLM_MODULE_NOT_SET); + if (rcode == RLM_MODULE_NOT_SET) { + cf_log_err(cs, "Invalid argument to 'catch' - unknown rcode '%s'.", name); + goto print_catch_url; + } + + if (catcher[rcode]) { + cf_log_err(next, "Duplicate rcode '%s'", name); + cf_log_err(catcher[rcode], "First instance is here"); + goto print_catch_url; + } + + catcher[rcode] = next; + } + } + + /* + * Check that the default will be used. + * + * Note that we do NOT change the priorities for the defaults. + */ + if (default_catch) { + bool set = false; + + for (i = 0; i < RLM_MODULE_NUMCODES; i++) { + if (!catcher[i]) { + set = true; + break; + } + } + + if (!set) { + cf_log_err(default_catch, "Invalide 'catch { ... }' - all rcodes had previously been used"); + goto print_catch_url; + } + + /* + * Errors are still "return", even in a default "catch". + * + * Normal rcodes will run to the end of the try section, and then be "catch"ed. + */ + if (!catcher[RLM_MODULE_REJECT]) catcher[RLM_MODULE_REJECT] = default_catch; + if (!catcher[RLM_MODULE_FAIL]) catcher[RLM_MODULE_FAIL] = default_catch; + if (!catcher[RLM_MODULE_INVALID]) catcher[RLM_MODULE_INVALID] = default_catch; + if (!catcher[RLM_MODULE_DISALLOW]) catcher[RLM_MODULE_DISALLOW] = default_catch; + if (!catcher[RLM_MODULE_TIMEOUT]) catcher[RLM_MODULE_TIMEOUT] = default_catch; + } + + /* + * Loop again over the rcodes, setting the child actions to RETURN if necessary. + * + * If the child is returning for that action, ensure that _we_ aren't returning. + * + * Note that as above, reject / fail / invalid / disallow / timeout are errors, and cause the + * child to immediately return. All other rcodes + */ + for (i = 0; i < RLM_MODULE_NUMCODES; i++) { + /* + * No one cares about this rcode. It can return, reject, etc. + */ + if (!catcher[i]) continue; + + /* + * Error rcodes cause the child section to bail immediately, but the "try" instruction + * does not bail. + */ + if ((i == RLM_MODULE_REJECT) || + (i == RLM_MODULE_FAIL) || + (i == RLM_MODULE_INVALID) || + (i == RLM_MODULE_DISALLOW) || + (i == RLM_MODULE_TIMEOUT)) { + unlang_ctx2.actions.actions[i] = MOD_ACTION_RETURN; + c->actions.actions[i] = MOD_ACTION_NOT_SET; + continue; + } + + /* + * Normal rcodes cause the child section to run to completion, and the "try" section does + * not bail. + */ + if ((unlang_ctx2.actions.actions[i] > MOD_ACTION_NOT_SET) && + (unlang_ctx2.actions.actions[i] < MOD_PRIORITY_MIN)) { + unlang_ctx2.actions.actions[i] = MOD_ACTION_NOT_SET; + c->actions.actions[i] = MOD_ACTION_NOT_SET; + } + } + + /* + * Compile the children using the new compile ctx. + */ + return unlang_compile_children(g, &unlang_ctx2); }