From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 2 Mar 2025 16:05:51 +0000 (-0500)
Subject: simplify sorting of sets
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d5af0092a88c7ae25a461ee5954cf0cb87f46de2;p=thirdparty%2Ffreeradius-server.git

simplify sorting of sets

so that there's less "back and forth"

Also the number of children might not be the same as the
number of _encodeable_ children, especially if the set is a group,
and there are internal attributes in the group

As a result, we set the max to the number of possible children,
and then loop until we've encoded all of the children
---

diff --git a/src/protocols/der/encode.c b/src/protocols/der/encode.c
index cc024e193f3..d6c46387a0b 100644
--- a/src/protocols/der/encode.c
+++ b/src/protocols/der/encode.c
@@ -772,8 +772,6 @@ static ssize_t fr_der_encode_set(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, fr_der
 {
 	fr_dbuff_t	      our_dbuff = FR_DBUFF(dbuff);
 	fr_pair_t	     *vp;
-	fr_da_stack_t	      da_stack;
-	fr_dcursor_t	      child_cursor;
 	ssize_t		      slen;
 	unsigned int	      depth = 0;
 
@@ -815,71 +813,81 @@ static ssize_t fr_der_encode_set(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, fr_der
 		/*
 		 *	Set-of items will all have the same tag, so we need to sort them lexicographically
 		 */
+		size_t				  i, count;
 		fr_dbuff_t			  work_dbuff;
 		fr_der_encode_set_of_ptr_pairs_t *ptr_pairs;
 		uint8_t				 *buff;
-		size_t				  i = 0, count;
-
-		buff = talloc_array(encode_ctx->tmp_ctx, uint8_t, fr_dbuff_remaining(&our_dbuff));
-
-		fr_dbuff_init(&work_dbuff, buff, fr_dbuff_remaining(&our_dbuff));
-
-		fr_proto_da_stack_build(&da_stack, vp->da);
-
-		FR_PROTO_STACK_PRINT(&da_stack, depth);
-
-		fr_pair_dcursor_child_iter_init(&child_cursor, &vp->children, cursor);
+		fr_da_stack_t	      		  da_stack;
+		fr_dcursor_t	      		  child_cursor;
 
+		/*
+		 *	This can happen, but is possible.
+		 */
 		count = fr_pair_list_num_elements(&vp->children);
+		if (unlikely(!count)) return 0;
+
+		/*
+		 *	Sets can be nested, so we have to use local buffers when sorting.
+		 */
+		buff = talloc_array(encode_ctx->tmp_ctx, uint8_t, fr_dbuff_remaining(&our_dbuff));
+		fr_assert(buff != NULL);
 
-		ptr_pairs = talloc_array(encode_ctx->tmp_ctx, fr_der_encode_set_of_ptr_pairs_t, count);
+		ptr_pairs = talloc_array(buff, fr_der_encode_set_of_ptr_pairs_t, count);
 		if (unlikely(ptr_pairs == NULL)) {
 			fr_strerror_const("Failed to allocate memory for set of pointers");
 			talloc_free(buff);
 			return -1;
 		}
 
-		for (i = 0; i < count; i++) {
-			if (unlikely(fr_dcursor_current(&child_cursor) == NULL)) {
-				fr_strerror_const("No pair to encode set of");
-				slen = -1;
+		/*
+		 *	Now that we have our intermediate buffers, initialize the buffers and start encoding.
+		 */
+		fr_dbuff_init(&work_dbuff, buff, fr_dbuff_remaining(&our_dbuff));
 
-			free_and_return:
-				talloc_free(ptr_pairs);
-				talloc_free(buff);
-				return slen;
-			}
+		fr_proto_da_stack_build(&da_stack, vp->da);
+
+		FR_PROTO_STACK_PRINT(&da_stack, depth);
+
+		fr_pair_dcursor_child_iter_init(&child_cursor, &vp->children, cursor);
 
+		for (i = 0; fr_dcursor_current(&child_cursor) != NULL; i++) {
 			ptr_pairs[i].data = fr_dbuff_current(&work_dbuff);
 
 			slen = encode_value(&work_dbuff, NULL, depth, &child_cursor, encode_ctx);
-
 			if (unlikely(slen < 0)) {
 				fr_strerror_printf("Failed to encode pair: %s", fr_strerror());
-				goto free_and_return;
+				talloc_free(buff);
+				return slen;
 			}
 
-			ptr_pairs[i].len  = slen;
+			ptr_pairs[i].len = slen;
 		}
 
-		if (unlikely(fr_dcursor_current(&child_cursor) != NULL)) {
-			fr_strerror_const("Failed to encode all pairs");
-			slen = -1;
-			goto free_and_return;
-		}
+		fr_assert(i <= count);
+		count = i;
 
-		qsort(ptr_pairs, count, sizeof(fr_der_encode_set_of_ptr_pairs_t), fr_der_encode_set_of_cmp);
+		/*
+		 *	If there's a "min" for this set, then we can't do anything about it.
+		 */
+		if (unlikely(!count)) goto done;
+
+		/*
+		 *	If there's only one child, we don't need to sort it.
+		 */
+		if (count > 1) qsort(ptr_pairs, count, sizeof(fr_der_encode_set_of_ptr_pairs_t), fr_der_encode_set_of_cmp);
 
+		/*
+		 *	The data in work_dbuff is always less than the data in the our_dbuff, so we don't need
+		 *	to check the return value here.
+		 */
 		for (i = 0; i < count; i++) {
-			if (fr_dbuff_in_memcpy(&our_dbuff, ptr_pairs[i].data, ptr_pairs[i].len) <= 0) {
-				fr_strerror_const("Failed to copy set of value");
-				slen = -1;
-				goto free_and_return;
-			}
+			(void) fr_dbuff_in_memcpy(&our_dbuff, ptr_pairs[i].data, ptr_pairs[i].len);
 		}
 
-		slen = fr_dbuff_set(dbuff, &our_dbuff);
-		goto free_and_return;
+	done:
+		talloc_free(buff);
+
+		return fr_dbuff_set(dbuff, &our_dbuff);
 	}
 
 	/*