From 49adb5ac8fed5ec972bf3907169d2efd378970ca Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Mon, 26 Feb 2018 15:12:14 +0100 Subject: [PATCH] winbind: Fix a race between the sigchld and 0-sized socket read Fix a bug when a child dies when a request is pending in the child. If the signal handler fires before epoll finds out the other end of the parent-child socket is closed, we close the socket on our side without taking care of the pending request. This causes two problems: First, that one pending request never is replied to properly, and secondly, we might end up with EPOLL_DEL on a wrong file descriptor. This causes all sorts of trouble if we hit an active one. The fix for this problem is not to close the socket in winbind_child_died(). This however stops an idle child that dies hard from being properly cleaned up. The fix for that is to add the child->monitor_fde that is set pending only when no child request is active. This way we can remove the close(sock) in the signal handler. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13309 Signed-off-by: Volker Lendecke Reviewed-by: Andreas Schneider --- source3/winbindd/winbindd.h | 1 + source3/winbindd/winbindd_dual.c | 47 +++++++++++++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h index 6aa57f2e704..f496c41ccbd 100644 --- a/source3/winbindd/winbindd.h +++ b/source3/winbindd/winbindd.h @@ -120,6 +120,7 @@ struct winbindd_child { char *logfilename; int sock; + struct tevent_fd *monitor_fde; /* Watch for dead children/sockets */ struct tevent_queue *queue; struct dcerpc_binding_handle *binding_handle; diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c index 30d3604bc94..2a4950b56bf 100644 --- a/source3/winbindd/winbindd_dual.c +++ b/source3/winbindd/winbindd_dual.c @@ -239,6 +239,8 @@ static void wb_child_request_waited(struct tevent_req *subreq) return; } + tevent_fd_set_flags(state->child->monitor_fde, 0); + subreq = wb_simple_trans_send(state, server_event_context(), NULL, state->child->sock, state->request); if (tevent_req_nomem(subreq, req)) { @@ -338,6 +340,8 @@ static void wb_child_request_cleanup(struct tevent_req *req, TALLOC_FREE(state->subreq); TALLOC_FREE(state->queue_subreq); + tevent_fd_set_flags(state->child->monitor_fde, TEVENT_FD_READ); + if (state->child->domain != NULL) { /* * If the child is attached to a domain, @@ -359,10 +363,36 @@ static void wb_child_request_cleanup(struct tevent_req *req, * The basic parent/child communication broke, close * our socket */ + TALLOC_FREE(state->child->monitor_fde); close(state->child->sock); state->child->sock = -1; } +static void child_socket_readable(struct tevent_context *ev, + struct tevent_fd *fde, + uint16_t flags, + void *private_data) +{ + struct winbindd_child *child = private_data; + + if ((flags & TEVENT_FD_READ) == 0) { + return; + } + + TALLOC_FREE(child->monitor_fde); + + /* + * We're only active when there is no outstanding child + * request. Arriving here means the child closed its socket, + * it died. Do the same here. + */ + + SMB_ASSERT(child->sock != -1); + + close(child->sock); + child->sock = -1; +} + static struct winbindd_child *choose_domain_child(struct winbindd_domain *domain) { struct winbindd_child *shortest = &domain->children[0]; @@ -798,11 +828,6 @@ void winbind_child_died(pid_t pid) } state.child->pid = 0; - - if (state.child->sock != -1) { - close(state.child->sock); - state.child->sock = -1; - } } /* Ensure any negative cache entries with the netbios or realm names are removed. */ @@ -1630,6 +1655,18 @@ static bool fork_domain_child(struct winbindd_child *child) return false; } + child->monitor_fde = tevent_add_fd(server_event_context(), + server_event_context(), + fdpair[1], + TEVENT_FD_READ, + child_socket_readable, + child); + if (child->monitor_fde == NULL) { + DBG_WARNING("tevent_add_fd failed\n"); + close(fdpair[1]); + return false; + } + child->sock = fdpair[1]; return True; } -- 2.11.4.GIT