Hi Theo,

Thanks for the review and your feedback.

The ldap_free_elements_and_respond() helper was added to avoid
repeating the cleanup pattern and to follow the style of
ldap_respond(). That said, I understand your preference for
inlining, especially since it's only needed in a few places.

I've updated the code accordingly. The revised diff is attached.

Best regards,
Anton Kasimov

10.05.2025 12:31, Theo Buehler пишет:
> On Sat, May 10, 2025 at 11:41:25AM +0200, Anton Kasimov wrote:
>> Hi tech@,
>>
>> This patch fixes several memory leaks in ldapd caused by missing calls
>> to ober_free_elements after using namespace_get. The namespace_get
>> function eventually calls ober_read_element, which allocates memory
>> for parsed BER elements.
>>
>> The most significant leak happens during processing of bind requests
>> from non-root users in auth.c. The returned ber_element was not freed
>> after being processed.
>>
>> Similar issues were found and fixed in ldape.c and modify.c. In these
>> cases, ber_element returned by namespace_get was also left unfreed.
>>
>> Additionally, minor cosmetic cleanups were made to simplify some
>> NULL checks. Since ober_free_elements safely handles NULL arguments,
>> explicit pointer checks before calling it are unnecessary and were
>> removed.
>>
>> Tested with multiple bind and modify operations to confirm no
>> regressions or new leaks.
>>
>> The diff is in the attachment.
> Thanks. This all looks correct to me. Just one comment:
>
>> @@ -205,6 +202,14 @@ ldap_respond(struct request *req, int co
>>   }
>>   
>>   int
>> +ldap_free_elements_and_respond(struct ber_element *elm, struct request *req,
>> +	int code)
>> +{
>> +	ober_free_elements(elm);
>> +	return ldap_respond(req, code);
>> +}
>> +
>> +int
>>   ldap_abandon(struct request *req)
>>   {
>>   	long long	 msgid;
>> @@ -266,19 +271,22 @@ ldap_compare(struct request *req)
>>   		return ldap_respond(req, LDAP_NO_SUCH_OBJECT);
>>   
>>   	if ((attr = ldap_find_attribute(entry, at)) == NULL)
>> -		return ldap_respond(req, LDAP_NO_SUCH_ATTRIBUTE);
>> +		return ldap_free_elements_and_respond(entry, req,
>> +			LDAP_NO_SUCH_ATTRIBUTE);
> I understand why you added ldap_free_elements_and_respond() but it feels
> a bit odd to me.  I would prefer not adding it and simply inlining it
> the few times you use it, like this:
>
> 	if ((attr = ldap_find_attribute(entry, at)) == NULL) {
> 		ober_free_elements(entry);
> 		return ldap_respond(req, LDAP_NO_SUCH_ATTRIBUTE);
> 	}
>
> 	if ((attr = attr->be_next) == NULL) {	/* skip attribute name */
> 		ober_free_elements(entry);
> 		return ldap_respond(req, LDAP_OTHER);
> 	}
>
> etc.
Index: auth.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/auth.c,v
diff -u -p -u -p -r1.15 auth.c
--- auth.c	29 Jun 2022 09:10:13 -0000	1.15
+++ auth.c	11 May 2025 14:26:02 -0000
@@ -301,7 +301,7 @@ ldap_auth_simple(struct request *req, ch
 	char			*password;
 	char			*user_password;
 	struct namespace	*ns;
-	struct ber_element	*elm = NULL, *pw = NULL;
+	struct ber_element	*entry = NULL, *elm = NULL, *pw = NULL;
 
 	if (*binddn == '\0') {
 		free(req->conn->binddn);		/* anonymous bind */
@@ -337,15 +337,15 @@ ldap_auth_simple(struct request *req, ch
 		    NULL, LDAP_SCOPE_BASE))
 			return LDAP_INSUFFICIENT_ACCESS;
 
-		elm = namespace_get(ns, binddn);
-		if (elm == NULL && errno == ESTALE) {
+		entry = namespace_get(ns, binddn);
+		if (entry == NULL && errno == ESTALE) {
 			if (namespace_queue_request(ns, req) != 0)
 				return LDAP_BUSY;
 			return -1;	/* Database is being reopened. */
 		}
 
-		if (elm != NULL)
-			pw = ldap_get_attribute(elm, "userPassword");
+		if (entry != NULL)
+			pw = ldap_get_attribute(entry, "userPassword");
 		if (pw != NULL) {
 			for (elm = pw->be_next->be_sub; elm;
 			    elm = elm->be_next) {
@@ -356,6 +356,7 @@ ldap_auth_simple(struct request *req, ch
 					break;
 			}
 		}
+		ober_free_elements(entry);
 	}
 
 	free(req->conn->binddn);
Index: ldape.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldape.c,v
diff -u -p -u -p -r1.38 ldape.c
--- ldape.c	17 Jan 2024 08:28:15 -0000	1.38
+++ ldape.c	11 May 2025 14:26:02 -0000
@@ -105,8 +105,7 @@ send_ldap_extended_response(struct conn 
 
 	return;
 fail:
-	if (root)
-		ober_free_elements(root);
+	ober_free_elements(root);
 }
 
 int
@@ -180,10 +179,8 @@ ldap_refer(struct request *req, const ch
 	return LDAP_REFERRAL;
 
 fail:
-	if (root != NULL)
-		ober_free_elements(root);
-	if (ref_root != NULL)
-		ober_free_elements(ref_root);
+	ober_free_elements(root);
+	ober_free_elements(ref_root);
 	request_free(req);
 	return LDAP_REFERRAL;
 }
@@ -265,19 +262,28 @@ ldap_compare(struct request *req)
 	if ((entry = namespace_get(ns, dn)) == NULL)
 		return ldap_respond(req, LDAP_NO_SUCH_OBJECT);
 
-	if ((attr = ldap_find_attribute(entry, at)) == NULL)
+	if ((attr = ldap_find_attribute(entry, at)) == NULL) {
+		ober_free_elements(entry);
 		return ldap_respond(req, LDAP_NO_SUCH_ATTRIBUTE);
+	}
 
-	if ((attr = attr->be_next) == NULL)	/* skip attribute name */
+	if ((attr = attr->be_next) == NULL) {	/* skip attribute name */
+		ober_free_elements(entry);
 		return ldap_respond(req, LDAP_OTHER);
+	}
 
 	for (elm = attr->be_sub; elm != NULL; elm = elm->be_next) {
-		if (ober_get_string(elm, &s) != 0)
+		if (ober_get_string(elm, &s) != 0) {
+			ober_free_elements(entry);
 			return ldap_respond(req, LDAP_OTHER);
-		if (strcasecmp(value, s) == 0)
+		}
+		if (strcasecmp(value, s) == 0) {
+			ober_free_elements(entry);
 			return ldap_respond(req, LDAP_COMPARE_TRUE);
+		}
 	}
 
+	ober_free_elements(entry);
 	return ldap_respond(req, LDAP_COMPARE_FALSE);
 }
 
Index: modify.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/modify.c,v
diff -u -p -u -p -r1.24 modify.c
--- modify.c	20 Dec 2021 13:26:11 -0000	1.24
+++ modify.c	11 May 2025 14:26:02 -0000
@@ -36,7 +36,7 @@ ldap_delete(struct request *req)
 	struct namespace	*ns;
 	struct referrals	*refs;
 	struct cursor		*cursor;
-	struct ber_element	*entry, *elm, *a;
+	struct ber_element	*entry = NULL, *elm, *a;
 	int			 rc = LDAP_OTHER;
 
 	++stats.req_mod;
@@ -114,6 +114,7 @@ ldap_delete(struct request *req)
 		rc = LDAP_SUCCESS;
 
 done:
+	ober_free_elements(entry);
 	btree_cursor_close(cursor);
 	btval_reset(&key);
 	namespace_abort(ns);
@@ -226,8 +227,7 @@ ldap_add(struct request *req)
 	return ldap_respond(req, rc);
 
 fail:
-	if (set != NULL)
-		ober_free_elements(set);
+	ober_free_elements(set);
 	namespace_abort(ns);
 	return ldap_respond(req, LDAP_OTHER);
 }
@@ -384,8 +384,8 @@ ldap_modify(struct request *req)
 		rc = LDAP_OTHER;
 
 done:
-	if (vals != NULL)
-		ober_free_elements(vals);
+	ober_free_elements(vals);
+	ober_free_elements(entry);
 	namespace_abort(ns);
 	return ldap_respond(req, rc);
 }

• ldapd2.diff (4K)