From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 17 Dec 2023 23:06:47 +0000 (-0500)
Subject: fix ordering issues by reordering the editing list
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a2a29d59ea1574d705e47d523763703599c28d95;p=thirdparty%2Ffreeradius-server.git

fix ordering issues by reordering the editing list

The old radius_pairmove() function went through a lot of work to
avoid deleting attributes it just added.  For example, if it had
"add" followed by "delete", it wouldn't delete the attribute which
it had just added.

That functionality in rlm_files broke when the subnet functionality
was added, as the call to radius_pairmove() was placed inside
of the loop over maps, instead of after it.

Instead of going through all kinds of crazy work again at run time,
we instead just reorder the list when sanitizing it.

Deletions are placed first.  Then set (which is partially delete)
and then any other add operations.

That way we know that we are never deleting an attribute which we
just added.
---

diff --git a/src/modules/rlm_files/rlm_files.c b/src/modules/rlm_files/rlm_files.c
index c3a1e730e8a..596e49f624e 100644
--- a/src/modules/rlm_files/rlm_files.c
+++ b/src/modules/rlm_files/rlm_files.c
@@ -149,8 +149,9 @@ static int getusersfile(TALLOC_CTX *ctx, char const *filename, fr_htrie_t **ptre
 	entry = NULL;
 	while ((entry = fr_dlist_next(&users.head, entry))) {
 		map_t *map = NULL;
-		map_t *prev;
+		map_t *prev, *next_map;
 		fr_dict_attr_t const *da;
+		map_t *sub_head, *set_head;
 
 		reply_head = NULL;
 
@@ -202,6 +203,11 @@ static int getusersfile(TALLOC_CTX *ctx, char const *filename, fr_htrie_t **ptre
 			}
 		} /* end of loop over check items */
 
+		/*
+		 *	Note that we also re-arrange any control items which are in the reply item list.
+		 */
+		sub_head = set_head = NULL;
+
 		/*
 		 *	Look for server configuration items
 		 *	in the reply list.
@@ -209,8 +215,11 @@ static int getusersfile(TALLOC_CTX *ctx, char const *filename, fr_htrie_t **ptre
 		 *	It's a common enough mistake, that it's
 		 *	worth doing.
 		 */
-		map = NULL;
-		while ((map = map_list_next(&entry->reply, map))) {
+		for (map = map_list_head(&entry->reply);
+		     map != NULL;
+		     map = next_map) {
+			next_map = map_list_next(&entry->reply, map);
+
 			if (!tmpl_is_attr(map->lhs)) {
 				ERROR("%s[%d] Left side of reply item %s is not an attribute",
 				      entry->filename, entry->lineno, map->lhs->name);
@@ -243,9 +252,33 @@ static int getusersfile(TALLOC_CTX *ctx, char const *filename, fr_htrie_t **ptre
 					entry->fall_through = tmpl_value(map->rhs)->vb_bool;
 				}
 
-				prev = map_list_prev(&entry->reply, map);
 				(void) map_list_remove(&entry->reply, map);
-				map = prev;
+				continue;
+			}
+
+			/*
+			 *	Removals are applied before anything else.
+			 */
+			if (map->op == T_OP_SUB_EQ) {
+				if (sub_head == map) continue;
+
+				(void) map_list_remove(&entry->reply, map);
+				map_list_insert_after(&entry->reply, sub_head, map);
+				sub_head = map;
+				continue;
+			}
+
+			/*
+			 *	Over-rides are applied after deletions.
+			 */
+			if (map->op == T_OP_SET) {
+				if (set_head == map) continue;
+
+				if (!set_head) set_head = sub_head;
+
+				(void) map_list_remove(&entry->reply, map);
+				map_list_insert_after(&entry->reply, set_head, map);
+				set_head = map;
 				continue;
 			}
 		}