Re: Tim Howes' 03.1.c: ldap_memfree/ber_free crashes

["Dr.Rich Artym" <openldap-bugs-mail@galacta.demon.co.uk>]

Thanks for all that info, Kurt.  No doubt I'll sort all this out in
my mind over the next few weeks.

In message <3.0.5.32.19990608230920.009f1c60@localhost>, Kurt wrote:

> >[As for the core dump in ber_free(), I triggered an actual bug
> > in 1.2.3, I presume?  Certainly the 03.1.c code seems innocuous.]
>
> No.  Crashing in chunk free implies you called ber_free() on a
> BerElement which was freed by the API.  Don't expect Netscape
> SDK semantics from OpenLDAP 1.x.

That's an interesting point which I think would benefit from examples,
because the semantics aren't obvious from the function prototypes in
this API.  As an example which might help, prog 03.1.c has the general
structure (most args removed for clarity):

| 1|        ldap_init
| 2|        ldap_simple_bind_s
| 3|        ldap_search_s
| 4|        entry = ldap_first_entry<result>
| 5|        WHILE entry != NULL {
| 6|            attrib = ldap_first_attribute<entry, &ber>
| 7|            WHILE attrib != NULL {
| 8|                values = ldap_get_values<entry, attrib>
| 9|                ldap_value_free<values>
|10|                ldap_memfree<attrib>
|11|                attrib = ldap_next_attribute<entry, ber>
|12|            }
|13|            if ber != NULL {
|14|                ber_free<ber, 0>
|15|            }   
|16|            entry = ldap_next_entry<entry>
|17|        }
|18|        ldap_msgfree<result>
|19|        ldap_unbind


The above suggests that ber_free() is only ever called a maximum of
one time per &ber allocation by ldap_first_attribute(), so it should
be impossible for a repeat ber_free() to occur on a given ber as long
as there are no ber_free()'s occurring elsewhere as side effects.
However, there *is* one well-documented side effect that breaks this.

>From the OpenLDAP ldap_first_attribute(3) man page, the side effect
that occurs is in ldap_next_attribute(), which frees the ber when it
returns NULL when there are no more attributes.  Consequently, if I
understand this correctly, ber_free() gets called a second time in
Tim Howes' code because his ber pointer is not NULLed by "next".
Maybe he expects ber_free() to be safe if invoked repeatedly on a ber.

Is that one semantic difference to which you were referring?  The
implied OpenLDAP semantic is of course that ber_free() should not be
called if ldap_next_attribute() ever returned a NULL for that ber.
In other words, one has to be aware of the past history of "next"
calls in OpenLDAP in order to be able to invoke ber_free() safely.

The above shows an inconsistency in the API.  If ldap_first_attribute()
writes to &ber and ldap_next_attribute() performs a ber_free() at the
end, then the latter should also write a NULL to &ber on termination,
otherwise that state variable is left in a state that is inconsistent
with the underlying first/next machinery.  This implies of course that
&ber would need to be stored by "first" for later use by "next".  Maybe
the two SDKs that Howes uses (Netscape and U-Mich) do precisely this.

The whole "&ber" thing is rather horrid though, presumably a legacy
from the early U-Mich days.  In a clean new API one would strive
for multi-point traversal of the graph with no hidden state and no
unsafe side effects.  Maybe I'll write such a thing one day. :-)

Thanks again for your comments, Kurt!

Rich Artym
rich@galacta.demon.co.uk