[PATCH] winbindd: disconnect child process if request is cancelled at main process

Stefan (metze) Metzmacher metze at samba.org
Tue Jun 23 16:09:38 MDT 2015


Am 24.06.2015 um 00:05 schrieb Stefan (metze) Metzmacher:
> Hi Uri,
> 
>> This patch fixes what seems to be breakage of line protocol between
>> the main winbindd process and one of its child processes, if a request
>> that is right now being served at the child process, is cancelled
>> (deleted) at the main process. The current code seems to nicely clean
>> all related objects and leave nothing dangling behind, except there's
>> an unanswered request in the pipe between the parent and the child,
>> and so the next request will read something that's not the response to
>> its request.
>>
>> The fix is to close the connection and fork a new child. A different
>> possible fix would be to detach the request and let it finish, without
>> processing the result. The advantage of opening a new connection is
>> "getting back to business" instead of continue waiting. The
>> disadvantage is possibly creating more and more winbindd child
>> processes. Perhaps this should be accompanied by actively killing the
>> child process (at least sending SIGTERM once) and not letting it exit
>> when it fails to write its response.
>>
>> The patch is related to the rework I'm doing to the patch that
>> controls number of fds, although technically it is not part of this
>> rework as it is a bug fix and needed in both the old patch of
>> stopping/resuming accept and the new patch I'm working on.
> 
> Some generic comments:
> - Please don't use tevent_req_set_cleanup_fn(req, NULL); The
>   cleanup function need some logic to turn into a no-op.
>   E.g. using something like: if (state->subreq == NULL) { return; }
> 
> - tevent_req_error(subreq, ECANCELED); is wrong a caller
>   need to use tevent_req_cancel(subreq). Only the implementation
>   is allowed to call tevent_req_{done,error}(req) (or wrappers),
>   but not the caller.
> 
> Regarding the specific patch:
> 
> I think the cleanup function should call:
> 
> 
> TALLOC_FREE(state->subreq);
> 
> if (state->child == NULL) {
>     /* nothing to cleanup */
>     return;
> }
> 
> if (req_state == TEVENT_REQ_DONE) {
>     /* we're done and don't need a reference to the child anymore */
>     state->child = NULL;
>     return;
> }
> 
> close(state->child->sock);
> state->child->sock = -1;
> DLIST_REMOVE(winbindd_children, state->child);
> state->child = NULL;
> return;
> 
> That way we just need to remember the subreq and set the timeout on
> the subreq.

Actually the timeout can also be set on req instead of subreq,
similar to smbcli_transport_connect_send().

metze

-------------- 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/20150624/911cd6a3/attachment.pgp>


More information about the samba-technical mailing list