RE: memory leaks (Warning: L-O-N-G response, discussion solicited )

["MCGARRAHAN, KENNETH L (SWBT)" <km4155@momail.sbc.com>]

Funny this post should appear now, as we were just trying to resolve a
memory leak appearing  after a series of calls to ldap_first_attribute() and
ldap_next_attribute(). In examining the libldap library, there appears to be
an inconsistency in how the BerElement *ber is handled by these functions. 

In the normal case, ldap_first_attribute() will allocate a BerElement
structure and assign the address of the allocated storage to the BerElement
*ber. In the normal case for ldap_next_attribute(), this BerElement
structure address is passed in (NOT BerElement **ber) and used to traverse
the list of attributes. 

The inconsistency I referenced in the opening is the handling of the
BerElement *ber, and what SHOULD happen in the case of error. 

The LDAP API specification (RFC1823?) is quiet on the handling of this
allocated memory, except to say that it is allocated on the
ldap_first_attribute() call and used on subsequent ldap_next_attribute()
calls.  "LDAP - Programming Directory-Enabled Applications with Lightweight
Directory Access Protocol" [Tim Howes & Mark Smith, Macmillan 1997] is
explicit in describing how to deal with this memory:

"If BER is non-NULL after the final call in a sequence of calls to
ldap_first_attribute() and ldap_next_attribute(), it should be freed by
calling ber_free() with a second argument of zero."

As currently implemented in the OpenLDAP libraries, ldap_first_attribute()
and ldap_next_attribute() do not use this approach, but use an approach that
leaves the developer with a BerElement *ber in an unknown state. Within
ldap_first_attribute(), if the subsequent ber_scanf() call fails, the
allocated structure is freed with a call to ber_free(), BUT the BerElement *
is not reset to NULL. 

char *
ldap_first_attribute( LDAP *ld, LDAPMessage *entry, BerElement **ber )
{
	long	len;

	if ( (*ber = ldap_alloc_ber_with_options( ld )) == NULLBER ) {
		return( NULL );
	}

	**ber = *entry->lm_ber;

	len = LDAP_MAX_ATTR_LEN;
	if ( ber_scanf( *ber, "{x{{sx}", ld->ld_attrbuffer, &len )
	    == LBER_ERROR ) {
		ld->ld_errno = LDAP_DECODING_ERROR;
		ber_free( *ber, 0 );               /* <---- here's the
problem */
		return( NULL );                    /* <----- *ber = NULL?
*/
	}

	return( ld->ld_attrbuffer );
}

Within ldap_next_attribute(), if the ber_scanf() call fails, the allocated
BerElement structure is freed with a call to ber_free(), BUT BerElement *ber
is not reset to NULL, nor can it be since the pointer is passed in by value.


char *
ldap_next_attribute( LDAP *ld, LDAPMessage *entry, BerElement *ber )
{
	long	len;

	len = LDAP_MAX_ATTR_LEN;
	if ( ber_scanf( ber, "{sx}", ld->ld_attrbuffer, &len ) 
	    == LBER_ERROR ) {
		ld->ld_errno = LDAP_DECODING_ERROR;
		ber_free( ber, 0 );                /* <---- here's the
problem */
		return( NULL );                    /* <---- ber cannot be
set to NULL */
	}

	return( ld->ld_attrbuffer );
}

The result is that, on error from ldap_first_attribute() and
ldap_next_attribute(), the programmer cannot easily tell whether the
allocated BerElement structure has already been freed. Attempting to free()
a pointer previously free()'d has undefined results.

It would seem the most straightforward implementation would place the onus
of freeing the allocated BerElement structure addressed by the BerElement
*ber solely on the programmer, removing any reference to ber_free() from the
ldap_first_attribute() and ldap_next_attribute() calls. The address of the
memory allocated for the BerElement structure, once allocated in
ldap_first_attribute(), can be assigned to the BerElement **ber, and its
handling turned over to the LDAP application developer. In this way, the
programmer has a clear and consistent method of determining the allocation
state of the BerElement structure (NULL or non-NULL pointer). Of course,
changing ldap_first_attribute() and ldap_next_attribute() behavior in this
way might cause problems with applications built around the existing
implementation.

Discussion on this topic is welcomed....

Ken McGarrahan
Southwestern Bell Telephone
314.235.3160

-----Original Message-----
From: Ashely Hornbeck [mailto:ahornbeck@Splitrock.net]
Sent: Monday, January 18, 1999 6:06 PM
To: Openldap-Devel (E-mail)
Subject: memory leaks


First off, thanks for all of the help everyone has already given with my
other questions.


Just to double check.  Could someone confirm that my following assumptions
are correct.

The definitions or below.

ld needs to be freed with ldap_unbind(ld);
result needs to be freed with ldap_msgfree(result);
e does *not* need to be freed!
attr does *not* need to be freed!
vals needs to be freed with ldap_value_free(vals);
ber needs to be freed with ber_free(ber);


LDAP		*ld	=(LDAP*)NULL;			/* ldap handle  */

LDAPMessage	*result	=(LDAPMessage*)NULL;	/* set by ldap_search_st()
*/
LDAPMessage	*e	=(LDAPMessage*)NULL;		/* returned from
ldap_first_entry() */


char		*attr	=(char*)NULL;			/* returned from
lpad_first_attribute() */
char		**vals	=(char**)NULL;		/* returned from
ldap_get_values() */
BerElement	*ber	=(BerElement*)NULL;		/* set by
ldap_first_attribute()


Thanks in advance,
-Ash

-------------------------
Ashley Neal Hornbeck
Splitrock Services, Inc.
281.465.1318