LDB hidden memory leaks

[Andrew Bartlett]

On Sun, 2010-07-11 at 21:32 +0300, Kamen Mazdrashki wrote:
> 
> 
> On Sat, Jul 10, 2010 at 17:00, Kamen Mazdrashki <kamenim at gmail.com>
> wrote:
>         On Sat, Jul 10, 2010 at 11:16, Stefan (metze) Metzmacher
>         <metze at samba.org> wrote:
>                  
>                  
>                 The way you added this:
>                 
>                 +#ifndef _DEPRECATED_
>                 +#define _DEPRECATED_   __attribute__ ((deprecated))
>                 +#endif
>                 
>                 is not portable, not every compiler supports this.
>                 
>                 The correct definition already comes from replace.h.
>                 So ldb.h only needs this:
>                 
>                 +#ifndef _DEPRECATED_
>                 +#define _DEPRECATED_
>                 +#endif
>         
>         
>         You are right but... this is not what my intention is.
>         I am pretty sure I've caught all places in Samba4 where
>         _deprecated_ 
>         functions are used. In your way we will receive warning only
>         when compiling Samba.
>         I wanted this for the users of LDB (not just Samba).
>         So, how to achieve this?
>         (I thought I did it the same way as with PRINTF_ATTRIBUTE
>         macro, but now
>          I see I am not)
>         
>         
>         Perhaps pasting the whole snippet from replace.h like this?
>         
>         
>         #ifndef _DEPRECATED_
>         #if (__GNUC__ >= 3) && (__GNUC_MINOR__ >= 1 )
>         #define _DEPRECATED_ __attribute__ ((deprecated))
>         #else
>         #define _DEPRECATED_
>         #endif
>         #endif
>         
>         
> 
> 
> 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.  

> Any comments? 

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.  

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. 

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

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/12d0088d/attachment.pgp>