From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Fri, 18 Nov 2016 18:17:17 +0000 (-0500)
Subject: Fix DHCP attribute ordering
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3ddc2d8bca823dbbf3836a7d2f0f2f4e9f59c7d2;p=thirdparty%2Ffreeradius-server.git

Fix DHCP attribute ordering
---

diff --git a/src/include/pair.h b/src/include/pair.h
index d7598441e25..70f68507d27 100644
--- a/src/include/pair.h
+++ b/src/include/pair.h
@@ -257,6 +257,7 @@ typedef		int8_t (*fr_cmp_t)(void const *a, void const *b);
  */
 #define		fr_pair_cmp_op(_op, _a, _b)	value_box_cmp_op(_op, _a->da->type, &_a->data, _b->da->type, &_b->data)
 int8_t		fr_pair_cmp_by_da_tag(void const *a, void const *b);
+int8_t		fr_pair_cmp_by_parent_num_tag(void const *a, void const *b);
 int		fr_pair_cmp(VALUE_PAIR *a, VALUE_PAIR *b);
 int		fr_pair_list_cmp(VALUE_PAIR *a, VALUE_PAIR *b);
 void		fr_pair_list_sort(VALUE_PAIR **vps, fr_cmp_t cmp);
diff --git a/src/lib/pair.c b/src/lib/pair.c
index 26759fe4eea..56e292683ab 100644
--- a/src/lib/pair.c
+++ b/src/lib/pair.c
@@ -903,16 +903,28 @@ void fr_pair_delete_by_num(VALUE_PAIR **head, unsigned int vendor, unsigned int
 	}
 }
 
+/** Order attributes by their da, and tag
+ *
+ * Useful where attributes need to be aggregated, but not necessarily
+ * ordered by attribute number.
+ *
+ * @param[in] a		first dict_attr_t.
+ * @param[in] b		second dict_attr_t.
+ * @return
+ *	- +1 if a > b
+ *	- 0 if a == b
+ *	- -1 if a < b
+ */
 int8_t fr_pair_cmp_by_da_tag(void const *a, void const *b)
 {
 	VALUE_PAIR const *my_a = a;
 	VALUE_PAIR const *my_b = b;
 
+	uint8_t cmp;
+
 	VERIFY_VP(my_a);
 	VERIFY_VP(my_b);
 
-	uint8_t cmp;
-
 	cmp = fr_pointer_cmp(my_a->da, my_b->da);
 	if (cmp != 0) return cmp;
 
@@ -923,6 +935,81 @@ int8_t fr_pair_cmp_by_da_tag(void const *a, void const *b)
 	return 0;
 }
 
