Add multiple types of variadic argument

Fix redis xlats to keep argument order and not crash when empty values are provided

diff --git a/src/lib/unlang/xlat.h b/src/lib/unlang/xlat.h
index 28a564e6eff24a3cfcbe7003c8fbbbafc7607f50..0077ae5b39b4b923c9de607bc432b89aaa7d529e 100644 (file)
--- a/src/lib/unlang/xlat.h
+++ b/src/lib/unlang/xlat.h
@@ -128,20 +128,28 @@ extern size_t xlat_action_table_len;
  */
 typedef int (*xlat_escape_func_t)(request_t *request, fr_value_box_t *vb, void *uctx);
 
+typedef enum {
+       XLAT_ARG_VARIADIC_DISABLED      = 0,
+       XLAT_ARG_VARIADIC_EMPTY_SQUASH  = 1,    //!< Empty argument groups are removed.
+       XLAT_ARG_VARIADIC_EMPTY_KEEP    = 2,    //!< Empty argument groups are left alone,
+                                               ///< and either passed through as empty groups
+                                               ///< or null boxes.
+} xlat_arg_parser_variadic_t;
+
 /** Definition for a single argument consumend by an xlat function
  *
  */
 typedef struct {
-       bool                    required;       //!< Argument must be present.
-       bool                    concat;         //!< Concat boxes together.
-       bool                    single;         //!< Argument must only contain a single box
-       bool                    variadic;       //!< All additional boxes should be processed
-                                               ///< using this definition.
-       bool                    always_escape;  //!< Pass all arguments to escape function not just
-                                               ///< tainted ones.
-       fr_type_t               type;           //!< Type to cast argument to.
-       xlat_escape_func_t      func;           //!< Function to handle tainted values.
-       void                    *uctx;          //!< Argument to pass to escape callback.
+       bool                            required;       //!< Argument must be present, and non-empty.
+       bool                            concat;         //!< Concat boxes together.
+       bool                            single;         //!< Argument must only contain a single box
+       xlat_arg_parser_variadic_t      variadic;       //!< All additional boxes should be processed
+                                                       ///< using this definition.
+       bool                            always_escape;  //!< Pass all arguments to escape function not just
+                                                       ///< tainted ones.
+       fr_type_t                       type;           //!< Type to cast argument to.
+       xlat_escape_func_t              func;           //!< Function to handle tainted values.
+       void                            *uctx;          //!< Argument to pass to escape callback.
 } xlat_arg_parser_t;
 
 typedef struct {

diff --git a/src/lib/unlang/xlat_builtin.c b/src/lib/unlang/xlat_builtin.c
index 213e175aa62c0d9b237d48cfb3c9795d4b57b9af..d9e909e234b04d6b41afd7390c6c36fe76a06710 100644 (file)
--- a/src/lib/unlang/xlat_builtin.c
+++ b/src/lib/unlang/xlat_builtin.c
@@ -1374,7 +1374,8 @@ finish:
 
 static xlat_arg_parser_t const xlat_func_cast_args[] = {
        { .required = true, .single = true, .type = FR_TYPE_VOID },
-       { .required = true, .variadic = true, .type = FR_TYPE_VOID },
+       { .required = true, .type = FR_TYPE_VOID },
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .type = FR_TYPE_VOID },
        XLAT_ARG_PARSER_TERMINATOR
 };
 
@@ -1645,27 +1646,26 @@ static xlat_action_t xlat_func_hmac_sha1(TALLOC_CTX *ctx, fr_dcursor_t *out,
 }
 
 static xlat_arg_parser_t const xlat_func_join_args[] = {
-       { .required = true, .variadic = true, .type = FR_TYPE_VOID },
+       { .required = true, .type = FR_TYPE_VOID },
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_SQUASH, .type = FR_TYPE_VOID },
        XLAT_ARG_PARSER_TERMINATOR
 };
 
 /** Join a series of arguments to form a single list
  *
+ * null boxes are not preserved.
  */
 static xlat_action_t xlat_func_join(UNUSED TALLOC_CTX *ctx, fr_dcursor_t *out,
                                    UNUSED xlat_ctx_t const *xctx,
                                    UNUSED request_t *request, fr_value_box_list_t *in)
 {
-       fr_value_box_t  *arg = NULL, *vb, *p;
-
-       while ((arg = fr_value_box_list_next(in, arg))) {
+       fr_value_box_list_foreach(in, arg) {
                fr_assert(arg->type == FR_TYPE_GROUP);
-               vb = fr_value_box_list_head(&arg->vb_group);
-               while (vb) {
-                       p = fr_value_box_list_remove(&arg->vb_group, vb);
+
+               fr_value_box_list_foreach_safe(&arg->vb_group, vb) {
+                       fr_value_box_list_remove(&arg->vb_group, vb);
                        fr_dcursor_append(out, vb);
-                       vb = fr_value_box_list_next(&arg->vb_group, p);
-               }
+               }}
        }
        return XLAT_ACTION_DONE;
 }
@@ -1701,11 +1701,13 @@ static xlat_action_t xlat_func_ungroup(UNUSED TALLOC_CTX *ctx, fr_dcursor_t *out
 }
 
 static xlat_arg_parser_t const xlat_func_length_args[] = {
-       { .single = true, .variadic = true, .type = FR_TYPE_VOID },
+       { .single = true, .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .type = FR_TYPE_VOID },
        XLAT_ARG_PARSER_TERMINATOR
 };
 
 /** Return the on-the-wire size of the boxes in bytes
+ *
+ * skips null values
  *
  * Example:
 @verbatim
@@ -1722,13 +1724,11 @@ static xlat_action_t xlat_func_length(TALLOC_CTX *ctx, fr_dcursor_t *out,
                                      UNUSED request_t *request, fr_value_box_list_t *in)
 
 {
-       fr_value_box_t  *vb = NULL;
-
-       while ((vb = fr_value_box_list_next(in, vb))) {
+       fr_value_box_list_foreach(in, vb) {
                fr_value_box_t *my;
 
                MEM(my = fr_value_box_alloc(ctx, FR_TYPE_SIZE, NULL, false));
-               my->vb_size = fr_value_box_network_length(vb);
+               if (!fr_type_is_null(vb->type)) my->vb_size = fr_value_box_network_length(vb);
                fr_dcursor_append(out, my);
        }
 
@@ -3001,7 +3001,7 @@ static xlat_action_t xlat_func_urlunquote(TALLOC_CTX *ctx, fr_dcursor_t *out,
 }
 
 static xlat_arg_parser_t const protocol_decode_xlat_args[] = {
-       { .single = true, .variadic = true, .type = FR_TYPE_VOID },
+       { .single = true, .variadic = XLAT_ARG_VARIADIC_EMPTY_SQUASH, .type = FR_TYPE_VOID },
        XLAT_ARG_PARSER_TERMINATOR
 };
 

diff --git a/src/lib/unlang/xlat_eval.c b/src/lib/unlang/xlat_eval.c
index cd04eaf2dc5e7528ffa59317d6e8026db2eab845..77d3b211c0b7254ec183606baccf3a60835f44b5 100644 (file)
--- a/src/lib/unlang/xlat_eval.c
+++ b/src/lib/unlang/xlat_eval.c
@@ -32,6 +32,7 @@ RCSID("$Id$")
 #include <freeradius-devel/util/debug.h>
 #include <freeradius-devel/util/types.h>
 #include <freeradius-devel/util/sbuff.h>
+#include <freeradius-devel/util/value.h>
 
 #include <freeradius-devel/unlang/unlang_priv.h>       /* Remove when everything uses new xlat API */
 
@@ -375,7 +376,6 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list,
         */
        if (!func->args) return XLAT_ACTION_DONE;
 
-
        /*
         *      xlat needs no input processing just return.
         */
