From: Alan T. DeKok <aland@freeradius.org>
Date: Thu, 13 Jul 2023 13:50:07 +0000 (-0400)
Subject: clean up fr_pair_list_move_op() and uses
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=48b7d6312c84ece8fd1fa9f3a59a27367cecfbad;p=thirdparty%2Ffreeradius-server.git

clean up fr_pair_list_move_op() and uses

some users didn't free the source list.  But it should really have
been freed.  So we move the free into fr_pair_list_move_op()

and do other minor cleanups of the function and users
---

diff --git a/src/lib/server/map.c b/src/lib/server/map.c
index f50233be8fb..356ef6d98fa 100644
--- a/src/lib/server/map.c
+++ b/src/lib/server/map.c
@@ -1917,14 +1917,12 @@ int map_to_request(request_t *request, map_t const *map, radius_map_getvalue_t f
 				FALL_THROUGH;
 
 		case T_OP_ADD_EQ:
-				fr_pair_list_move_op(list, &src_list, T_OP_ADD_EQ);
-				fr_pair_list_free(&src_list);
+			fr_pair_list_move_op(list, &src_list, T_OP_ADD_EQ);
 			}
 			goto update;
 
 		case T_OP_PREPEND:
-			fr_pair_list_move_op(list, &src_list, map->op);
-			fr_pair_list_free(&src_list);
+			fr_pair_list_move_op(list, &src_list, T_OP_PREPEND);
 			goto update;
 
 		default:
diff --git a/src/lib/server/pairmove.c b/src/lib/server/pairmove.c
index 1f42a114320..d0a575ec960 100644
--- a/src/lib/server/pairmove.c
+++ b/src/lib/server/pairmove.c
@@ -35,10 +35,7 @@ RCSID("$Id$")
 #include <ctype.h>
 
 /*
- *	The fr_pair_list_move_op() function in src/lib/valuepair.c does all sorts of
- *	extra magic that we don't want here.
- *
- *	FIXME: integrate this with the code calling it, so that we
+ *	@fixme - integrate this with the code calling it, so that we
  *	only fr_pair_list_copy() those attributes that we're really going to
  *	use.
  */
