From: Alan T. DeKok <aland@freeradius.org>
Date: Thu, 18 May 2023 21:02:07 +0000 (-0400)
Subject: make fr_pair_update_by_da_parent() do nested attributes
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ae112ed65efdcd508383afe98a296f9b8fcc0f46;p=thirdparty%2Ffreeradius-server.git

make fr_pair_update_by_da_parent() do nested attributes

which automatically means that modules like mschap will create
attributes with the correct hierarchy.

Note that ALL callers of pair_update_request() and pair_update_reply()
will be affected.  However, most of them create attributes at the
"root", and will not be affected by this change.
---

diff --git a/src/lib/server/pair.h b/src/lib/server/pair.h
index 3a7b85e6d9f..0a9a608ae8b 100644
--- a/src/lib/server/pair.h
+++ b/src/lib/server/pair.h
@@ -115,7 +115,7 @@ RCSIDH(server_pair_h, "$Id$")
  *	- 0 if we allocated a new attribute.
  *	- -1 on failure.
  */
-#define pair_update_request(_attr, _da) fr_pair_update_by_da(request->request_ctx, _attr, _da)
+#define pair_update_request(_attr, _da) fr_pair_update_by_da_parent(request->request_ctx, _attr, _da)
 
 /** Return or allocate a fr_pair_t in the reply list
  *
@@ -126,7 +126,7 @@ RCSIDH(server_pair_h, "$Id$")
  *	- 0 if we allocated a new attribute.
  *	- -1 on failure.
  */
-#define pair_update_reply(_attr, _da) fr_pair_update_by_da(request->reply_ctx, _attr, _da)
+#define pair_update_reply(_attr, _da) fr_pair_update_by_da_parent(request->reply_ctx, _attr, _da)
 
 /** Return or allocate a fr_pair_t in the control list
  *
@@ -137,7 +137,7 @@ RCSIDH(server_pair_h, "$Id$")
  *	- 0 if we allocated a new attribute.
  *	- -1 on failure.
  */
-#define pair_update_control(_attr, _da) fr_pair_update_by_da(request->control_ctx, _attr, _da)
+#define pair_update_control(_attr, _da) fr_pair_update_by_da_parent(request->control_ctx, _attr, _da)
 
 /** Return or allocate a fr_pair_t in the session_state list
  *
@@ -148,7 +148,7 @@ RCSIDH(server_pair_h, "$Id$")
  *	- 0 if we allocated a new attribute.
  *	- -1 on failure.
  */
-#define pair_update_session_state(_attr, _da) fr_pair_update_by_da(request->session_state_ctx, _attr, _da)
+#define pair_update_session_state(_attr, _da) fr_pair_update_by_da_parent(request->session_state_ctx, _attr, _da)
 
 /** Delete one or move fr_pair_t in a list
  *
diff --git a/src/lib/util/pair.c b/src/lib/util/pair.c
index 9847e0a6d6f..6f0d6e81f2d 100644
--- a/src/lib/util/pair.c
+++ b/src/lib/util/pair.c
@@ -1488,28 +1488,69 @@ int fr_pair_append_by_da_parent(TALLOC_CTX *ctx, fr_pair_t **out, fr_pair_list_t
  *	- 0 if we allocated a new attribute.
  *	- -1 on failure.
  */