+/** Order attributes by their attribute number, and tag
+ *
+ * @param[in] a		first dict_attr_t.
+ * @param[in] b		second dict_attr_t.
+ * @return
+ *	- +1 if a > b
+ *	- 0 if a == b
+ *	- -1 if a < b
+ */
+static inline int8_t pair_cmp_by_num_tag(void const *a, void const *b)
+{
+	VALUE_PAIR const *my_a = a;
+	VALUE_PAIR const *my_b = b;
+
+	VERIFY_VP(my_a);
+	VERIFY_VP(my_b);
+
+	if (my_a->da->attr < my_b->da->attr) return -1;
+	if (my_a->da->attr > my_b->da->attr) return +1;
+
+	if (my_a->tag < my_b->tag) return -1;
+	if (my_a->tag > my_b->tag) return +1;
+
+	return 0;
+}
+
+/** Order attributes by their parent(s), attribute number, and tag
+ *
+ * Useful for some protocols where attributes of the same number should by aggregated
+ * within a packet or container TLV.
+ *
+ * @param[in] a		first dict_attr_t.
+ * @param[in] b		second dict_attr_t.
+ * @return
+ *	- +1 if a > b
+ *	- 0 if a == b
+ *	- -1 if a < b
+ */
+int8_t fr_pair_cmp_by_parent_num_tag(void const *a, void const *b)
+{
+	VALUE_PAIR const	*vp_a = a;
+	VALUE_PAIR const	*vp_b = b;
+	fr_dict_attr_t const	*da_a = vp_a->da;
+	fr_dict_attr_t const	*da_b = vp_b->da;
+	fr_dict_attr_t const	*tlv_stack_a[FR_DICT_MAX_TLV_STACK + 1];
+	fr_dict_attr_t const	*tlv_stack_b[FR_DICT_MAX_TLV_STACK + 1];
+
+	/*
+	 *	Fast path (assuming attributes
+	 *	are in the same dictionary).
+	 */
+	if ((da_b->parent->flags.is_root) && (da_b->parent->flags.is_root)) return pair_cmp_by_num_tag(vp_a, vp_b);
+
+	fr_proto_tlv_stack_build(tlv_stack_a, da_a);
+	fr_proto_tlv_stack_build(tlv_stack_b, da_b);
+
+	for (da_a = tlv_stack_a[0], da_b = tlv_stack_b[0]; da_a && da_b; da_a++, da_b++) {
+		if (da_a->attr > da_b->attr) return +1;
+		if (da_b->attr < da_b->attr) return -1;
+	}
+
+	/*
+	 *	If a has a shallower attribute
+	 *	hierarchy than b, it should come
+	 *	before b.
+	 */
+	if (!da_a && da_b) return +1;
+	if (da_a && !da_b) return -1;
+
+	if (vp_a->tag > vp_b->tag) return +1;
+	if (vp_b->tag < vp_b->tag) return -1;
+
+	return 0;
+}
+
 /** Compare two pairs, using the operator from "a"
  *
  *	i.e. given two attributes, it does:
diff --git a/src/modules/proto_dhcp/dhcp.c b/src/modules/proto_dhcp/dhcp.c
index 46f99179aff..3e177951a50 100644
--- a/src/modules/proto_dhcp/dhcp.c
+++ b/src/modules/proto_dhcp/dhcp.c
@@ -1323,18 +1323,28 @@ int8_t fr_dhcp_attr_cmp(void const *a, void const *b)
 	VERIFY_VP(my_b);
 
 	/*
-	 *	DHCP-Message-Type is first, for simplicity.
+	 *	We can only use attribute numbers if we know they're
+	 *	not nested attributes.
+	 *
+	 *	@fixme We should be able to use my_a->da->parent->flags.is_root,
+	 *	but the DHCP attributes are hacked into the server under a vendor
+	 *	dictionary, so we can't.
 	 */
-	if ((my_a->da->attr == PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr != PW_DHCP_MESSAGE_TYPE)) return -1;
+	if ((my_a->da->parent->type != PW_TYPE_TLV) && (my_b->da->parent->type != PW_TYPE_TLV)) {
+		/*
+		 *	DHCP-Message-Type is first, for simplicity.
+		 */
+		if ((my_a->da->attr == PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr != PW_DHCP_MESSAGE_TYPE)) return -1;
+		if ((my_a->da->attr != PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr == PW_DHCP_MESSAGE_TYPE)) return +1;
 
-	/*
-	 *	Relay-Agent is last
-	 */
-	if ((my_a->da->attr == PW_DHCP_OPTION_82) && (my_b->da->attr != PW_DHCP_OPTION_82)) return 1;
-	if (my_a->da->attr < my_b->da->attr) return -1;
-	if (my_a->da->attr > my_b->da->attr) return 1;
+		/*
+		 *	Relay-Agent is last
+		 */
+		if ((my_a->da->attr == PW_DHCP_OPTION_82) && (my_b->da->attr != PW_DHCP_OPTION_82)) return +1;
+		if ((my_a->da->attr != PW_DHCP_OPTION_82) && (my_b->da->attr == PW_DHCP_OPTION_82)) return -1;
+	}
 
-	return 0;
+	return fr_pair_cmp_by_parent_num_tag(my_a->da, my_b->da);
 }
 
 /** Write DHCP option value into buffer