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

[Julien Kerihuel]

Patch updated,

Cheers,
Julien.

On 07/04/15 08:39, Stefan (metze) Metzmacher wrote:
> 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.
>>>>

-- 
Julien Kerihuel
j.kerihuel at openchange.org
OpenChange Project Founder

Twitter: http://twitter.com/jkerihuel

GPG Fingerprint: 0B55 783D A781 6329 108A  B609 7EF6 FE11 A35F 1F79

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-DCERPC-flag-to-call-unbind-hooks-without-destroy.patch
Type: text/x-diff
Size: 2764 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150407/0c1cdb84/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150407/0c1cdb84/attachment.pgp>