-int fr_pair_update_by_da(fr_pair_t *parent, fr_pair_t **out,
-			 fr_dict_attr_t const *da)
+int fr_pair_update_by_da_parent(fr_pair_t *parent, fr_pair_t **out,
+				fr_dict_attr_t const *da)
 {
-	fr_pair_t	*vp;
+	fr_pair_t		*vp = NULL;
+	fr_da_stack_t		da_stack;
+	fr_dict_attr_t const	**find;
+	TALLOC_CTX		*pair_ctx = parent;
+	fr_pair_list_t		*list = &parent->vp_group;
 
-	vp = fr_pair_find_by_da_idx(&parent->vp_group, da, 0);
-	if (vp) {
-		PAIR_VERIFY_WITH_LIST(&parent->vp_group, vp);
-		if (out) *out = vp;
-		return 1;
-	}
+	/*
+	 *	Fast path for non-nested attributes
+	 */
+	if (da->depth <= 1) {
+		vp = fr_pair_find_by_da(list, NULL, da);
+		if (vp) {
+			*out = vp;
+			return 1;
+		}
 
-	vp = fr_pair_afrom_da(parent, da);
-	if (unlikely(!vp)) {
-		if (out) *out = NULL;
-		return -1;
+		return fr_pair_append_by_da(parent, out, list, da);
 	}
 
-	fr_pair_append(&parent->vp_group, vp);
-	if (out) *out = vp;
+	fr_proto_da_stack_build(&da_stack, da);
+	find = &da_stack.da[0];
 
-	return 0;
+	/*
+	 *	Walk down the da stack looking for candidate parent
+	 *	attributes and then allocating the leaf.
+	 */
+	while (true) {
+		fr_assert((*find)->depth <= da->depth);
+
+		/*
+		 *	We're not at the leaf, look for a potential parent
+		 */
+		if ((*find) != da) vp = fr_pair_find_by_da(list, NULL, *find);
+
+		/*
+		 *	Nothing found, create the pair
+		 */
+		if (!vp) {
+			if (fr_pair_append_by_da(pair_ctx, &vp, list, *find) < 0) {
+				*out = NULL;
+				return -1;
+			}
+		}
+
+		/*
+		 *	We're at the leaf, return
+		 */
+		if ((*find) == da) {
+			*out = vp;
+			return 0;
+		}
+
+		/*
+		 *	Prepare for next level
+		 */
+		list = &vp->vp_group;
+		pair_ctx = vp;
+		vp = NULL;
+		find++;
+	}
 }
 
 /** Delete matching pairs from the specified list
diff --git a/src/lib/util/pair.h b/src/lib/util/pair.h
index 4748d12cefd..ee1cea78c3f 100644
--- a/src/lib/util/pair.h
+++ b/src/lib/util/pair.h
@@ -501,8 +501,8 @@ int		fr_pair_prepend_by_da(TALLOC_CTX *ctx, fr_pair_t **out, fr_pair_list_t *lis
 int		fr_pair_append_by_da_parent(TALLOC_CTX *ctx, fr_pair_t **out, fr_pair_list_t *list,
 					    fr_dict_attr_t const *da) CC_HINT(nonnull(3,4));
 
-int		fr_pair_update_by_da(fr_pair_t *parent, fr_pair_t **out,
-				     fr_dict_attr_t const *da) CC_HINT(nonnull(1,3));
+int		fr_pair_update_by_da_parent(fr_pair_t *parent, fr_pair_t **out,
+					    fr_dict_attr_t const *da) CC_HINT(nonnull(1,3));
 
 int		fr_pair_delete_by_da(fr_pair_list_t *head, fr_dict_attr_t const *da) CC_HINT(nonnull);
 
diff --git a/src/lib/util/pair_tests.c b/src/lib/util/pair_tests.c
index 0549cd998a4..c8fb7538896 100644
--- a/src/lib/util/pair_tests.c
+++ b/src/lib/util/pair_tests.c
@@ -523,14 +523,14 @@ static void test_fr_pair_prepend_by_da(void)
 	fr_pair_list_free(&local_pairs);
 }
 
-static void test_fr_pair_update_by_da(void)
+static void test_fr_pair_update_by_da_parent(void)
 {
 	fr_pair_t *vp, *group;
 
 	TEST_CHECK((group = fr_pair_afrom_da(autofree, fr_dict_attr_test_group)) != NULL);
 
 	TEST_CASE("Update Add using fr_pair_prepend_by_da()");
-	TEST_CHECK(fr_pair_update_by_da(group, &vp, fr_dict_attr_test_uint32) == 0); /* attribute doesn't exist in this group */
+	TEST_CHECK(fr_pair_update_by_da_parent(group, &vp, fr_dict_attr_test_uint32) == 0); /* attribute doesn't exist in this group */
 	vp->vp_uint32 = 54321;
 
 	TEST_CASE("Expected fr_dict_attr_test_uint32 (vp->vp_uint32 == 54321)");
@@ -1360,7 +1360,7 @@ TEST_LIST = {
 	{ "fr_pair_prepend_by_da",                test_fr_pair_prepend_by_da },
 	{ "fr_pair_append_by_da_parent",          test_fr_pair_append_by_da_parent },
 	{ "fr_pair_delete_by_child_num",          test_fr_pair_delete_by_child_num },
-	{ "fr_pair_update_by_da",                 test_fr_pair_update_by_da },
+	{ "fr_pair_update_by_da_parent",          test_fr_pair_update_by_da_parent },
 	{ "fr_pair_delete",                       test_fr_pair_delete },
 	{ "fr_pair_delete_by_da",                 test_fr_pair_delete_by_da },