From: Alan T. DeKok <aland@freeradius.org>
Date: Tue, 12 Nov 2024 16:04:13 +0000 (-0500)
Subject: move struct checking to explicit "is closed"
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ae1d1a51aaf29c23eb0bdad65828aacf0422ee82;p=thirdparty%2Ffreeradius-server.git

move struct checking to explicit "is closed"

instead of checking for a few special cases.

We already have flags->is_known_width, so we set that in more
places, and then in the MEMBER parsing, check if the current
MEMBER is !flags->is_known_width,  If so, the struct is closed.
---

diff --git a/src/lib/util/dict_tokenize.c b/src/lib/util/dict_tokenize.c
index 85a2eac4044..7d02fe7016f 100644
--- a/src/lib/util/dict_tokenize.c
+++ b/src/lib/util/dict_tokenize.c
@@ -60,6 +60,7 @@ typedef struct {
 	fr_dict_attr_t const	*da;			//!< the da we care about
 	dict_nest_t		nest;			//!< for manual vs automatic begin / end things
 	int			member_num;		//!< structure member numbers
+	fr_dict_attr_t const	*struct_is_closed;	//!< no more members are allowed
 	ssize_t			struct_size;		//!< size of the struct.
 } dict_tokenize_frame_t;
 
@@ -1499,12 +1500,10 @@ static int dict_read_process_member(dict_tokenize_ctx_t *ctx, char **argv, int a
 	 *	Check if the parent 'struct' is fixed size.  And if
 	 *	so, complain if we're adding a variable sized member.
 	 */
-	if (ctx->stack[ctx->stack_depth].da->flags.length && ctx->stack[ctx->stack_depth].da->flags.is_known_width &&
-	    ((da->type == FR_TYPE_TLV) || da->flags.is_known_width ||
-	     ((da->type == FR_TYPE_STRING) && !da->flags.length) ||
-	     ((da->type == FR_TYPE_OCTETS) && !da->flags.length))) {
-		fr_strerror_printf("'struct' %s has fixed size %u, we cannot add a variable-sized member.",
-				   ctx->stack[ctx->stack_depth].da->name, ctx->stack[ctx->stack_depth].da->flags.length);
+	if (ctx->stack[ctx->stack_depth].struct_is_closed) {
+		fr_strerror_printf("Cannot add MEMBER to 'struct' %s after a variable sized member %s",
+				   ctx->stack[ctx->stack_depth].da->name,
+				   ctx->stack[ctx->stack_depth].struct_is_closed->name);
 		goto error;
 	}
 
@@ -1574,32 +1573,45 @@ static int dict_read_process_member(dict_tokenize_ctx_t *ctx, char **argv, int a
 			ctx->relative_attr = dict_attr_child_by_num(ctx->stack[ctx->stack_depth].da,
 								ctx->stack[ctx->stack_depth].member_num);
 			if (ctx->relative_attr && (dict_gctx_push(ctx, ctx->relative_attr) < 0)) return -1;
+			return 0;
 
-		} else {
+		}
 
-			/*
-			*	Add the size of this member to the parent struct.
-			*/
-			ctx->stack[ctx->stack_depth].struct_size += da->flags.length;
+		/*
+		 *	Add the size of this member to the parent struct.
+		 */
+		ctx->stack[ctx->stack_depth].struct_size += da->flags.length;
 
-			/*
-			*	Check for overflow.
-			*/
-			if (ctx->stack[ctx->stack_depth].da->flags.length &&
-			(ctx->stack[ctx->stack_depth].struct_size > ctx->stack[ctx->stack_depth].da->flags.length)) {
-				fr_strerror_printf("'struct' %s has fixed size %u, but member %s overflows that length",
-						ctx->stack[ctx->stack_depth].da->name, ctx->stack[ctx->stack_depth].da->flags.length,
-						argv[0]);
-				return -1;
-			}
+		/*
+		 *	Check for overflow.
+		 */
+		if (ctx->stack[ctx->stack_depth].da->flags.length &&
+		    (ctx->stack[ctx->stack_depth].struct_size > ctx->stack[ctx->stack_depth].da->flags.length)) {
+			fr_strerror_printf("'struct' %s has fixed size %u, but member %s overflows that length",
+					   ctx->stack[ctx->stack_depth].da->name, ctx->stack[ctx->stack_depth].da->flags.length,
+					   argv[0]);
+			return -1;
 		}
 
 		/*
-		*	Adding a member of type 'struct' is an implicit BEGIN-STRUCT.
-		*/
+		 *	Adding a member of type 'struct' is an implicit BEGIN-STRUCT.
+		 */
 		if (da->type == FR_TYPE_STRUCT) {
 			if (dict_gctx_push(ctx, da) < 0) return -1;
 			ctx->value_attr = NULL;
+
+		} else {
+			/*
+			 *	Check if this MEMBER closes the structure.
+			 *
+			 *	@todo - close this struct if the child struct is variable sized.  For now, it
+			 *	looks like most child structs are at the end of the parent.
+			 *
+			 *	The solution is to update the unwind() function to check if the da we've
+			 *	unwound to is a struct, and then if so... get the last child, and mark it
+			 *	closed.
+			 */
+			if (!da->flags.is_known_width) ctx->stack[ctx->stack_depth].struct_is_closed = da;
 		}
 		break;
 
diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c
index 00af217a452..01cc360a8c4 100644
--- a/src/lib/util/dict_util.c
+++ b/src/lib/util/dict_util.c
@@ -575,6 +575,8 @@ int dict_attr_type_init(fr_dict_attr_t **da_p, fr_type_t type)
 		break;
 	}
 
+	(*da_p)->flags.is_known_width = fr_type_fixed_size[type];
+
 	/*
 	 *	Set default type-based flags
 	 */
@@ -585,6 +587,12 @@ int dict_attr_type_init(fr_dict_attr_t **da_p, fr_type_t type)
 		(*da_p)->flags.flag_time_res = FR_TIME_RES_SEC;
 		break;
 
+
+	case FR_TYPE_OCTETS:
+	case FR_TYPE_STRING:
+		(*da_p)->flags.is_known_width = ((*da_p)->flags.length != 0);
+		break;
+
 	default:
 		break;
 	}