Fixes to LDB memory handling

simo simo at samba.org
Fri Dec 17 06:29:55 MST 2010


On Fri, 2010-12-17 at 16:48 +1100, Andrew Bartlett wrote:
> Attached are two patches:
>  - A fix to the memory handling of async responses (ares) in the ldb_tdb
> backend
>  - A revert to the patch I put in for the descriptor module to alliviate
> a double-free observed in upgradeprovision error cases.
> 
> Matthias,
> 
> The reason I bring this up on the list is that I'm keen to ensure I
> understand the bug you were hoping to fix with:
> 
> commit 0941099a2839812b18c2d3db86b18e92b152f1c8
> Author: Matthias Dieter Wallnöfer <mdw at samba.org>
> Date:   Wed Oct 20 14:27:04 2010 +0200
> 
>     ldb:ldb_index.c - fix some memory leaks
> 
> @@ -962,6 +966,7 @@ static int ltdb_index_filter(const struct dn_list
> *dn_list,
>  
>                 ret = ldb_module_send_entry(ac->req, msg, NULL);
>                 if (ret != LDB_SUCCESS) {
> +                       talloc_free(msg);
>                         ac->request_terminated = true;
>                         return ret;
>                 }
> 
> 
> In particular, what test should I use to show the leaks you saw here?
> I'm keen to ensure that we don't go around in circles here, as we
> presumably need to fix whatever callback you observed the leak in. 
> 
> I'm confident that reverting this part of your patch is correct, because
> no other ldb backend calls talloc_free() in these circumstances (indeed,
> even the un-indexed ldb_tdb callback did not), but in memory handling,
> I'm particularly keen to understand the full story first.
> 
> It also seems to me that any other solution would break the pre-October
> ABI in any case, as user-supplied callers are entitled to rely on the
> previous behaviour (that is, callback to always free). 
> 
> Nadya,
> 
> As I mention above, I'm also including a revert of the patch I pushed to
> descriptor.c.  It may be one of the few callbacks correctly handling
> memory in the failure case, but I'm pretty sure the original code was
> correct. 
> 
> Simo,
> 
> The async behaviour here is yours originally.  Can you confirm that this
> (ldb_module_send_entry() expects the callback to handle the talloc_free)
> is how it is intended to work?

The handling of frees in that part of the code was done very carefully.
Randomly freeing stuff i there is certainly going to cause issues, with
code going to access freed memory.
In general the code there relies on the fact that all responses are
ultimately child memory of the original request, so there should never
be a memory leak, at most freeing data is deferred to later when the
request is completed.

The free introduced by Matthias is almost certainly wrong.
The msg is stolen on the response structure and passed to the callbacks,
at that point we have no idea what happens to it and no right to free
it.
A callback may have stolen it to save it for future use or whatever
else. The fact we return an error has no bearing on what is the destiny
of that piece of memory. Worst case it is attached to the request and
will be freed soon enough anyway.

Mathias,
please run this kind of changes to the core ldb code through me or
Tridge before pushing.

Andrew, the free you introduced is fine. A callback can do whatever it
wants with a response structure including leaving it alone, it will be
eventually freed when the request is terminated. Of course freeing it
earlier is fine too.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>



More information about the samba-technical mailing list