[PATCH] Fixes for winbindd

[Stefan (metze) Metzmacher]

Am 01.08.2014 um 01:15 schrieb 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?

Yes, please push them.

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/20140801/305eb53b/attachment.pgp>