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

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


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.

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


More information about the samba-technical mailing list