LDB hidden memory leaks

[Andrew Bartlett]

On Mon, 2010-07-12 at 02:44 +0300, Kamen Mazdrashki wrote:
> 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]

I still think the commit would have been clearer if these were combined
- or if it isn't clearer, then we should examine why that isn't the
case. 

>         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] :)

We can assume that this will be dealt with one way or the other
however. 

> 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.

No, it means that another project that uses LDB and had one behaviour,
now has another behaviour because LDB on the user's system was upgraded
by their distributor. This isn't acceptable under the current promises
we make to our users about library versions. 

>    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.

None of this allows us to breach the ABI we have promised.  All we can
do is change the compile-time marking so that a developer knows to use
something else.  We MUST NOT change the semantics of the original
call.  

Andrew Bartlett


-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100712/df4455a9/attachment.pgp>