don't let the user delete attributes we're looping over

this isn't perfect, but it's good enough.

It can still crash if the user has one "foreach" loop nested
inside of another one, AND both loop over the same attributes,
AND a variable is deleted after the inner "foreach" loop is run.

The general answer then is "don't do that".

We could fix this by using the old way of copying all of the VPs,
looping over the copy, and then moving the copy back in place of
the original VPs.  But that process is expensive, and almost always
not needed.

For now, it's simpler to just say "don't modify the attributes
you're looping over"

diff --git a/src/lib/unlang/foreach.c b/src/lib/unlang/foreach.c
index 623a63bab6ea8a8f3a8033ee778caa0d171fb1f8..8f42d6e603eb7682a5cc5ad8c9c07c14171f9b27 100644 (file)
--- a/src/lib/unlang/foreach.c
+++ b/src/lib/unlang/foreach.c
@@ -52,6 +52,7 @@ typedef struct {
        request_t               *request;                       //!< The current request.
        fr_dcursor_t            cursor;                         //!< Used to track our place in the list
        fr_pair_t               *key;                           //!< local variable which contains the key
+       tmpl_t const            *vpt;                           //!< pointer to the vpt
 
        tmpl_dcursor_ctx_t      cc;                             //!< tmpl cursor state
 
@@ -76,7 +77,22 @@ static xlat_action_t unlang_foreach_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
 static int _free_unlang_frame_state_foreach(unlang_frame_state_foreach_t *state)
 {
        if (state->key) {
+               fr_pair_t *vp;
+
+               tmpl_dcursor_clear(&state->cc);
+
+               /*
+                *      Now that we're done, the leaf entries can be changed again.
+                */
+               vp = tmpl_dcursor_init(NULL, NULL, &state->cc, &state->cursor, state->request, state->vpt);
+               fr_assert(vp != NULL);
+
+               do {
+                       if (fr_type_is_leaf(vp->vp_type)) fr_pair_clear_immutable(vp);
+               
+               } while ((vp = fr_dcursor_next(&state->cursor)) != NULL);
                tmpl_dcursor_clear(&state->cc);
+
        } else {
                request_data_get(state->request, FOREACH_REQUEST_DATA, state->depth);
        }
@@ -138,6 +154,11 @@ static unlang_action_t unlang_foreach_next(rlm_rcode_t *p_result, request_t *req
        vp = fr_dcursor_current(&state->cursor);
        fr_assert(vp != NULL);
 
+       /*
+        *      If we modified the value, copy it back to the original pair.  Note that the copy does NOT
+        *      check the "immutable" flag.  That flag is for the people using unlang, not for the
+        *      interpreter.
+        */
        if (!fr_type_is_structural(vp->vp_type) && (vp->vp_type == state->key->vp_type)) {
                fr_value_box_clear_value(&vp->data);
                (void) fr_value_box_copy(vp, &vp->data, &state->key->data);
@@ -213,6 +234,8 @@ static unlang_action_t unlang_foreach(rlm_rcode_t *p_result, request_t *request,
         *      We have a key variable, let's use that.
         */
        if (gext->key) {
+               state->vpt = gext->vpt;
+
                /*
                 *      No matching attributes, we can't do anything.
                 */
@@ -222,6 +245,19 @@ static unlang_action_t unlang_foreach(rlm_rcode_t *p_result, request_t *request,
                        return UNLANG_ACTION_CALCULATE_RESULT;
                }
 
+               /*
+                *      Before we loop over the variables, ensure that the user can't pull the rug out from
+                *      under us.
+                */
+               do {
+                       if (fr_type_is_leaf(vp->vp_type)) fr_pair_set_immutable(vp);
+               
+               } while ((vp = fr_dcursor_next(&state->cursor)) != NULL);
+               tmpl_dcursor_clear(&state->cc);
+
+               vp = tmpl_dcursor_init(NULL, NULL, &state->cc, &state->cursor, request, gext->vpt);
+               fr_assert(vp != NULL);
+
                /*
                 *      Create the local variable and populate its value.
                 */

diff --git a/src/lib/util/pair.h b/src/lib/util/pair.h
index 0a9e98d212b120b5d4dff46598c298d100b99a83..776799edd49810829715e9ca59e87b53a29bef24 100644 (file)
--- a/src/lib/util/pair.h
+++ b/src/lib/util/pair.h
@@ -686,9 +686,19 @@ bool               fr_pair_immutable(fr_pair_t const *vp) CC_HINT(nonnull);
 static inline CC_HINT(nonnull, always_inline)
 void fr_pair_set_immutable(fr_pair_t *vp)
 {
+       fr_assert(fr_type_is_leaf(vp->vp_type));
+
        fr_value_box_set_immutable(&vp->data);
 }
 
+static inline CC_HINT(nonnull, always_inline)
+void fr_pair_clear_immutable(fr_pair_t *vp)
+{
+       fr_assert(fr_type_is_leaf(vp->vp_type));
+
+       fr_value_box_clear_immutable(&vp->data);
+}
+
 
 /* Lists */
 int            fr_pair_list_copy(TALLOC_CTX *ctx, fr_pair_list_t *to, fr_pair_list_t const *from);

diff --git a/src/lib/util/value.h b/src/lib/util/value.h
index d2901a73352c08c391af3ad99bc7ff888b276ada..26e9c64673d2098b3fcc47438e4dadc45c32e76c 100644 (file)
--- a/src/lib/util/value.h
+++ b/src/lib/util/value.h
@@ -1075,6 +1075,12 @@ void fr_value_box_set_immutable(fr_value_box_t *box)
        box->immutable = true;
 }
 
+static inline CC_HINT(nonnull, always_inline)
+void fr_value_box_clear_immutable(fr_value_box_t *box)
+{
+       box->immutable = false;
+}
+
 
 /** @name Assign and manipulate binary-unsafe C strings
  *

diff --git a/src/tests/keywords/foreach-delete b/src/tests/keywords/foreach-delete
new file mode 100644 (file)
index 0000000..fec3644
--- /dev/null
+++ b/src/tests/keywords/foreach-delete
@@ -0,0 +1,32 @@
+#
+#  PRE: foreach-modify
+#
+
+&Tmp-Integer-0 := { 1, 3, 5, 11 }
+
+#
+#  Try to delete one of the variables we're looping over.  If the user could delete one,
+#  then the underlying tmpl_dcursor would crash, as it didn't know about the deletion.
+#
+foreach uint32 self (&Tmp-Integer-0) {
+       &request -= &Tmp-Integer-0[1]
+       &self += 19
+}
+
+if (&Tmp-Integer-0[0] != 20) {
+       test_fail
+}
+
+if (&Tmp-Integer-0[1] != 22) {
+       test_fail
+}
+
+if (&Tmp-Integer-0[2] != 24) {
+       test_fail
+}
+
+if (&Tmp-Integer-0[3] != 30) {
+       test_fail
+}
+
+success