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 <vl@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>

diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h
index 6aa57f2e7045a64b3ff575ace5d4cc5de3768eb3..f496c41ccbde6f56116dca0dc17a99e3edc3de17 100644 (file)
--- 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 30d3604bc94c14f56ca189ef48f2b0b6e6f5230c..2a4950b56bfe81cfa054040ff194ff9de91b2362 100644 (file)
--- 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;
        }