[PATCH] Fix terminate connection behavior for asynchronous endpoint with PUSH notification flavors

[Stefan (metze) Metzmacher]

Hi Julien,

> Find below a third version of the patch which now loops over all
> contexts to call the destructor. It is the ubind function which is
> responsible for managing/cleaning up context->conn->pending_call_list.
> 
> I have also moved the code into dcesrv_cleanup_broken_connections()
> because this function is called first whenever there are outstanding
> pending calls.
> 
> Does this approach sounds better/more complete?

Yep, if you know move context_cur->iface = NULL; into the destructor I'm
happy.

Thanks!
metze

> On 31/03/15 19:05, Julien Kerihuel wrote:
>> Metze,
>>
>> Following-up on the chat we had this morning, I have updated the patch
>> accordingly.
>>
>> The new patch explicitly calls the unbind function in
>> dcesrv_terminate_connection() if the flag (renamed to)
>> DCESRV_CALL_STATE_FLAG_PROCESS_PENDING_CALL is set.
>> The unbind function of the interface is responsible for dealing with
>> pending_calls and cleaning-up the list.
>>
>> Does this look more like what you had in mind?
>>
>> Cheers,
>> Julien.
>>
>> On 30/03/15 15:47, Julien Kerihuel wrote:
>>> On 30/03/15 13:22, Stefan (metze) Metzmacher wrote:
>>>> Now, because I call dcesrv_reply() from the callback, it means that I
>>>> have an outstanding pending call as long as I didn't receive a
>>>> notification on this custom p->fd socket from a remote system.
>>>> Sure.
>>>>
>>>> But how is dcerpc_server_asyncemsmdb_unbind() called, with the your changes?
>>>> I don't see it called from dcesrv_terminate_connection().
>>> This is a very good question.
>>>
>>> I had troubles tracking down how the destructor was called due to the
>>> asynchronous mechanism in place and I am afraid I still miss some bits
>>> of it. The patch is relying on these assumptions and observations:
>>>
>>>     1. when the client disconnects and there is no pending call, the
>>> dcerpc server remove dce_conn from broken connections, terminate the
>>> connection using stream_terminate_connection and
>>> dcesrv_connection_context_destructor is called. This destructor is
>>> responsible for calling the unbind function of the interface:
>>>
>>> #0  dcesrv_connection_context_destructor (c=0x7f50ffbcd0e0) at ../source4/rpc_server/dcerpc_server.c:499
>>> #1  0x00007f50f947eb3d in ?? () from /usr/lib/x86_64-linux-gnu/libtalloc.so.2
>>> #2  0x00007f50f947e87b in ?? () from /usr/lib/x86_64-linux-gnu/libtalloc.so.2
>>> #3  0x00007f50f947af33 in _talloc_free () from /usr/lib/x86_64-linux-gnu/libtalloc.so.2
>>> #4  0x00007f50ebedddf6 in dcesrv_read_fragment_done (subreq=0x0) at ../source4/rpc_server/dcerpc_server.c:1563
>>>
>>>
>>>     2. when the client disconnects and there is pending call, the dcerpc
>>> server adds the connection to dce_ctx->broken_connections but
>>> dcesrv_connection_context_destructor is never called.
>>>
>>>>> What is implemented in Samba is correct in all cases with regards to
>>>>> pending calls except this one. In this case, and without this patch,
>>>>> this connection is going to be considered broken 100% of the time.
>>>>> What I want, it for this connection to behave normally upon
>>>>> disconnection from client (close Outlook) and not be considered as a
>>>>> broken connection.
>>>> Why if the client disconnect, the connection is broken, why should we
>>>> pretend
>>>> it's not?
>>> Because if we don't, then we can't execute teardown code upon client
>>> disconnection. If the endpoint is part of a more global asynchronous
>>> infrastructure, we may have to communicate with other services to
>>> invalidate a user session or notify a user has disconnected. That is the
>>> context in which asyncemsmdb endpoint is: upon client disconnection, we
>>> notify a resolution name service and delete from the record the entry
>>> referring to the customer socket associated to the connection.
>>>
>>> Given that I don't have (without the patch) any way to invalidate the
>>> remote data from asyncemsmdb, I end up with deprecated cached session
>>> information. I could invalidate it with the client, but it is less
>>> elegant and really should be a task for the service who registered it in
>>> the first place, hence asyncemsmdb.
>>>
>>> Cheers,
>>> Julien.
>>>
> 

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