LDB hidden memory leaks

Kamen Mazdrashki kamenim at gmail.com
Sun Jul 11 17:44:03 MDT 2010


Hi Andrew,

Thanks a lot for your comments!

On Mon, Jul 12, 2010 at 01:17, Andrew Bartlett <abartlet at samba.org> wrote:

> On Sun, 2010-07-11 at 21:32 +0300, Kamen Mazdrashki wrote:
> >
> > After Metze's comment I've updated the last commit:
> >
> http://gitweb.samba.org/?p=kamenim/samba.git;a=shortlog;h=refs/heads/ldb-mem-fix
> >
> >
> > As I want to warn not only in Samba's source base that those functions
> > are deprecated,
> > I've copy&paste the whole snippet for _DEPRECATED_ from replace.h
>
> OK, so this is because we need _DEPRECATED_ to work in code that isn't
> Samba, and while ldb uses replace.h, it does not expose replace.h into
> it's caller's namespace.
>

Correct [ref1]


>
> I looked at
>
> http://gitweb.samba.org/?p=kamenim/samba.git;a=commitdiff;h=35a3455f1a0062d77ba7e03d4da2bd6eab7bb040(s4-ldb: Implement ldb_msg_diff_ex() function to accept a memory context
> from client) and I wondered why you didn't do it in the same commit as
> http://gitweb.samba.org/?p=kamenim/samba.git;a=commitdiff;h=c3db036163bbb780047dd69725eae01e94825cf7(s4-ldb: Refactor ldb_msg_diff() to be based on ldb_msg_diff_ex()
> implementatoin) to show that you are not really changing the code, just
> adding a memory context.


I thing the answer to this question lies in the next comment of yours ;)
I wanted the commits to be as atomic as possible so we can discuss them
separately
(and it is easier to amend short commit that a larger one).
It is not only adding a memory context here but also changing the memory
context
resulting message is allocated in - now it is allocated in NULL context.
As I wrote in commit message, I want this to appear as memory leak in talloc
reports
if returned message is left as it is, without changing memory context.
[ref2]


> However, looking at this commit, I do have one problem:  You should not
> change which memory context ldb_msg_diff() uses.  It is now a deprecated
> function, so you can assume that smart callers are no longer using it -
> but older callers may still assume that the result will go away when the
> ldb does, so you can't use a NULL context here.
>

You are correct.
I did it because:
1. I can't be sure the "deprecated" commit will be pushed as I propose.
   We may decide Metze's suggestion is better. And if the LDB users doesn't
have
   _DEPRECATE_ defined properly, they will not notice something is wrong.
   See [ref1] :)
2. "smart" caller is very vague you know. Are we in Samba "smart callers"?
   I think we are :).
   Despite our "smartness" though, a quick look into build log shows we have
2 or 3
   "this function is deprecated" warning that nobody does anything to fix
this. And those
   warning are there for months.
   I still thing we are "smart callers", we just don't have enough time to
fix this
   (and it is working, so nobody wants to spend time on something that is
working)
   On the other hand - if those "deprecated" functions start leaking
memory,
   or produce segfaults, them I am pretty sure someone will fix them asap.
   I can continue giving examples in this direction, but I thing my point is
clear now :)
   In resume, I think that if we notice some part of the code behaves in not
a nice way,
   then we should make sure it is  as visible as possible.



> The same applies to:
>
> http://gitweb.samba.org/?p=kamenim/samba.git;a=commitdiff;h=7ce36261a7b17b8485924021971b44ef4314edca
> (s4-ldb: Add ldb_msg_canonicalize_ex() to accept a memory context from
> client) and
>
> http://gitweb.samba.org/?p=kamenim/samba.git;a=commitdiff;h=694fef30d8e5f4f9a0122bb24f8a339938e50a57
> (s4-ldb: Refactor ldb_msg_canonicalize() to be based on
> ldb_msg_canonicalize_ex() ...)
>
>
Same as with ldb_msg_diff_ex() applies here.

-- 
CU,
Kamen


More information about the samba-technical mailing list