[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