[PATCH] Fixes for winbindd

[Andrew Bartlett]

On Thu, 2014-07-31 at 10:49 +0200, Stefan (metze) Metzmacher wrote:
> Am 31.07.2014 um 10:28 schrieb Andrew Bartlett:
> > On Sat, 2014-07-26 at 09:44 +1200, Andrew Bartlett wrote:
> >> 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. 
> > 
> > Can we please push these patches?  I still want metze's view, but I
> > think the patches are as correct as things can be right now, and so
> > should go in while we restructure the pidl generator.
> 
> The patches are fine.
> 
> The argument based wrappers are more complex as the REF_ALLOC behavior
> for the struct based functions is different between, source4/librpc
> and source3/rpc_client/.
> 
> We might simplify this in future, but that requires audits of existing
> callers.

To avoid confusion, is that a formal review?

Thanks,

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140801/0135e45b/attachment.pgp>