@@ -430,12 +430,25 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list,
                         *      This argument doesn't exist.  That might be OK, or it may be a fatal error.
                         */
                        if (fr_value_box_list_empty(&vb->vb_group)) {
+                               /*
+                                *      Variadic rules deal with empty boxes differently...
+                                */
+                               switch (arg_p->variadic) {
+                               case XLAT_ARG_VARIADIC_EMPTY_SQUASH:
+                                       fr_value_box_list_talloc_free_head(list);
+                                       continue;
+
+                               case XLAT_ARG_VARIADIC_EMPTY_KEEP:
+                                       goto empty_ok;
+
+                               case XLAT_ARG_VARIADIC_DISABLED:
+                                       break;
+                               }
+
                                /*
                                 *      Empty groups for optional arguments are OK, we can just stop processing the list.
                                 */
                                if (!arg_p->required) {
-                                       fr_assert(!next);
-
                                        /*
                                         *      If the caller doesn't care about the type, then we leave the
                                         *      empty group there.
@@ -455,8 +468,7 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list,
                                         *      accessing the box as vb_uint8, hoping that it's being passed
                                         *      the right thing.
                                         */
-                                       fr_value_box_list_remove(list, vb);
-                                       talloc_free(vb);
+                                       fr_value_box_list_talloc_free_head(list);
                                        break;
                                }
 
@@ -470,13 +482,23 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list,
                                if (arg_p->type != FR_TYPE_VOID) goto missing;
                        }
 
+               empty_ok:
                        /*
                         *      In some cases we replace the current argument with the head of the group.
                         *
                         *      xlat_process_arg_list() has already done concatenations for us.
                         */
                        if (arg_p->single || arg_p->concat) {
-                               fr_value_box_list_replace(list, vb, fr_value_box_list_pop_head(&vb->vb_group));
+                               fr_value_box_t *head = fr_value_box_list_pop_head(&vb->vb_group);
+
+                               /*
+                                *      If we're meant to be smashing the argument
+                                *      to a single box, but the group was empty,
+                                *      add a null box instead so ordering is maintained
+                                *      for subsequent boxes.
+                                */
+                               if (!head) head = fr_value_box_alloc_null(ctx);
+                               fr_value_box_list_replace(list, vb, head);
                                talloc_free(vb);
                        }
 

