| Subject: | Crash in named-sdb -- freeing an invalid pointer. |
| Date: | Fri, 19 Sep 2014 23:20:01 -0600 |
| To: | bind-bugs@isc.org |
| From: | Troy Noble <econoplas@gmail.com> |
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:
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