From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 5 Jun 2022 13:54:15 +0000 (-0400)
Subject: omit optional arguments entirely, instead of passing the wrong type
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9ab7aeba954f37e48d604e4835858c5b57d7e60d;p=thirdparty%2Ffreeradius-server.git

omit optional arguments entirely, instead of passing the wrong type

if an optional argument is missing (i.e. source list is empty),
then we can pass it to the function ONLY if the function accepts
type VOID for that argument.

Otherwise, we have to omit that argument, and all subsequent ones.
Otherwise, we'll pass the wrong data type to a function which
expects to be able to dereference the type-specific field of that
value box, and who knows what will happen.

As a result, any function which takes an optional argument has to
check for non-existence, not for NULL type.

We will reserve NULL types for "error", not for normal usage.
---

diff --git a/src/lib/eap_aka_sim/xlat.c b/src/lib/eap_aka_sim/xlat.c
index 85e2db8d26f..b7920403b9b 100644
--- a/src/lib/eap_aka_sim/xlat.c
+++ b/src/lib/eap_aka_sim/xlat.c
@@ -245,7 +245,7 @@ static xlat_action_t aka_sim_3gpp_temporary_id_decrypt_xlat(TALLOC_CTX *ctx, fr_
 	fr_value_box_t	*vb;
 	fr_pair_t	*eap_type;
 
-	if (!fr_type_is_null(tag_vb->type)) include_tag = tag_vb->vb_bool;
+	if (tag_vb) include_tag = tag_vb->vb_bool;
 
 	if (id_len != (AKA_SIM_3GPP_PSEUDONYM_LEN)) {
 		REDEBUG2("3gpp pseudonym incorrect length, expected %i bytes, got %zu bytes",
@@ -384,7 +384,7 @@ static xlat_action_t aka_sim_3gpp_temporary_id_encrypt_xlat(TALLOC_CTX *ctx, fr_
 	/*
 	 *	Check for the optional type argument
 	 */
-	if (!fr_type_is_null(type_vb->type)) {
+	if (type_vb) {
 		fr_dict_enum_value_t const		*type_enum;
 
 		type_enum = fr_dict_enum_by_name(attr_eap_aka_sim_identity_type,
diff --git a/src/lib/unlang/xlat_builtin.c b/src/lib/unlang/xlat_builtin.c
index 0dac2b39fb0..0a015ba0670 100644
--- a/src/lib/unlang/xlat_builtin.c
+++ b/src/lib/unlang/xlat_builtin.c
@@ -961,7 +961,7 @@ static xlat_action_t xlat_func_debug(TALLOC_CTX *ctx, fr_dcursor_t *out,
 	/*
 	 *  Assume we just want to get the current value and NOT set it to 0
 	 */
-	if (fr_box_is_null(in_head)) goto done;
+	if (!in_head) goto done;
 
 	level = in_head->vb_int8;
 	if (level == 0) {
diff --git a/src/lib/unlang/xlat_eval.c b/src/lib/unlang/xlat_eval.c
index dbb4a162b7e..8c0f97b4334 100644
--- a/src/lib/unlang/xlat_eval.c
+++ b/src/lib/unlang/xlat_eval.c
@@ -410,6 +410,7 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list, requ
 			 */
 			if (!vb) {
 				if (arg_p->required) {
+				missing:
 					REDEBUG("Missing required argument %u", (unsigned int)((arg_p - args) + 1));
 					return XLAT_ACTION_FAIL;
 				}
@@ -437,26 +438,60 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list, requ
 			if (xa != XLAT_ACTION_DONE) return xa;
 
 			/*
-			 *	In some cases we replace the current
-			 *	argument with the head of the group.
+			 *	This argument doesn't exist.  That might be OK, or it may be a fatal error.
 			 */
-			if (arg_p->single || arg_p->concat) {
+			if (fr_dlist_empty(&vb->vb_group)) {
 				/*
-				 *	If the group is empty, convert
-				 *	it to a null box to maintain
-				 *	correct ordering in the argument
-				 *	list.
+				 *	Empty groups for optional arguments are OK, we can just stop processing the list.
 				 */
-				if (fr_dlist_empty(&vb->vb_group)) {
-					fr_value_box_t *prev = fr_dlist_remove(list, vb);
-					fr_value_box_init_null(vb);
-					fr_dlist_insert_after(list, prev, vb);
-				} else {
-					fr_dlist_replace(list, vb, fr_dlist_pop_head(&vb->vb_group));
+				if (!arg_p->required) {
+					fr_assert(!next);
+
+					/*
+					 *	If the caller doesn't care about the type, then we leave the
+					 *	empty group there.
+					 */
+					if (arg_p->type == FR_TYPE_VOID) goto do_next;
+
+					/*
+					 *	The caller does care about the type, and we don't have any
+					 *	matching data.  Omit this argument, and all arguments after it.
+					 *
+					 *	i.e. if the caller has 3 optional arguments, all
+					 *	FR_TYPE_UINT8, and the first one is missing, then we MUST
+					 *	either supply boxes all of FR_TYPE_UINT8, OR we supply nothing.
+					 *
+					 *	We can't supply a box of any other type, because the caller
+					 *	has declared that it wants FR_TYPE_UINT8, and is naively
+					 *	accessing the box as vb_uint8, hoping that it's being passed
+					 *	the right thing.
+					 */
+					fr_dlist_remove(list, vb);
 					talloc_free(vb);
+					break;
 				}
+
+				/*
+				 *	If the caller is expecting a particular type, then getting nothing is
+				 *	an error.
+				 *
+				 *	If the caller manually checks the input type, then we can leave it as
+				 *	an empty group.
+				 */
+				if (arg_p->type != FR_TYPE_VOID) goto missing;
+			}
+
+			/*
+			 *	In some cases we replace the current argument with the head of the group.
+			 *
+			 *	xlat_process_arg_list() has already done concatenations for us.
+			 */
+			if (arg_p->single || arg_p->concat) {
+				fr_dlist_replace(list, vb, fr_dlist_pop_head(&vb->vb_group));
+				talloc_free(vb);
 			}
 
+		do_next:
 			if (arg_p->variadic) {
 				if (!next) break;
 			} else {
diff --git a/src/modules/rlm_client/rlm_client.c b/src/modules/rlm_client/rlm_client.c
index b195880b0f2..7a4b431be62 100644
--- a/src/modules/rlm_client/rlm_client.c
+++ b/src/modules/rlm_client/rlm_client.c
@@ -240,7 +240,7 @@ static xlat_action_t xlat_client(TALLOC_CTX *ctx, fr_dcursor_t *out,
 	fr_value_box_t	*client_ip = fr_dlist_next(in, field);
 	fr_value_box_t	*vb;
 
-	if (!fr_box_is_null(client_ip)) {
+	if (client_ip) {
 		if (fr_inet_pton(&ip, client_ip->vb_strvalue, -1, AF_UNSPEC, false, true) < 0) {
 			RDEBUG("Invalid client IP address \"%s\"", client_ip->vb_strvalue);
 			return XLAT_ACTION_FAIL;