[PATCH] Fix new Coverity IDs

[Jeremy Allison]

On Wed, Jan 11, 2017 at 08:24:26AM +0100, Volker Lendecke wrote:
> On Wed, Jan 11, 2017 at 07:56:04AM +0100, Andreas Schneider wrote:
> > > diff --git a/source3/winbindd/winbindd_samr.c
> > > b/source3/winbindd/winbindd_samr.c index dd674965f17..d2ce1d2d5e2 100644
> > > --- a/source3/winbindd/winbindd_samr.c
> > > +++ b/source3/winbindd/winbindd_samr.c
> > > @@ -176,8 +176,6 @@ static NTSTATUS sam_query_user_list(struct
> > > winbindd_domain *domain,
> > > 
> > >         DEBUG(3,("samr_query_user_list\n"));
> > > 
> > > -       *prids = NULL;
> > > -
> > >         tmp_ctx = talloc_stackframe();
> > >         if (tmp_ctx == NULL) {
> > >                 return NT_STATUS_NO_MEMORY;
> > > 
> > > instead ?
> > 
> > Yes, and we need backports to 4.6 which has already been branched. So could 
> > someone please create a bug for them?
> 
> This comes from previous code where "num_entries" was unconditionally
> initialized always. Now we have rids only as a talloc array carrying
> its own length, no "pnum_rids" anymore.

Just looked at the (one) caller:

source3/winbindd/winbindd_cache.c:

1498                 rids = NULL;
1499                 status = domain->backend->query_user_list(domain, mem_ctx,
1500                                                           &rids);
1501                 num_rids = talloc_array_length(rids);

so 'rids' is already initialized to NULL, but we then
call talloc_array_length(rids) without checking if
the return was NULL. This works (I think), but goes
via:

#define talloc_array_length(ctx) (talloc_get_size(ctx)/sizeof(*ctx))

and talloc_get_size() is:

_PUBLIC_ size_t talloc_get_size(const void *context)
{
        struct talloc_chunk *tc;

        if (context == NULL) {
                context = null_context;
        }
        if (context == NULL) {
                return 0;
        }

        tc = talloc_chunk_from_ptr(context);

        return tc->size;
}

- which uses null_context - which can change what it returns
if someone ever calls talloc_enable_null_tracking() !!!!!

Oh. And winbindd *does* do that :-(.

from source3/winbindd/winbindd.c:

1543         /*
1544          * Do this before any other talloc operation
1545          */
1546         talloc_enable_null_tracking();

So does nmbd and smbd.

That's really ugly, shitty code in talloc. Changing it is
an ABI change though :-(.

git show d1895d2fee9e

Metze - what were you *thinking* (although this has been in the code
since 2009 so I guess forgiveness is in order) ?

--------------------------------------------------------------------------------
commit d1895d2fee9eec6f0b16dd38728b34d2f8233b0c
Author: Stefan Metzmacher <metze at samba.org>
Date:   Mon Jul 13 18:51:24 2009 +0200

    talloc: let talloc_total_blocks() and talloc_get_size() operate on the null_context
    
    metze

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index 0126567c45a..8e91b60c99b 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1227,7 +1227,16 @@ size_t talloc_total_size(const void *ptr)
 size_t talloc_total_blocks(const void *ptr)
 {
        size_t total = 0;
-       struct talloc_chunk *c, *tc = talloc_chunk_from_ptr(ptr);
+       struct talloc_chunk *c, *tc;
+
+       if (ptr == NULL) {
+               ptr = null_context;
+       }
+       if (ptr == NULL) {
+               return 0;
+       }
+
+       tc = talloc_chunk_from_ptr(ptr);
 
        if (tc->flags & TALLOC_FLAG_LOOP) {
                return 0;
@@ -1807,8 +1816,12 @@ size_t talloc_get_size(const void *context)
 {
        struct talloc_chunk *tc;
 
-       if (context == NULL)
+       if (context == NULL) {
+               context = null_context;
+       }
+       if (context == NULL) {
                return 0;
+       }
 
        tc = talloc_chunk_from_ptr(context);
--------------------------------------------------------------------------------

This might be the cause of *MANY* subtle bugs, so we need
to address this I think.

I think the safest thing to do right now is ensure we don't
call talloc_array_length(NULL) here by doing (not a git-am
fix as I think we need to discuss this).

diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c
index 4431cb52b64..39ff14824de 100644
--- a/source3/winbindd/winbindd_cache.c
+++ b/source3/winbindd/winbindd_cache.c
@@ -1498,7 +1498,11 @@ do_query:
 		rids = NULL;
 		status = domain->backend->query_user_list(domain, mem_ctx,
 							  &rids);
-		num_rids = talloc_array_length(rids);
+		if (rids != NULL) {
+			num_rids = talloc_array_length(rids);
+		} else {
+			num_rids == NULL;
+		}
 
 		if (!NT_STATUS_IS_OK(status)) {
 			DEBUG(3, ("query_user_list: returned 0x%08x, "
-------------- next part --------------
diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c
index 4431cb52b64..39ff14824de 100644
--- a/source3/winbindd/winbindd_cache.c
+++ b/source3/winbindd/winbindd_cache.c
@@ -1498,7 +1498,11 @@ do_query:
 		rids = NULL;
 		status = domain->backend->query_user_list(domain, mem_ctx,
 							  &rids);
-		num_rids = talloc_array_length(rids);
+		if (rids != NULL) {
+			num_rids = talloc_array_length(rids);
+		} else {
+			num_rids == NULL;
+		}
 
 		if (!NT_STATUS_IS_OK(status)) {
 			DEBUG(3, ("query_user_list: returned 0x%08x, "