diff --git a/src/lib/unlang/xlat_pair.c b/src/lib/unlang/xlat_pair.c
index 023c8f2226c42738a754cdb2bc66d9698d1caf81..da3a48948135cca202e585d5cffad878447c8b2e 100644 (file)
--- a/src/lib/unlang/xlat_pair.c
+++ b/src/lib/unlang/xlat_pair.c
@@ -92,14 +92,13 @@ int xlat_decode_value_box_list(TALLOC_CTX *ctx, fr_pair_list_t *out,
                               fr_value_box_list_t *in)
 {
        int             decoded = 0;
-       fr_value_box_t  *vb = NULL;
        fr_pair_t       *vp = NULL;
        fr_dict_attr_t const *parent = fr_dict_root(request->dict);
        fr_pair_list_t  head;
 
        fr_pair_list_init(&head);
 
-       while ((vb = fr_value_box_list_next(in, vb))) {
+       fr_value_box_list_foreach(in, vb) {
                ssize_t         len;
 
                if (vb->type != FR_TYPE_OCTETS) {

diff --git a/src/modules/rlm_cipher/rlm_cipher.c b/src/modules/rlm_cipher/rlm_cipher.c
index 799fddd2bd1a57541dcb068ed6d1f6ad0cfa2a1b..8729bf1e412a1c29d7501c1cda01eaf90fa630ca 100644 (file)
--- a/src/modules/rlm_cipher/rlm_cipher.c
+++ b/src/modules/rlm_cipher/rlm_cipher.c
@@ -35,6 +35,7 @@ RCSID("$Id$")
 #include <freeradius-devel/tls/strerror.h>
 #include <freeradius-devel/util/debug.h>
 #include <freeradius-devel/unlang/xlat_func.h>
+#include <freeradius-devel/unlang/xlat.h>
 
 #include <freeradius-devel/tls/openssl_user_macros.h>
 #include <openssl/crypto.h>
@@ -763,9 +764,9 @@ static xlat_action_t cipher_rsa_decrypt_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
 }
 
 static xlat_arg_parser_t const cipher_rsa_verify_xlat_arg[] = {
-       { .required = true, .concat = true, .single = false, .variadic = true, .type = FR_TYPE_STRING,
-         .func = NULL, .uctx = NULL },
        { .required = true, .concat = false, .single = true, .type = FR_TYPE_VOID },
+       { .required = true, .concat = true, .type = FR_TYPE_STRING },
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_SQUASH, .concat = true,  .type = FR_TYPE_STRING },
        XLAT_ARG_PARSER_TERMINATOR
 };
 

diff --git a/src/modules/rlm_exec/rlm_exec.c b/src/modules/rlm_exec/rlm_exec.c
index b888e76cbd89e64caab4d0094d41a0804d2777d0..4684f26f466e84eec5a20dba52da455fb750e240 100644 (file)
--- a/src/modules/rlm_exec/rlm_exec.c
+++ b/src/modules/rlm_exec/rlm_exec.c
@@ -101,7 +101,7 @@ static xlat_action_t exec_xlat_oneshot_wait_resume(TALLOC_CTX *ctx, fr_dcursor_t
 
 static xlat_arg_parser_t const exec_xlat_args[] = {
        { .required = true, .type = FR_TYPE_STRING },
-       { .variadic = true, .type = FR_TYPE_VOID},
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .type = FR_TYPE_VOID},
        XLAT_ARG_PARSER_TERMINATOR
 };
 

diff --git a/src/modules/rlm_perl/rlm_perl.c b/src/modules/rlm_perl/rlm_perl.c
index ddd999c009433a077b8a50878b8f68333d258c4f..a1f1f8b33b6642e7cf9a364ff99c607d66d95b2d 100644 (file)
--- a/src/modules/rlm_perl/rlm_perl.c
+++ b/src/modules/rlm_perl/rlm_perl.c
@@ -30,6 +30,7 @@ RCSID("$Id$")
 #include <freeradius-devel/server/module_rlm.h>
 #include <freeradius-devel/util/debug.h>
 #include <freeradius-devel/unlang/xlat_func.h>
+#include <freeradius-devel/unlang/xlat.h>
 #include <freeradius-devel/radius/radius.h>
 
 DIAG_OFF(DIAG_UNKNOWN_PRAGMAS)
@@ -412,7 +413,7 @@ static int perl_sv_to_vblist(TALLOC_CTX *ctx, fr_value_box_list_t *list, request
 
 static xlat_arg_parser_t const perl_xlat_args[] = {
        { .required = true, .single = true, .type = FR_TYPE_STRING },
-       { .variadic = true, .type = FR_TYPE_VOID },
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .type = FR_TYPE_VOID },
        XLAT_ARG_PARSER_TERMINATOR
 };
 
@@ -429,7 +430,7 @@ static xlat_action_t perl_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
        xlat_action_t                   ret = XLAT_ACTION_FAIL;
        STRLEN                          n_a;
        fr_value_box_t                  *func = fr_value_box_list_pop_head(in);
-       fr_value_box_t                  *arg = NULL, *child;
+       fr_value_box_t                  *child;
        SV                              *sv;
        AV                              *av;
        fr_value_box_list_t             list, sub_list;
@@ -449,7 +450,7 @@ static xlat_action_t perl_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
 
                PUSHMARK(SP);
 
-               while ((arg = fr_value_box_list_next(in, arg))) {
+               fr_value_box_list_foreach(in, arg) {
                        fr_assert(arg->type == FR_TYPE_GROUP);
                        if (fr_value_box_list_empty(&arg->vb_group)) continue;
 

diff --git a/src/modules/rlm_radius/rlm_radius_udp.c b/src/modules/rlm_radius/rlm_radius_udp.c
index 41630f3de1f220dbfc8c9c9a440e7a269b6a2ffb..573c2430cb7b877b1e3389da6606d23c2568a4b3 100644 (file)
--- a/src/modules/rlm_radius/rlm_radius_udp.c
+++ b/src/modules/rlm_radius/rlm_radius_udp.c
@@ -1578,7 +1578,6 @@ static bool check_for_zombie(fr_event_list_t *el, fr_trunk_connection_t *tconn,
 
        /*
         *      If we're status checking OR already zombie, don't go to zombie
-        *
         */
        if (h->status_checking || h->zombie_ev) return true;
 
@@ -1595,13 +1594,7 @@ static bool check_for_zombie(fr_event_list_t *el, fr_trunk_connection_t *tconn,
        if (h->inst->parent->synchronous && fr_time_gt(last_sent, fr_time_wrap(0)) &&
            (fr_time_lt(fr_time_add(last_sent, h->inst->parent->response_window), now))) return false;
 
-       /*
-        *      Mark the connection as inactive, but keep sending
-        *      packets on it.
-        */
        WARN("%s - Entering Zombie state - connection %s", h->module_name, h->name);
-       fr_trunk_connection_signal_inactive(tconn);
-
        if (h->inst->parent->status_check) {
                h->status_checking = true;
 

diff --git a/src/modules/rlm_redis/rlm_redis.c b/src/modules/rlm_redis/rlm_redis.c
index 222e7d5be9592e26ae6cf3fb6afe3f07ebfbef6a..e443bb65af23ae5e846faa98d682c5e11b8b0420 100644 (file)
--- a/src/modules/rlm_redis/rlm_redis.c
+++ b/src/modules/rlm_redis/rlm_redis.c
@@ -351,7 +351,7 @@ static xlat_action_t redis_node_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
 
 static xlat_arg_parser_t const redis_lua_func_args[] = {
        { .required = true, .single = true, .type = FR_TYPE_UINT64 }, /* key count */
-       { .variadic = true, .concat = true, .type = FR_TYPE_STRING }, /* keys and args */
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .concat = true, .type = FR_TYPE_STRING }, /* keys and args */
        XLAT_ARG_PARSER_TERMINATOR
 };
 
@@ -411,9 +411,20 @@ static xlat_action_t redis_lua_func_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
                        REXDENT();
                        return XLAT_ACTION_FAIL;
                }
+
+               /*
+                *      Fixup null or empty arguments to be
+                *      zero length strings so that the position
+                *      of subsequent arguments are maintained.
+                */
+               if (!fr_type_is_string(vb->type)) {
+                       argv[argc] = "";
+                       arg_len[argc++] = 0;
+                       continue;
+               }
+
                argv[argc] = vb->vb_strvalue;
-               arg_len[argc] = vb->vb_length;
-               argc++;
+               arg_len[argc++] = vb->vb_length;
        }
 
        /*
@@ -551,14 +562,15 @@ static int redis_lua_func_instantiate(xlat_inst_ctx_t const *xctx)
 }
 
 static xlat_arg_parser_t const redis_args[] = {
-       { .required = true, .variadic = true, .concat = true, .type = FR_TYPE_STRING },
+       { .required = true, .concat = true, .type = FR_TYPE_STRING },
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .concat = true, .type = FR_TYPE_STRING },
        XLAT_ARG_PARSER_TERMINATOR
 };
 
 /** Xlat to make calls to redis
  *
 @verbatim
-%{redis:<redis command>}
+%(redis:<redis command>)
 @endverbatim
  *
  * @ingroup xlat_functions
@@ -628,9 +640,19 @@ static xlat_action_t redis_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
                                goto fail;
                        }
 
+                       /*
+                        *      Fixup null or empty arguments to be
+                        *      zero length strings so that the position
+                        *      of subsequent arguments are maintained.
+                        */
+                       if (!fr_type_is_string(vb->type)) {
+                               argv[argc] = "";
+                               arg_len[argc++] = 0;
+                               continue;
+                       }
+
                        argv[argc] = vb->vb_strvalue;
-                       arg_len[argc] = vb->vb_length;
-                       argc++;
+                       arg_len[argc++] = vb->vb_length;
                }
 
                RDEBUG2("Executing command: %pV", fr_value_box_list_head(in));

diff --git a/src/modules/rlm_rest/rlm_rest.c b/src/modules/rlm_rest/rlm_rest.c
index 360fec27355287d037ea6feb2796ef2cdc53e02e..13cbdd63b96487a7885843d23fc36fff3676403c 100644 (file)
--- a/src/modules/rlm_rest/rlm_rest.c
+++ b/src/modules/rlm_rest/rlm_rest.c
@@ -386,7 +386,7 @@ static fr_uri_part_t const rest_uri_parts[] = {
 };
 
 static xlat_arg_parser_t const rest_xlat_args[] = {
-       { .required = true, .variadic = true, .type = FR_TYPE_STRING },
+       { .required = true, .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .type = FR_TYPE_STRING },
        XLAT_ARG_PARSER_TERMINATOR
 };
 

diff --git a/src/modules/rlm_test/rlm_test.c b/src/modules/rlm_test/rlm_test.c
index d26cb8e0318f46b86e4bf33a39ca2e34939aae93..210f31cbefb0627f202b14df4ade106f855c80a8 100644 (file)
--- a/src/modules/rlm_test/rlm_test.c
+++ b/src/modules/rlm_test/rlm_test.c
@@ -380,7 +380,8 @@ static xlat_action_t trigger_test_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
 
 
 static xlat_arg_parser_t const test_xlat_args[] = {
-       { .required = true, .concat = true, .variadic = true, .type = FR_TYPE_STRING },
+       { .required = true, .concat = true, .type = FR_TYPE_STRING },
+       { .variadic = XLAT_ARG_VARIADIC_EMPTY_KEEP, .concat = true, .type = FR_TYPE_STRING },
        XLAT_ARG_PARSER_TERMINATOR
 };
 
@@ -393,10 +394,9 @@ static xlat_action_t test_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
                               UNUSED xlat_ctx_t const *xctx, UNUSED request_t *request,
                               fr_value_box_list_t *in)
 {
-       fr_value_box_t  *vb_p = NULL;
        fr_value_box_t  *vb;
 
-       while ((vb_p = fr_value_box_list_next(in, vb_p))) {
+       fr_value_box_list_foreach(in, vb_p) {
                MEM(vb = fr_value_box_alloc(ctx, FR_TYPE_STRING, NULL, false));
 
                if (fr_value_box_copy(ctx, vb, vb_p) < 0) {

diff --git a/src/tests/modules/redis/functions.unlang b/src/tests/modules/redis/functions.unlang
index a21f37ee347e47ce71921ed9b2b5a499a2cedf71..6b42018431393a1666b9d47ee827eb05cb7033d3 100644 (file)
--- a/src/tests/modules/redis/functions.unlang
+++ b/src/tests/modules/redis/functions.unlang
@@ -15,7 +15,7 @@ if (!(%(redis.hello_world:0) == 'hello world')) {
 # ...and again using an argument that would produce a null result
 # this is a regression test where the arg parser would require all
 # arguments to be non-null if the first argument was
-if (!(%(redis.hello_world:0 %{Framed-IP-Address}) == 'hello world')) {
+if (!(%(redis.hello_world:0 %{Tmp-String-9}) == 'hello world')) {
        test_fail
 }
 
@@ -23,6 +23,11 @@ if (!(%(redis.concat_args_keys:1 foo bar baz) == 'foo,bar,baz')) {
        test_fail
 }
 
+# Concat with an empty argument.  This is a regression test
+if (!(%(redis.concat_args_keys:1 foo %{Tmp-String-9} baz) == 'foo,,baz')) {
+       test_fail
+}
+
 if (!(%(redis.multiline:0 0) == 0)) {
        test_fail
 }