Hello bind developers!

I wanted to report a bug we found in sdbldap.c recently. I know sdb is a contrib module so I am not sure if it is officially supported by the bind-bugs maintainers the original author.

In short, several bugs could potentially be fixed by correcting a few "ism's" related to potentially unsafe use of "free" in contrib/sdb/ldap/ldapdb.c by adding appropriate if ( ... != NULL) checks before things like free(conndata->index) at line 136... which is the location that caused our particular crash, and free(threaddata->index) at line 133, etc.

For best cross-platform/cross-compiler compatibility it would probably also be good practice to be setting conndata->index = NULL after calling free(conndata->index) so the if ( ... != NULL) checks would always prevent double-freeing of blocks if the implementation of "free" doesn't happen to also zero out the pointer on all platforms.

I am referring to what I hope is the latest code here:

https://source.isc.org/cgi-bin/gitweb.cgi?p=bind9.git;a=blob;f=contrib/sdb/ldap/ldapdb.c;h=caade37c80e0a7ae8320d6faa3e921e094a90022;hb=HEAD

The crash we encountered specifically occurred at line 136 below when free(conndata->index) tried to free an invalid pointer. Unfortunately the code optimizer had optimized away conndata->index so I could not tell if it was NULL or just a double-free of a dangling pointer reference. But as soon as I looked at the code and saw that they were checking for NULL sometime and not other times, that seemed like a likely cause & good place for a NULL check.

 127         if (data == NULL) {

 128                 /* cleanup */

 129                 /* lock out other threads */

 130                 ldapdb_lock(1);

 131                 while (allthreadsdata != NULL) {

 132                         threaddata = allthreadsdata;

 133                         free(threaddata->index);

 134                         while (threaddata->data != NULL) {

 135                                 conndata = threaddata->data;

 136                                 free(conndata->index);

 137                                 if (conndata->data != NULL)

 138                                         ldap_unbind((LDAP *)conndata->data);

 139                                 threaddata->data = conndata->next;

 140                                 free(conndata);

 141                         }

 142                         allthreadsdata = threaddata->next;

 143                         free(threaddata);

 144                 }

 145                 ldapdb_lock(-1);

 146                 return (NULL);

 147         }

I could supply a coredump file, but would probably be difficult to debug unless you have an RHEL 6.5 machine with latest patches to debug with, so I thought it would be better just to describe what appears to be a somewhat pervasive problem in several places in this particular module.

If you would rather I compile up the latest code & try to make the fixes myself, and then submit a patch against the latest code repo (assuming I am looking at the latest repo) let me know and I would be glad to contribute.

Thanks for your support & contributions to the internet community.  Your efforts are appreciated!

Troy