push checks for vp_edit and immutable down to edit list

where they are caught more often.

and update test.  it's a failure to delete an attribute which
can't be deleted.  So the attempt has to be in a try / catch
block

diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c
index dccefced32820311bb0f271ff7f1812e2b8d01dc..d87453fe81104eb0a804b664a51627df47fab522 100644 (file)
--- a/src/lib/unlang/edit.c
+++ b/src/lib/unlang/edit.c
@@ -390,11 +390,12 @@ static int apply_edits_to_list(request_t *request, unlang_frame_state_edit_t *st
                        }
 
                        if (vp->vp_edit) {
-                               RWDEBUG("Attribute cannot be removed, as it is being used in a 'foreach' loop - %pP", vp);
-                               continue;
+                               REDEBUG("Attribute cannot be removed, as it is being used in a 'foreach' loop - %pP", vp);
+                               return -1;
                        }
 
                        if (fr_edit_list_pair_delete(current->el, list, vp) < 0) {
+                               RPEDEBUG("Failed deleting attribute");
                                tmpl_dcursor_clear(&cc);
                                return -1;
                        }
@@ -593,7 +594,10 @@ static int edit_delete_lhs(request_t *request, edit_map_t *current, bool delete)
                /*
                 *      Delete all of them.  We'll create one later for the SET operation.
                 */
-               if (fr_edit_list_pair_delete(current->el, &parent->vp_group, vp) < 0) return -1;
+               if (fr_edit_list_pair_delete(current->el, &parent->vp_group, vp) < 0) {
+                       RPWDEBUG("Failed deleting attribute");
+                       return -1;
+               }
                tmpl_dcursor_clear(&cc);
        }
 

diff --git a/src/lib/util/edit.c b/src/lib/util/edit.c
index 64d211a250036ca5102ed65474d431be3773b387..57b076e618bbd8a9d5d2426d7a6731c978b0368d 100644 (file)
--- a/src/lib/util/edit.c
+++ b/src/lib/util/edit.c
@@ -269,12 +269,27 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa
                         *      No need to save the value.
                         */
                case FR_EDIT_VALUE:
+                       if (fr_pair_immutable(vp)) {
+                               fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
+                               return -1;
+                       }
                        return 0;
 
                        /*
                         *      No need to save the value.
                         */
                case FR_EDIT_DELETE:
+                       /*
+                        *      We silently refuse to delete immutable attributes.
+                        */
+                       if (fr_pair_immutable(vp)) return 0;
+
+                       if (vp->vp_edit) {
+                       vp_edit_error:
+                               fr_strerror_printf("Attribute cannot be removed, as it is being used in a 'foreach' loop - %pP", vp);
+                               return -1;
+                       }
+
                        fr_pair_remove(list, vp);
                        return 0;
 
@@ -466,6 +481,7 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa
                         *      which has been deleted!
                         */
                        e->op = FR_EDIT_DELETE;
+                       fr_assert(!e->vp->vp_edit);
                        fr_dlist_remove(&el->undo, e);
                        goto delete;
 
@@ -522,6 +538,19 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa
 
        case FR_EDIT_DELETE:
        delete:
+               /*
+                *      We silently refuse to delete immutable attributes.
+                */
+               if (fr_pair_immutable(e->vp)) {
+                       talloc_free(e);
+                       return 0;
+               }
+
+               if (e->vp->vp_edit) {
+                       talloc_free(e);
+                       goto vp_edit_error;
+               }
+
                fr_assert(list != NULL);
                fr_assert(ref == NULL);
 
@@ -574,11 +603,6 @@ int fr_edit_list_insert_pair_after(fr_edit_list_t *el, fr_pair_list_t *list, fr_
  */
 int fr_edit_list_pair_delete(fr_edit_list_t *el, fr_pair_list_t *list, fr_pair_t *vp)
 {
-       if (fr_pair_immutable(vp)) {
-               fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
-               return -1;
-       }
-
        if (!el) {
                fr_pair_delete(list, vp);
                return 0;
@@ -602,12 +626,8 @@ int fr_edit_list_pair_delete_by_da(fr_edit_list_t *el, fr_pair_list_t *list, fr_
         *      Delete all VPs with a matching da.
         */
        fr_pair_list_foreach(list, vp) {
-               if (fr_pair_immutable(vp)) continue;
-
                if (vp->da != da) continue;
 
-               (void) fr_pair_remove(list, vp);
-
                if (edit_record(el, FR_EDIT_DELETE, vp, list, NULL) < 0) return -1;
        }
 
@@ -636,11 +656,6 @@ int fr_edit_list_replace_pair_value(fr_edit_list_t *el, fr_pair_t *vp, fr_value_
 {
        if (!fr_type_is_leaf(vp->vp_type)) return -1;
 
-       if (vp->vp_immutable) {
-               fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
-               return -1;
-       }
-
        if (el && (edit_record(el, FR_EDIT_VALUE, vp, NULL, NULL) < 0)) return -1;
 
        if (!fr_type_is_fixed_size(vp->vp_type)) fr_value_box_clear(&vp->data);
@@ -659,11 +674,6 @@ int fr_edit_list_replace_pair(fr_edit_list_t *el, fr_pair_list_t *list, fr_pair_
 {
        if (to_replace->da != vp->da) return -1;
 
-       if (fr_pair_immutable(to_replace)) {
-               fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
-               return -1;
-       }
-
        if (!el) {
                if (fr_pair_insert_after(list, to_replace, vp) < 0) return -1;
                fr_pair_delete(list, to_replace);

diff --git a/src/lib/util/pair.c b/src/lib/util/pair.c
index 4124b6f28efd2b5610edf77b3b1bc6f1faf6f9ea..a7b2238ab54fb17d53c154c82b89701dc1dfc647 100644 (file)
--- a/src/lib/util/pair.c
+++ b/src/lib/util/pair.c
@@ -92,10 +92,12 @@ static int _fr_pair_free(fr_pair_t *vp)
 
        case FR_TYPE_STRING:
        case FR_TYPE_OCTETS:
+               fr_assert(!vp->vp_edit);
                if (vp->data.secret) memset_explicit(vp->vp_ptr, 0, vp->vp_length);
                break;
 
        default:
+               fr_assert(!vp->vp_edit);
                if (vp->data.secret) memset_explicit(&vp->data, 0, sizeof(vp->data));
                break;
        }

diff --git a/src/tests/keywords/foreach-delete b/src/tests/keywords/foreach-delete
index e30e515761f9d5f5cd6af21ef30845abcd2a659f..675f6fc24f66047eba092ce85e4f283e3f7f1bde 100644 (file)
--- a/src/tests/keywords/foreach-delete
+++ b/src/tests/keywords/foreach-delete
@@ -9,10 +9,16 @@ Tmp-Integer-0 := { 1, 3, 5, 11 }
 #  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
+       try {
+               request -= Tmp-Integer-0[1]
+       }
+       catch {
+               self += 19
+       }
 }
 
+%debug(&request)
+
 if (Tmp-Integer-0[0] != 20) {
        test_fail
 }