[PATCH] Fixes for winbindd

[Stefan (metze) Metzmacher]

Am 31.07.2014 um 10:49 schrieb Stefan (metze) Metzmacher:
> 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.

Also for the argument based functions there's no way to known the
the ref pointers are valid talloc pointers, so the caller
need to make sure to pass a talloc parent for all children,
as mem_ctx to the sync function or to the _recv() function.

I don't see how we can avoid that.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 246 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140731/5e1eb9db/attachment.pgp>