From: Theo Buehler <tb@theobuehler.org>
Subject: Re: Fix memory leaks in ldapd caused by missing ober_free_elements calls
To: Anton Kasimov <kasimov.an@gmail.com>
Cc: tech@openbsd.org
Date: Sat, 10 May 2025 12:31:31 +0200

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.