Re: ldap_msgfree() when ldap_search_s() failed

[Brian Candler <B.Candler@pobox.com>]

On Thu, Jun 02, 2005 at 06:26:20PM +0900, Kazuaki Anami wrote:
> > Can you be more specific about "success or failure"?
> 
> I think only the case ldap_search_s() returned LDAP_SUCCESS is 
> "success". The other cases, LDAP_NO_SUCH_OBJECT for example, are
> "failure".
> 
> > If I understand rightly, ldap_result() sets a pointer to an LDAPMessage 
> > structure, in the event that a message is available. If it does, you have to 
> > free it. This will be the case even if your search returns zero entries, for 
> > instance (which is a "successful" search).
> 
> Yes, the case no entry found in directory causes the problem.
> 
> I agree the case no entry found is a successful search, but
> ldap_search_s() doesn't return LDAP_SUCCESS. 

OK, but the LDAP protocol still sends back a result message from the server.
It can, for example, have an information string embedded in it.

If you look in the source at libraries/libldap/search.c, you'll see that
ldap_search_s is very simple:

int
ldap_search_s(
	LDAP *ld,
	LDAP_CONST char *base,
	int scope,
	LDAP_CONST char *filter,
	char **attrs,
	int attrsonly,
	LDAPMessage **res )
{
	int	msgid;

	if ( (msgid = ldap_search( ld, base, scope, filter, attrs, attrsonly ))
	    == -1 )
		return( ld->ld_errno );

	if ( ldap_result( ld, msgid, 1, (struct timeval *) NULL, res ) == -1 )
		return( ld->ld_errno );

	return( ldap_result2error( ld, *res, 0 ) );
}

Now, at first glance this API is very poor. If ldap_search fails or
ldap_result fails (either returns -1) then no memory is allocated, and you
get an ld_errno value returned. Otherwise, memory is allocated from
ldap_result, and you get ldap_result2error returned.

However, it seems that API errors (like LDAP_NO_MEMORY or LDAP_FILTER_ERROR)
are negative, whereas errors returned in messages from the server are
positive.

So one approach is to modify your code:

>     if ((rv = ldap_search_s(..., msg)) != LDAP_SUCCESS) {
>     	puts(ldap_err2string(rv));
>     	return rv;
>     }

to, say,

      if ((rv = ldap_search_s(..., msg)) != LDAP_SUCCESS) {
      	puts(ldap_err2string(rv));
        if (rv > 0) ldap_msgfree(msg);
      	return rv;
      }

But being a bit cleverer here, if rv > 0 you would call ldap_parse_result on
msg first, to extract extra detail to display. See print_result() in
clients/tools/ldapsearch.c for an example.

This is still icky though; maybe you're better off calling ldap_search and
ldap_result explicitly yourself.

Perhaps another approach is to set msg to NULL before the call, and free the
memory only if it's not NULL afterwards. I don't know if that approach is
"blessed" by the API.

I think this definitely could be made clearer in the documentation; you
shouldn't have to read the source code to work out how to use an API.

Regards,

Brian.