@@ -57,12 +54,7 @@ void radius_pairmove(request_t *request, fr_pair_list_t *to, fr_pair_list_t *fro
 	 *	deletion.
 	 *
 	 *	It also means that the operators apply ONLY to the
-	 *	attributes in the original list.  With the previous
-	 *	implementation of fr_pair_list_move_op(), adding two attributes
-	 *	via "+=" and then "=" would mean that the second one
-	 *	wasn't added, because of the existence of the first
-	 *	one in the "to" list.  This implementation doesn't
-	 *	have that bug.
+	 *	attributes in the original list.
 	 *
 	 *	Also, the previous implementation did NOT implement
 	 *	"-=" correctly.  If two of the same attributes existed
diff --git a/src/lib/util/pair_legacy.c b/src/lib/util/pair_legacy.c
index 259228a5aea..5901ebf4c59 100644
--- a/src/lib/util/pair_legacy.c
+++ b/src/lib/util/pair_legacy.c
@@ -519,8 +519,8 @@ int fr_pair_list_afrom_file(TALLOC_CTX *ctx, fr_dict_t const *dict, fr_pair_list
  */
 void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t op)
 {
-	fr_pair_t *i, *found;
-	fr_pair_list_t head_new, head_prepend;
+	fr_pair_t *vp, *next, *found;
+	fr_pair_list_t head_append, head_prepend;
 
 	if (!to || fr_pair_list_empty(from)) return;
 
@@ -530,7 +530,7 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 	 *	be edited, so we create an intermediate list to hold
 	 *	them during the editing process.
 	 */
-	fr_pair_list_init(&head_new);
+	fr_pair_list_init(&head_append);
 
 	/*
 	 *	Any attributes that are requested to be prepended
@@ -542,16 +542,15 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 	 *	We're looping over the "from" list, moving some
 	 *	attributes out, but leaving others in place.
 	 */
-	for (i = fr_pair_list_head(from); i; ) {
-		fr_pair_t *j;
-
-		PAIR_VERIFY(i);
+	for (vp = fr_pair_list_head(from); vp != NULL; vp = next) {
+		PAIR_VERIFY(vp);
+		next = fr_pair_list_next(from, vp);
 
 		/*
 		 *	We never move Fall-Through.
 		 */
-		if (fr_dict_attr_is_top_level(i->da) && (i->da->attr == FR_FALL_THROUGH)) {
-			i = fr_pair_list_next(from, i);
+		if (fr_dict_attr_is_top_level(vp->da) && (vp->da->attr == FR_FALL_THROUGH) &&
+		    (fr_dict_by_da(vp->da) == fr_dict_internal())) {
 			continue;
 		}
 
@@ -561,14 +560,13 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 		 *	treatment for passwords or Hint.
 		 */
 
-		switch (i->op) {
+		switch (vp->op) {
 		/*
 		 *	Anything else are operators which
 		 *	shouldn't occur.  We ignore them, and
 		 *	leave them in place.
 		 */
 		default:
-			i = fr_pair_list_next(from, i);
 			continue;
 
 		/*
@@ -576,10 +574,8 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 		 *	it doesn't already exist.
 		 */
 		case T_OP_EQ:
-			found = fr_pair_find_by_da(to, NULL, i->da);
+			found = fr_pair_find_by_da(to, NULL, vp->da);
 			if (!found) goto do_add;
-
-			i = fr_pair_list_next(from, i);
 			continue;
 
 		/*
@@ -587,12 +583,11 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 		 *	of the same vendor/attr which already exists.
 		 */
 		case T_OP_SET:
-			found = fr_pair_find_by_da(to, NULL, i->da);
+			found = fr_pair_find_by_da(to, NULL, vp->da);
 			if (!found) goto do_add;
 
 			/*
-			 *	Delete *all* of the attributes
-			 *	of the same number.
+			 *	Delete *all* matching attribues.
 			 */
 			fr_pair_delete_by_da(to, found->da);
 			goto do_add;
@@ -603,16 +598,13 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 		 */
 		case T_OP_ADD_EQ:
 	do_add:
-			j = fr_pair_list_next(from, i);
-			fr_pair_remove(from, i);
-			fr_pair_append(&head_new, i);
-			i = j;
+			fr_pair_remove(from, vp);
+			fr_pair_append(&head_append, vp);
 			continue;
+
 		case T_OP_PREPEND:
-			j = fr_pair_list_next(from, i);
-			fr_pair_remove(from, i);
-			fr_pair_prepend(&head_prepend, i);
-			i = j;
+			fr_pair_remove(from, vp);
+			fr_pair_prepend(&head_prepend, vp);
 			continue;
 		}
 	} /* loop over the "from" list. */
@@ -622,7 +614,7 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 	 *	attributes first as those whose individual operator
 	 *	is prepend should be prepended to the resulting list
 	 */
-	if (op == T_OP_PREPEND) fr_pair_list_prepend(to, &head_new);
+	if (op == T_OP_PREPEND) fr_pair_list_prepend(to, &head_append);
 
 	/*
 	 *	If there are any items in the prepend list prepend
@@ -634,6 +626,7 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
 	 *	If the op parameter was not prepend, take the "new"
 	 *	list, and append it to the "to" list.
 	 */
-	if (op != T_OP_PREPEND) fr_pair_list_append(to, &head_new);
+	if (op != T_OP_PREPEND) fr_pair_list_append(to, &head_append);
 
+	fr_pair_list_free(from);
 }
diff --git a/src/lib/util/pair_legacy_tests.c b/src/lib/util/pair_legacy_tests.c
index e4c1626f103..4be133e7df3 100644
--- a/src/lib/util/pair_legacy_tests.c
+++ b/src/lib/util/pair_legacy_tests.c
@@ -200,7 +200,6 @@ static void test_fr_pair_list_move_op(void)
 	TEST_CASE("Checking if (Test-String-0 == 'Testing123')");
 	TEST_CHECK(vp && strcmp(vp->vp_strvalue, "Testing123") == 0);
 
-	fr_pair_list_free(&old_list);
 	fr_pair_list_free(&new_list);
 
 	fclose(fp);
diff --git a/src/modules/rlm_files/rlm_files.c b/src/modules/rlm_files/rlm_files.c
index 235ea538128..e351d4c785a 100644
--- a/src/modules/rlm_files/rlm_files.c
+++ b/src/modules/rlm_files/rlm_files.c
@@ -511,7 +511,6 @@ redo:
 		 *	Move the control items over, too.
 		 */
 		fr_pair_list_move_op(&request->control_pairs, &list, T_OP_ADD_EQ);
-		fr_pair_list_free(&list);
 
 		/* ctx may be reply */
 		if (!map_list_empty(&pl->reply)) {