[PATCH] Fixes for winbindd

[Andrew Bartlett]

On Fri, 2014-07-25 at 22:59 +0200, Volker Lendecke wrote:
> On Sat, Jul 26, 2014 at 08:26:18AM +1200, Andrew Bartlett wrote:
> > netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_send() creates
> > state on mem_ctx, which is frame in
> > netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords()
> > 
> > 	struct netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_state *state;
> > 	struct tevent_req *subreq;
> > 
> > 	req = tevent_req_create(mem_ctx, &state,
> > 				struct netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords_state);
> > 	if (req == NULL) {
> > 		return NULL;
> > 	}
> > 
> > The state->dns_names is an in/out parameter as dns_names to
> > netlogon_creds_cli_DsrUpdateReadOnlyServerDnsRecords() and has a longer
> > life time. 
> > 
> > The issue isn't the async, it is that the memory needs to be around
> > longer than the state variable.
> > 
> > The typical approach of a talloc_steal won't work, due to the way the
> > ref variable is overwritten.  The struct NL_DNS_NAME_INFO_ARRAY
> > dns_names is the original input array, but the elements are overwritten
> > in dcerpc_netr_DsrUpdateReadOnlyServerDnsRecords_done()
> 
> Ok, understood. To be honest, I'd call this a bug in the way
> pidl generates code. I'm not sure there's a better way to do
> it, but this is really, really obscure. I'd like to hear
> what metze has to say about this before it goes in.
> 
> The pure fact that this crash initially went in shows that
> something is wrong.

I do feel the same.  It seems to me that in/out variables used this way
should be the talloc children of what we pass in, and/or that the
structure (rather than argument based) calling convention could have
made this clearer. 

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba