[PATCH] Fixes for winbindd

[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.

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/91569038/attachment.pgp>