svn commit: samba r9815 - in branches/SOC/SAMBA_4_0/source: libnet torture/rpc

[Brad Henry]

Hi Metze, thanks for the comments! I'm still working through some of 
them, but I have a couple of questions so far:

Stefan (metze) Metzmacher wrote:

>+	r->out.domain_sid = talloc_reference(tmp_ctx, domain_sid);
>+	r->out.domain_name = talloc_reference(tmp_ctx, domain_name);
>+	r->out.realm = talloc_reference(tmp_ctx, realm);
>+	r->out.samr_pipe = talloc_reference(tmp_ctx, samr_pipe);
>+	r->out.samr_binding = talloc_reference(tmp_ctx, samr_binding);
>+	r->out.user_handle = &u_handle;
>+	r->out.error_string = talloc_reference(tmp_ctx, r2.samr_handle.out.error_string);
>using tmp_ctx here will result in a mem leak
>as you just increase the reference count,
>as this values are supposed to be passed to the caller
>this should be talloc_reference(mem_ctx, ...);
>this will create a 2nd reference to the values from the callers TALLOC_CTX
>and they get free'ed when the tmp_ctx AND the mem_ctx are free'ed.
>
>but also in this case you can also use talloc_steal(mem_ctx, ...)
>as talloc_steal() can never fail with a alloc failure,
>where talloc_reference can!
>
>and as we know that we'll destroy the tmp_ctx shortly,
>talloc_steal is better.
>  
>
Should I be doing something different to keep a valid reference to 
u_handle here?
I can't talloc_steal() it because it's not currently allocated memory 
within a talloc context, and I don't think that
    struct policy_handle *u_handle;
    u_handle =  talloc(tmp_ctx, struct policy_handle);
     ...
    r->out.user_handle = talloc_steal(mem_ctx, u_handle);
is the right approach.

>
>+	computer_dn = talloc_asprintf(ctx,
>+			"%s",r_crack_names.out.ctr.ctr1->array[0].result_name);
>here we should also use talloc_steal(), with the right TALLOC_CTX.
>(I haven't looked close to it to find the right one out.)
>and also we should move that statement after the DsCrackNames call,
>and make shure we didn't dereference a NULL pointer or empty array,
>in case the server sends bad data.
>
>so we need to check if r_crack_names.out.ctr.ctr1 and r_crack_names.out.ctr.ctr1->array[0]
>and r_crack_names.out.ctr.ctr1->array[0].result_name are valid.
>(we may already do this checks, I haven't checked closer)
>
>  
>
r_crack_names.out.ctr.ctr1 gets to ensure it's not NULL.
To ensure that r_crack_names.out.ctr.ctr1->array[0] and 
r_crack_names.out.ctr.ctr1->array[0].result_name are valid, do I just 
check for a NULL reference?


Thanks again!
Brad