Re: util_ldap [Bug 29217] - Remove references to calloc() and free()

[Brad Nicholes <BN...@novell.com>]

>The LDAP cache is a complex memory structure - each cache entry
consists 
>of a struct with a whole lot of malloc'ed blocks. And there are 
>different kind of cache entries (bind cache, opcache). Deleting an
entry 
>means walking through the struct and relying on a human writing code
to 
>delete each block allocated one by one, and finally to delete the 
>struct. If one of the free()'s are missed, leak.
>
>If there was a single pool per cache entry, to remove the entry, just

>call apr_pool_destroy() once, not free() many times, and be confident

>that the memory is gone and leak free.

I see your point and it would be nice to just make a single
apr_pool_destroy() call rather than traversing the entire node and
freeing each individual structure element.  But the way that the code is
designed today, you might as well redesign and rewrite the entire
cache_mgr because I don't see how you could retrofit it to use
apr_rmm_xxx() alloc'ed memory for the shared memory case and apr_pool
memory for the non-share memory case.  Allocating and destroying the two
types of memory are completely different and trying to maintain two
versions of the cache_mgr would be a nightmare.  Also, I don't believe
that apr_pool supports shared memory.  I could be wrong.  (Sorry for the
redundancy, I think we both agree on these points)

The cache and cache_mgr code today seems very straight forward.  If you
want to destroy the url_node, you call util_ldap_url_node_free().  If
you want to destroy the search_node, you call
util_ldap_search_node_free(), etc.  All virtual allocations and
deallocations are done in either the xxx_node_copy or xxx_node_free
functions and the deallocations are done in a very hierarchical fashion.
 The cache_mgr calls util_ldap_url_node_free() which in turn indirectly
calls each of the xxx_node_free() functions.   As I see it, there is
little room for error.  The cache_mgr itself is written in such a way
that it doesn't really care what kind of memory you stuff into the
payload and destroying the cache struct itself is nothing more than a
free.

Do we even know that there is a problem with this code?  I haven't seen
any memory leaks so far.  I would hate to go to all of the work to
redesign and rewrite the ldap_cache manager for little to no gain.

Brad


Brad Nicholes
Senior Software Engineer
Novell, Inc., the leading provider of Net business solutions
http://www.novell.com