minor tweaks to tmpl_afrom_attr_substr()

and disallow raw local attributes

diff --git a/src/lib/server/tmpl_tokenize.c b/src/lib/server/tmpl_tokenize.c
index c77fd0b4cd1d535e95730ed936b55147abc5d83c..75d05c98348b6e1e74fa2e84fd620a8f24b35383 100644 (file)
--- a/src/lib/server/tmpl_tokenize.c
+++ b/src/lib/server/tmpl_tokenize.c
@@ -2291,55 +2291,10 @@ ssize_t tmpl_afrom_attr_substr(TALLOC_CTX *ctx, tmpl_attr_error_t *err,
                                          &our_name, p_rules, at_rules, 0);
        if (ret < 0) goto error;
 
-       /*
-        *      Check to see if the user wants the leaf
-        *      attribute to be raw.
-        *
-        *      We can only do the conversion now _if_
-        *      the complete hierarchy has been resolved
-        *      otherwise we'll need to do the conversion
-        *      later.
-        */
-       if (tmpl_is_attr(vpt) && is_raw) {
-               ret = attr_to_raw(vpt, tmpl_attr_list_tail(tmpl_attr(vpt)));
-               if (ret < 0) goto error;
-       }
-
-       /*
-        *      Local variables cannot be given a list modifier.
-        */
-       if (tmpl_is_attr(vpt) && tmpl_attr_tail_da(vpt)) {
-               tmpl_attr_t     *ar = tmpl_attr_list_head(tmpl_attr(vpt));
-               bool            is_local = ar->ar_da->flags.local;
-
-               for (; ar != NULL;
-                    ar = tmpl_attr_list_next(tmpl_attr(vpt), ar)) {
-                       if (!ar->ar_da->flags.local ||
-                           (ar->ar_da->flags.local && is_local)) continue;
-
-                       fr_strerror_printf("Local attributes cannot be used in any list");
-                       if (err) *err = TMPL_ATTR_ERROR_FOREIGN_NOT_ALLOWED;
-                       fr_sbuff_set(&our_name, &m_l);
-                       goto error;
-               }
-
-               /*
-                *      That being said, local variables are named "foo", but are always put into the local list.
-                */
-               if (is_local && (at_rules->list_presence != TMPL_ATTR_LIST_FORBID)) {
-                       MEM(ar = talloc(vpt, tmpl_attr_t));
-                       *ar = (tmpl_attr_t){
-                               .ar_type = TMPL_ATTR_TYPE_NORMAL,
-                               .ar_da = request_attr_local,
-                               .ar_parent = fr_dict_root(fr_dict_internal())
-                       };
-
-                       /*
-                        *      Prepend the local list ref so it gets evaluated
-                        *      first.
-                        */
-                       tmpl_attr_list_insert_head(tmpl_attr(vpt), ar);
-               }
+       if (!tmpl_substr_terminal_check(&our_name, p_rules)) {
+               fr_strerror_const("Unexpected text after attribute reference");
+               if (err) *err = TMPL_ATTR_ERROR_MISSING_TERMINATOR;
+               goto error;
        }
 
        /*
@@ -2366,8 +2321,88 @@ ssize_t tmpl_afrom_attr_substr(TALLOC_CTX *ctx, tmpl_attr_error_t *err,
                break;
        }
 
+       tmpl_set_name(vpt, T_BARE_WORD, fr_sbuff_start(&our_name), fr_sbuff_used(&our_name));
+       vpt->rules = *t_rules;  /* Record the rules */
+
+       /*
+        *      Check to see if the user wants the leaf
+        *      attribute to be raw.
+        *
+        *      We can only do the conversion now _if_
+        *      the complete hierarchy has been resolved
+        *      otherwise we'll need to do the conversion
+        *      later.
+        */
+       if (tmpl_is_attr(vpt)) {
+               if (is_raw) {
+                       ret = attr_to_raw(vpt, tmpl_attr_list_tail(tmpl_attr(vpt)));
+                       if (ret < 0) goto error;
+               }
+
+               /*
+                *      Local variables cannot be given a list modifier.
+                */
+               if (tmpl_attr_tail_da(vpt)) {
+                       tmpl_attr_t     *ar = tmpl_attr_list_head(tmpl_attr(vpt));
+                       bool            is_local = ar->ar_da->flags.local;
+                       bool            allow_local = is_local;
+
+                       if (is_local && is_raw) {
+                               fr_strerror_printf("Local attributes cannot be 'raw'");
+                               if (err) *err = TMPL_ATTR_ERROR_UNKNOWN_NOT_ALLOWED;
+                               fr_sbuff_set(&our_name, &m_l);
+                               goto error;
+                       }
+
+                       /*
+                        *      We can transition from local to non-local, but not the other way around.
+                        */
+                       for (;
+                            ar != NULL;
+                            ar = tmpl_attr_list_next(tmpl_attr(vpt), ar)) {
+                               if (ar->ar_da->flags.local == allow_local) continue;
+
+                               if (!ar->ar_da->flags.local && allow_local) {
+                                       allow_local = false;
+                                       continue;
+                               }
+
+                               if (ar->ar_da->flags.local) {
+                                       fr_strerror_printf("Local attributes cannot be used in any list");
+                                       if (err) *err = TMPL_ATTR_ERROR_FOREIGN_NOT_ALLOWED;
+                                       fr_sbuff_set(&our_name, &m_l);
+                                       goto error;
+                               }
+                       }
+
+                       /*
+                        *      Local variables are named "foo", but are always put into the local list.
+                        *
+                        *      We add the list after checking for non-local -> local transition, as
+                        *      request_attr_local isn't a local attribute.
+                        *
+                        *      When the list is forbidden, we're creating a local attribute inside of a local
+                        *      TLV.
+                        */
+                       if (is_local && (at_rules->list_presence != TMPL_ATTR_LIST_FORBID)) {
+                               MEM(ar = talloc(vpt, tmpl_attr_t));
+                               *ar = (tmpl_attr_t){
+                                       .ar_type = TMPL_ATTR_TYPE_NORMAL,
+                                       .ar_da = request_attr_local,
+                                       .ar_parent = fr_dict_root(fr_dict_internal())
+                               };
+
+                               /*
+                                *      Prepend the local list ref so it gets evaluated
+                                *      first.
+                                */
+                               tmpl_attr_list_insert_head(tmpl_attr(vpt), ar);
+                       }
+               }
+       }
+
        /*
-        *      If we're using lists, ensure that the default list is specified.
+        *      Ensure that the default list is specified.
         */
        if (!tmpl_attr_is_list_attr(tmpl_attr_list_head(tmpl_attr(vpt)))) {
                tmpl_attr_t *ar;
@@ -2388,30 +2423,23 @@ ssize_t tmpl_afrom_attr_substr(TALLOC_CTX *ctx, tmpl_attr_error_t *err,
                tmpl_attr_list_insert_head(tmpl_attr(vpt), ar);
        }
 
-       tmpl_set_name(vpt, T_BARE_WORD, fr_sbuff_start(&our_name), fr_sbuff_used(&our_name));
-       vpt->rules = *t_rules;  /* Record the rules */
-
        /*
-        *      If there are actual requests, duplicate them
-        *      and move them into the list.
+        *      If there is a default request (parent, outer, etc.), duplicate them and move them into the
+        *      list.
         *
-        *      A NULL request_def pointer is equivalent to the
-        *      current request.
+        *      A NULL request_def pointer is equivalent to the current request.
         */
        if (t_rules->attr.request_def) {
                tmpl_request_ref_list_acopy(vpt, &vpt->rules.attr.request_def, t_rules->attr.request_def);
        }
 
+       /*
+        *      Now that all of the lists are set correctly, do some final validation and updates on the
+        *      attribute.
+        */
        if (tmpl_is_attr(vpt)) {
                tmpl_attr_t *ar;
 
-               /*
-                *      Suppress useless casts.
-                */
-               if (tmpl_attr_tail_is_normal(vpt) && (tmpl_attr_tail_da(vpt)->type == tmpl_rules_cast(vpt))) {
-                       vpt->rules.cast = FR_TYPE_NULL;
-               }
-
                /*
                 *      Ensure that the list is set correctly, so that
                 *      the returned vpt just doesn't just match the
@@ -2421,25 +2449,27 @@ ssize_t tmpl_afrom_attr_substr(TALLOC_CTX *ctx, tmpl_attr_error_t *err,
                fr_assert(ar != NULL);
 
                if (tmpl_attr_is_list_attr(ar)) vpt->rules.attr.list_def = ar->ar_da;
-       }
 
-       if (!tmpl_substr_terminal_check(&our_name, p_rules)) {
-               fr_strerror_const("Unexpected text after attribute reference");
-               if (err) *err = TMPL_ATTR_ERROR_MISSING_TERMINATOR;
-               goto error;
-       }
+               if (tmpl_attr_tail_is_normal(vpt)) {
+                       /*
+                        *      Suppress useless casts.
+                        */
+                       if (tmpl_attr_tail_da(vpt)->type == tmpl_rules_cast(vpt)) {
+                               vpt->rules.cast = FR_TYPE_NULL;
+                       }
 
-       /*
-        *      If everything was resolved correctly
-        *      we now need to check the cast type.
-        */
-       if (!tmpl_needs_resolving(vpt) && !fr_type_is_null(t_rules->cast) &&
-           !fr_type_cast(t_rules->cast, tmpl_attr_tail_da(vpt)->type)) {
-               fr_strerror_printf("Cannot cast type '%s' to '%s'",
-                                  fr_type_to_str(tmpl_attr_tail_da(vpt)->type), fr_type_to_str(t_rules->cast));
-               if (err) *err = TMPL_ATTR_ERROR_BAD_CAST;
-               fr_sbuff_set_to_start(&our_name);
-               goto error;
+                       /*
+                        *      Check if the cast is allowed.  This lets us give better errors at compile time.
+                        */
+                       if ((tmpl_rules_cast(vpt)!= FR_TYPE_NULL) &&
+                           !fr_type_cast(tmpl_rules_cast(vpt), tmpl_attr_tail_da(vpt)->type)) {
+                               fr_strerror_printf("Cannot cast type '%s' to '%s'",
+                                          fr_type_to_str(tmpl_attr_tail_da(vpt)->type), fr_type_to_str(t_rules->cast));
+                               if (err) *err = TMPL_ATTR_ERROR_BAD_CAST;
+                               fr_sbuff_set_to_start(&our_name);
+                               goto error;
+                       }
+               }
        }
 
        TMPL_VERIFY(vpt);       /* Because we want to ensure we produced something sane */

diff --git a/src/lib/util/dict_unknown.c b/src/lib/util/dict_unknown.c
index 744ae7d3a5238c1e65631dcd3ce9f095c9fa2681..06124fe02b3d45e7e2f7a4508bda0b11c07dbe4f 100644 (file)
--- a/src/lib/util/dict_unknown.c
+++ b/src/lib/util/dict_unknown.c
@@ -28,6 +28,12 @@ static int dict_attr_unknown_init(fr_dict_attr_t const *parent, UNUSED fr_dict_a
 {
        flags->is_unknown = true;
 
+       if (parent->flags.local) {
+               fr_strerror_printf("Cannot create 'raw' attribute of data type '%s' which is a local variable",
+                                  fr_type_to_str(type));
+               return -1;
+       }
+
        if (parent->flags.internal) {
                fr_strerror_printf("Cannot create 'raw' attribute of data type '%s' which is 'internal'",
                                   fr_type_to_str(type));

diff --git a/src/lib/util/dict_validate.c b/src/lib/util/dict_validate.c
index 2b0443724818f20cb60fd6bc186789e5468eca08..2268c0bdb4a8b4acdb0868fb01feb2ebad6234bb 100644 (file)
--- a/src/lib/util/dict_validate.c
+++ b/src/lib/util/dict_validate.c
@@ -97,6 +97,11 @@ bool dict_attr_flags_valid(fr_dict_attr_t *da)
                return false;
        }
 
+       if (flags->local && (flags->is_unknown || flags->is_raw)) {
+               fr_strerror_const("Local variables cannot be 'raw' or unknown");
+               return false;
+       }
+
        /*
         *      Only some data types can be in arrays, because we need
         *      to be able to decode the various array members.

diff --git a/src/tests/keywords/local-raw-error b/src/tests/keywords/local-raw-error
new file mode 100644 (file)
index 0000000..69ef25e
--- /dev/null
+++ b/src/tests/keywords/local-raw-error
@@ -0,0 +1,4 @@
+uint32 foo
+octets bar
+
+bar := raw.foo # ERROR