From: Alan T. DeKok <aland@freeradius.org>
Date: Tue, 18 Mar 2025 13:45:27 +0000 (+0700)
Subject: push checks for vp_edit and immutable down to edit list
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=219f82102beff241cf5e346d46e28f505aa2ed67;p=thirdparty%2Ffreeradius-server.git

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 dccefced328..d87453fe811 100644
--- 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 64d211a2500..57b076e618b 100644
--- 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 4124b6f28ef..a7b2238ab54 100644
--- 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 e30e515761f..675f6fc24f6 100644
--- 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
 }