Fixes to LDB memory handling

Andrew Bartlett abartlet at samba.org
Thu Dec 16 22:48:55 MST 2010


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?

Thanks,

Andrew Bartlett


More information about the samba-technical mailing list