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

[Julien Kerihuel]

Metze,

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?

Cheers,
Julien.

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: 2583 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150406/130e4d68/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/20150406/130e4d68/attachment.pgp>