From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Fri, 17 Jul 2026 18:01:30 +0000 (-0400)
Subject: lib/ldap: add fr_ldap_entry_value_find for in place value access
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=06020eedf80d10450825d8600a6cc7052a8211f2;p=thirdparty%2Ffreeradius-server.git

lib/ldap: add fr_ldap_entry_value_find for in place value access

Returns the first value of an attribute referenced in place from the result message, replacing ldap_get_values_len at call sites which only read a single value. Each converted site loses several allocations and a duplicate of the value bytes.
---

diff --git a/src/lib/ldap/base.h b/src/lib/ldap/base.h
index 9528820f73b..b75477212e7 100644
--- a/src/lib/ldap/base.h
+++ b/src/lib/ldap/base.h
@@ -968,6 +968,8 @@ char const	**fr_ldap_berval_to_string_list(TALLOC_CTX *ctx, struct berval **valu
 int		fr_ldap_result_values_len(size_t *num, size_t *strings_len, LDAP *handle, LDAPMessage *result,
 					  char const *attr);
 
+int		fr_ldap_entry_value_find(struct berval *out, LDAP *handle, LDAPMessage *entry, char const *attr);
+
 talloc_str_list_t *fr_ldap_str_list_afrom_result(TALLOC_CTX *ctx, LDAP *handle, LDAPMessage *result,
 						 char const *attr);
 
diff --git a/src/lib/ldap/directory.c b/src/lib/ldap/directory.c
index 80c1220dc4d..0430084ffe5 100644
--- a/src/lib/ldap/directory.c
+++ b/src/lib/ldap/directory.c
@@ -71,6 +71,7 @@ int fr_ldap_directory_result_parse(fr_ldap_directory_t *directory, LDAP *handle,
 	int			entry_cnt, i, num, ldap_errno;
 	LDAPMessage		*entry;
 	struct berval		**values = NULL;
+	struct berval		value;
 
 	/*
 	 *	Connections spawned concurrently may each run discovery
@@ -93,20 +94,14 @@ int fr_ldap_directory_result_parse(fr_ldap_directory_t *directory, LDAP *handle,
 	}
 	directory->discovered = true;
 
-	values = ldap_get_values_len(handle, entry, "vendorname");
-	if (values) {
-		directory->vendor_str = fr_ldap_berval_to_string(directory, values[0]);
+	if (fr_ldap_entry_value_find(&value, handle, entry, "vendorname") > 0) {
+		directory->vendor_str = fr_ldap_berval_to_string(directory, &value);
 		INFO("Directory vendor: %s", directory->vendor_str);
-
-		ldap_value_free_len(values);
 	}
 
-	values = ldap_get_values_len(handle, entry, "vendorversion");
-	if (values) {
-		directory->version_str = fr_ldap_berval_to_string(directory, values[0]);
+	if (fr_ldap_entry_value_find(&value, handle, entry, "vendorversion") > 0) {
+		directory->version_str = fr_ldap_berval_to_string(directory, &value);
 		INFO("Directory version: %s", directory->version_str);
-
-		ldap_value_free_len(values);
 	}
 
 	if (directory->vendor_str) {
@@ -158,10 +153,8 @@ int fr_ldap_directory_result_parse(fr_ldap_directory_t *directory, LDAP *handle,
 	 *	isGlobalCatalogReady is only present on ActiveDirectory
 	 *	instances. AD doesn't provide vendorname or vendorversion
 	 */
-	values = ldap_get_values_len(handle, entry, "isGlobalCatalogReady");
-	if (values) {
+	if (fr_ldap_entry_value_find(&value, handle, entry, "isGlobalCatalogReady") > 0) {
 		directory->type = FR_LDAP_DIRECTORY_ACTIVE_DIRECTORY;
-		ldap_value_free_len(values);
 		goto found;
 	}
 
@@ -184,14 +177,12 @@ int fr_ldap_directory_result_parse(fr_ldap_directory_t *directory, LDAP *handle,
 	/*
 	 *	Oracle Virtual Directory and Oracle Internet Directory
 	 */
-	values = ldap_get_values_len(handle, entry, "orcldirectoryversion");
-	if (values) {
-		if (memmem(values[0]->bv_val, values[0]->bv_len, "OID", 3)) {
+	if (fr_ldap_entry_value_find(&value, handle, entry, "orcldirectoryversion") > 0) {
+		if (memmem(value.bv_val, value.bv_len, "OID", 3)) {
 			directory->type = FR_LDAP_DIRECTORY_ORACLE_INTERNET_DIRECTORY;
-		} else if (memmem(values[0]->bv_val, values[0]->bv_len, "OVD", 3)) {
+		} else if (memmem(value.bv_val, value.bv_len, "OVD", 3)) {
 			directory->type = FR_LDAP_DIRECTORY_ORACLE_VIRTUAL_DIRECTORY;
 		}
-		ldap_value_free_len(values);
 	}
 
 found:
diff --git a/src/lib/ldap/util.c b/src/lib/ldap/util.c
index 8d40feb37e4..41c71e2293d 100644
--- a/src/lib/ldap/util.c
+++ b/src/lib/ldap/util.c
@@ -669,6 +669,65 @@ talloc_str_list_t *fr_ldap_str_list_afrom_result(TALLOC_CTX *ctx, LDAP *handle,
 	return list;
 }
 
+/** Find an attribute in an entry, returning its first value referenced in place
+ *
+ * The value points into the result message the entry belongs to, nothing
+ * is allocated and nothing needs freeing.  The value remains valid until
+ * the result message is freed with ldap_msgfree.
+ *
+ * @param[out] out	First value of the attribute.  Untouched when the
+ *			attribute is not found.
+ * @param[in] handle	the entry was received on.
+ * @param[in] entry	to search.
+ * @param[in] attr	to find.
+ * @return
+ *	- The number of values the attribute has.
+ *	- 0 if the entry does not contain the attribute.
+ *	- -1 if the entry could not be parsed.
+ */
+int fr_ldap_entry_value_find(struct berval *out, LDAP *handle, LDAPMessage *entry, char const *attr)
+{
+	BerElement	*ber = NULL;
+	struct berval	dn, name, value;
+	size_t		attr_len = strlen(attr);
+	ber_len_t	len, remaining;
+	char		*last;
+	ber_tag_t	tag;
+	int		num = 0;
+
+	if (ldap_get_dn_ber(handle, entry, &ber, &dn) != LDAP_SUCCESS) {
+	error:
+		fr_strerror_const("Malformed search result entry");
+		ber_free(ber, 0);
+		return -1;
+	}
+
+	for (;;) {
+		if (ber_get_option(ber, LBER_OPT_BER_REMAINING_BYTES, &remaining) != LBER_OPT_SUCCESS) goto error;
+		if (remaining == 0) break;
+
+		if (ber_scanf(ber, "{m" /*}*/, &name) == LBER_ERROR) goto error;
+
+		if ((name.bv_len != attr_len) || (strncasecmp(name.bv_val, attr, attr_len) != 0)) {
+			if (ber_scanf(ber, "x") == LBER_ERROR) goto error;
+			continue;
+		}
+
+		for (tag = ber_first_element(ber, &len, &last);
+		     tag != LBER_DEFAULT;
+		     tag = ber_next_element(ber, &len, last)) {
+			if (ber_scanf(ber, "m", &value) == LBER_ERROR) goto error;
+
+			if (num == 0) *out = value;
+			num++;
+		}
+		break;
+	}
+	ber_free(ber, 0);
+
+	return num;
+}
+
 /** Convert a berval to a talloced string
  *
  * The ldap_get_values function is deprecated, and ldap_get_values_len
diff --git a/src/modules/rlm_ldap/groups.c b/src/modules/rlm_ldap/groups.c
index 037727770f6..245c2b81eb2 100644
--- a/src/modules/rlm_ldap/groups.c
+++ b/src/modules/rlm_ldap/groups.c
@@ -276,7 +276,7 @@ static unlang_action_t ldap_group_dn2name_resume(unlang_result_t *p_result, requ
 	fr_ldap_query_t			*query = talloc_get_type_abort(group_ctx->query, fr_ldap_query_t);
 	rlm_ldap_t const		*inst = group_ctx->inst;
 	LDAPMessage			*entry;
-	struct berval			**values = NULL;
+	struct berval			value;
 	int				ldap_errno;
 	rlm_rcode_t			rcode = RLM_MODULE_OK;
 	fr_pair_t			*vp;
@@ -304,15 +304,14 @@ static unlang_action_t ldap_group_dn2name_resume(unlang_result_t *p_result, requ
 		goto finish;
 	}
 
-	values = ldap_get_values_len(query->ldap_conn->handle, entry, inst->group.obj_name_attr);
-	if (!values) {
+	if (fr_ldap_entry_value_find(&value, query->ldap_conn->handle, entry, inst->group.obj_name_attr) <= 0) {
 		REDEBUG("No %s attributes found in object", inst->group.obj_name_attr);
 		rcode = RLM_MODULE_INVALID;
 		goto finish;
 	}
 
 	MEM(vp = fr_pair_afrom_da(group_ctx->list_ctx, inst->group.cache_da));
-	fr_pair_value_bstrndup(vp, values[0]->bv_val, values[0]->bv_len, true);
+	fr_pair_value_bstrndup(vp, value.bv_val, value.bv_len, true);
 	fr_pair_append(&group_ctx->groups, vp);
 	RDEBUG2("Group DN \"%s\" resolves to name \"%pV\"", *group_ctx->dn, &vp->data);
 
@@ -323,7 +322,6 @@ finish:
 	 */
 	group_ctx->dn++;
 
-	if (values) ldap_value_free_len(values);
 	talloc_free(query);
 
 	RETURN_UNLANG_RCODE(rcode);
@@ -712,19 +710,17 @@ static unlang_action_t ldap_cacheable_groupobj_resume(unlang_result_t *p_result,
 		}
 
 		if (inst->group.cacheable_name) {
-			struct berval **values;
+			struct berval value;
 
-			values = ldap_get_values_len(query->ldap_conn->handle, entry, inst->group.obj_name_attr);
-			if (!values) continue;
+			if (fr_ldap_entry_value_find(&value, query->ldap_conn->handle, entry,
+						     inst->group.obj_name_attr) <= 0) continue;
 
 			MEM(pair_append_control(&vp, inst->group.cache_da) == 0);
-			fr_pair_value_bstrndup(vp, values[0]->bv_val, values[0]->bv_len, true);
+			fr_pair_value_bstrndup(vp, value.bv_val, value.bv_len, true);
 
 			RINDENT();
 			RDEBUG2("control.%pP", vp);
 			REXDENT();
-
-			ldap_value_free_len(values);
 		}
 	} while ((entry = ldap_next_entry(query->ldap_conn->handle, entry)));
 
@@ -1116,7 +1112,7 @@ static unlang_action_t ldap_check_userobj_resume(unlang_result_t *p_result, requ
 	 */
 	if (group_ctx->query) {
 		char		*buff;
-		struct berval	**values = NULL;
+		struct berval	name_value;
 
 		switch (group_ctx->query->ret) {
 		case LDAP_RESULT_SUCCESS:
@@ -1139,16 +1135,15 @@ static unlang_action_t ldap_check_userobj_resume(unlang_result_t *p_result, requ
 			RETURN_UNLANG_INVALID;
 		}
 
-		values = ldap_get_values_len(group_ctx->query->ldap_conn->handle, entry, inst->group.obj_name_attr);
-		if (!values) {
+		if (fr_ldap_entry_value_find(&name_value, group_ctx->query->ldap_conn->handle, entry,
+					     inst->group.obj_name_attr) <= 0) {
 			REDEBUG("No %s attributes found in object", inst->group.obj_name_attr);
 			RETURN_UNLANG_INVALID;
 		}
 
-		MEM(buff = talloc_bstrndup(group_ctx, values[0]->bv_val, values[0]->bv_len));
+		MEM(buff = talloc_bstrndup(group_ctx, name_value.bv_val, name_value.bv_len));
 		RDEBUG2("Group DN \"%pV\" resolves to name \"%pV\"", fr_box_strvalue_buffer(group_ctx->lookup_dn),
-			fr_box_strvalue_len(values[0]->bv_val, values[0]->bv_len));
-		ldap_value_free_len(values);
+			fr_box_strvalue_len(name_value.bv_val, name_value.bv_len));
 
 		if (group_ctx->resolving_value) {
 			value_name = buff;
diff --git a/src/modules/rlm_ldap/profile.c b/src/modules/rlm_ldap/profile.c
index a91e67ededd..ba15fdfbf03 100644
--- a/src/modules/rlm_ldap/profile.c
+++ b/src/modules/rlm_ldap/profile.c
@@ -78,7 +78,7 @@ static void ldap_profile_entry_map(bool *fallthrough, request_t *request, ldap_p
 	}
 
 	if (profile_ctx->inst->profile.fallthrough_attr) {
-		struct berval		**values;
+		struct berval		value_bv;
 		int			count;
 		char			*value;
 		xlat_exp_head_t		*cond_expr = NULL;
@@ -95,14 +95,14 @@ static void ldap_profile_entry_map(bool *fallthrough, request_t *request, ldap_p
 			.at_runtime = true,
 		};
 
-		values = ldap_get_values_len(handle, entry, profile_ctx->inst->profile.fallthrough_attr);
-		count = ldap_count_values_len(values);
-		if (count == 0) goto free_values;
+		count = fr_ldap_entry_value_find(&value_bv, handle, entry,
+						 profile_ctx->inst->profile.fallthrough_attr);
+		if (count <= 0) return;
 		if (count > 1) {
 			RWARN("%s returned more than 1 value.  Only evaluating the first.",
 			      profile_ctx->inst->profile.fallthrough_attr);
 		}
-		value = fr_ldap_berval_to_string(request, values[0]);
+		value = fr_ldap_berval_to_string(request, &value_bv);
 
 		RDEBUG3("Parsing fallthrough condition %s", value);
 		if (xlat_tokenize_expression(request, &cond_expr,
@@ -129,8 +129,6 @@ static void ldap_profile_entry_map(bool *fallthrough, request_t *request, ldap_p
 	free:
 		talloc_free(value);
 		talloc_free(cond_expr);
-	free_values:
-		ldap_value_free_len(values);
 	}
 }
 
diff --git a/src/modules/rlm_ldap/user.c b/src/modules/rlm_ldap/user.c
index 0690a20ab60..d351bf1e066 100644
--- a/src/modules/rlm_ldap/user.c
+++ b/src/modules/rlm_ldap/user.c
@@ -211,44 +211,38 @@ unlang_action_t rlm_ldap_find_user_async(TALLOC_CTX *ctx,
  */
 ldap_access_state_t rlm_ldap_check_access(rlm_ldap_t const *inst, request_t *request, LDAPMessage *entry)
 {
-	ldap_access_state_t ret = LDAP_ACCESS_ALLOWED;
-	struct berval **values = NULL;
+	struct berval value;
 
-	values = ldap_get_values_len(fr_ldap_handle_thread_local(), entry, inst->user.obj_access_attr);
-	if (values) {
+	if (fr_ldap_entry_value_find(&value, fr_ldap_handle_thread_local(), entry,
+				     inst->user.obj_access_attr) > 0) {
 		size_t negate_value_len = talloc_strlen(inst->user.access_value_negate);
 		if (inst->user.access_positive) {
-			if ((values[0]->bv_len >= negate_value_len) &&
-			    (strncasecmp(values[0]->bv_val, inst->user.access_value_negate, negate_value_len) == 0)) {
+			if ((value.bv_len >= negate_value_len) &&
+			    (strncasecmp(value.bv_val, inst->user.access_value_negate, negate_value_len) == 0)) {
 				REDEBUG("\"%s\" attribute exists but is set to '%s' - user locked out",
 				        inst->user.obj_access_attr, inst->user.access_value_negate);
-				ret = LDAP_ACCESS_DISALLOWED;
-				goto done;
+				return LDAP_ACCESS_DISALLOWED;
 			}
 			/* RLM_MODULE_OK set above... */
-		} else if ((values[0]->bv_len < negate_value_len) ||
-		           (strncasecmp(values[0]->bv_val, inst->user.access_value_negate, negate_value_len) != 0)) {
+		} else if ((value.bv_len < negate_value_len) ||
+		           (strncasecmp(value.bv_val, inst->user.access_value_negate, negate_value_len) != 0)) {
 			REDEBUG("\"%s\" attribute exists - user locked out", inst->user.obj_access_attr);
-			ret = LDAP_ACCESS_DISALLOWED;
-			goto done;
+			return LDAP_ACCESS_DISALLOWED;
 		}
 		{
 			size_t suspend_value_len = talloc_strlen(inst->user.access_value_suspend);
-			if ((values[0]->bv_len == suspend_value_len) &&
-			    (strncasecmp(values[0]->bv_val, inst->user.access_value_suspend, suspend_value_len) == 0)) {
+			if ((value.bv_len == suspend_value_len) &&
+			    (strncasecmp(value.bv_val, inst->user.access_value_suspend, suspend_value_len) == 0)) {
 				RIDEBUG("\"%s\" attribute exists and indicates suspension", inst->user.obj_access_attr);
-				ret = LDAP_ACCESS_SUSPENDED;
-				goto done;
+				return LDAP_ACCESS_SUSPENDED;
 			}
 		}
-	done:
-		ldap_value_free_len(values);
 	} else if (inst->user.access_positive) {
 		REDEBUG("No \"%s\" attribute - user locked out", inst->user.obj_access_attr);
-		ret = LDAP_ACCESS_DISALLOWED;
+		return LDAP_ACCESS_DISALLOWED;
 	}
 
-	return ret;
+	return LDAP_ACCESS_ALLOWED;
 }
 
 /** Verify we got a password from the search