Hangs in Netatalk which causes it to stop responding to connections
authorRalph Boehme <rb@sernet.de>
Sat, 5 Jul 2014 06:14:41 +0000 (08:14 +0200)
committerRalph Boehme <rb@sernet.de>
Thu, 10 Jul 2014 08:31:09 +0000 (10:31 +0200)
Hangs in Netatalk which causes it to stop responding to
connections. The master afpd process gets stuck in a poll loop, being
repeatedly notified that there are connections on its socket, but
never actually doesn anything with them.

Analysis with gdb revealed that the dat astructure dealing with the
main AFP socket and the IPC client sockets was smashed. This could
happen because the function fdset_add_fd() doesn't do bound checking
itself but relied on other parts of the code that enforce a connection
limit. Unfortunately, for low-level AFP connections that don't result
in a full AFP login these checks come too late resulting in a buffer
overflow.

Add a bound check. While we're at it, rewrite the fdset code to use a
full blown data structure 'struct asev' encapsultating the
implementation details.

Fixes bug #572.

Signed-off-by: Ralph Boehme <rb@sernet.de>
NEWS
etc/afpd/main.c
include/atalk/util.h
libatalk/util/socket.c

diff --git a/NEWS b/NEWS
index 9587f42..9cf18b9 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,8 @@
+Changes in 3.1.4
+================
+* FIX: afpd: Hangs in Netatalk which causes it to stop responding to
+       connections, bug #572.
+
 Changes in 3.1.3
 ================
 * UPD: Spotlight: more SPARQL query optimisations
index 9bea44a..e612a01 100644 (file)
@@ -40,8 +40,7 @@
 #include "afp_zeroconf.h"
 #include "afpstats.h"
 
-#define AFP_LISTENERS 32
-#define FDSET_SAFETY  5
+#define ASEV_THRESHHOLD 10
 
 unsigned char nologin = 0;
 
@@ -49,12 +48,7 @@ static AFPObj obj;
 static server_child_t *server_children;
 static sig_atomic_t reloadconfig = 0;
 static sig_atomic_t gotsigchld = 0;
-
-/* Two pointers to dynamic allocated arrays which store pollfds and associated data */
-static struct pollfd *fdset;
-static struct polldata *polldata;
-static int fdset_size;          /* current allocated size */
-static int fdset_used;          /* number of used elements */
+static struct asev *asev;
 
 static afp_child_t *dsi_start(AFPObj *obj, DSI *dsi, server_child_t *server_children);
 
@@ -67,29 +61,39 @@ static void afp_exit(int ret)
 /* ------------------
    initialize fd set we are waiting for.
 */
-static void fd_set_listening_sockets(const AFPObj *config)
+static bool init_listening_sockets(const AFPObj *config)
 {
     DSI *dsi;
+    int numlisteners;
+
+    for (numlisteners = 0, dsi = config->dsi; dsi; dsi = dsi->next) {
+        numlisteners++;
+    }
+
+    asev = asev_init(config->options.connections + numlisteners + ASEV_THRESHHOLD);
+    if (asev == NULL) {
+        return false;
+    }
 
     for (dsi = config->dsi; dsi; dsi = dsi->next) {
-        fdset_add_fd(config->options.connections + AFP_LISTENERS + FDSET_SAFETY,
-                     &fdset,
-                     &polldata,
-                     &fdset_used,
-                     &fdset_size,
-                     dsi->serversock,
-                     LISTEN_FD,
-                     dsi);
+        if (!(asev_add_fd(asev, dsi->serversock, LISTEN_FD, dsi))) {
+            return false;
+        }
     }
+
+    return true;
 }
  
-static void fd_reset_listening_sockets(const AFPObj *config)
+static bool reset_listening_sockets(const AFPObj *config)
 {
     const DSI *dsi;
 
     for (dsi = config->dsi; dsi; dsi = dsi->next) {
-        fdset_del_fd(&fdset, &polldata, &fdset_used, &fdset_size, dsi->serversock);
+        if (!(asev_del_fd(asev, dsi->serversock))) {
+            return false;
+        }
     }
+    return true;
 }
 
 /* ------------------ */
@@ -133,7 +137,7 @@ static void afp_goaway(int sig)
 static void child_handler(void)
 {
     int fd;
-    int status, i;
+    int status;
     pid_t pid;
   
 #ifndef WAIT_ANY
@@ -157,7 +161,9 @@ static void child_handler(void)
         if (fd == -1) {
             continue;
         }
-        fdset_del_fd(&fdset, &polldata, &fdset_used, &fdset_size, fd);
+        if (!(asev_del_fd(asev, fd))) {
+            LOG(log_error, logtype_afpd, "child[%d]: asev_del_fd: %d", pid, fd);
+        }
     }
 }
 
@@ -328,14 +334,15 @@ int main(int ac, char **av)
     cnid_init();
 
     /* watch atp, dsi sockets and ipc parent/child file descriptor. */
-    fd_set_listening_sockets(&obj);
+    if (!(init_listening_sockets(&obj))) {
+        LOG(log_error, logtype_afpd, "main: couldn't initialize socket handler");
+        afp_exit(EXITERR_CONF);
+    }
 
     /* set limits */
     (void)setlimits();
 
     afp_child_t *child;
-    int recon_ipc_fd;
-    pid_t pid;
     int saveerrno;
 
     /* wait for an appleshare connection. parent remains in the loop
@@ -345,9 +352,8 @@ int main(int ac, char **av)
      * afterwards. establishing timeouts for logins is a possible 
      * solution. */
     while (1) {
-        LOG(log_maxdebug, logtype_afpd, "main: polling %i fds", fdset_used);
         pthread_sigmask(SIG_UNBLOCK, &sigs, NULL);
-        ret = poll(fdset, fdset_used, -1);
+        ret = poll(asev->fdset, asev->used, -1);
         pthread_sigmask(SIG_BLOCK, &sigs, NULL);
         saveerrno = errno;
 
@@ -360,7 +366,10 @@ int main(int ac, char **av)
         if (reloadconfig) {
             nologin++;
 
-            fd_reset_listening_sockets(&obj);
+            if (!(reset_listening_sockets(&obj))) {
+                LOG(log_error, logtype_afpd, "main: reset socket handlers");
+                afp_exit(EXITERR_CONF);
+            }
 
             LOG(log_info, logtype_afpd, "re-reading configuration file");
 
@@ -375,7 +384,10 @@ int main(int ac, char **av)
                 afp_exit(EXITERR_CONF);
             }
 
-            fd_set_listening_sockets(&obj);
+            if (!(init_listening_sockets(&obj))) {
+                LOG(log_error, logtype_afpd, "main: couldn't initialize socket handler");
+                afp_exit(EXITERR_CONF);
+            }
 
             nologin = 0;
             reloadconfig = 0;
@@ -393,30 +405,40 @@ int main(int ac, char **av)
             break;
         }
 
-        for (int i = 0; i < fdset_used; i++) {
-            if (fdset[i].revents & (POLLIN | POLLERR | POLLHUP | POLLNVAL)) {
-                switch (polldata[i].fdtype) {
+        for (int i = 0; i < asev->used; i++) {
+            if (asev->fdset[i].revents & (POLLIN | POLLERR | POLLHUP | POLLNVAL)) {
+                switch (asev->data[i].fdtype) {
 
                 case LISTEN_FD:
-                    if ((child = dsi_start(&obj, (DSI *)polldata[i].data, server_children))) {
-                        /* Add IPC fd to select fd set */
-                        fdset_add_fd(obj.options.connections + AFP_LISTENERS + FDSET_SAFETY,
-                                     &fdset,
-                                     &polldata,
-                                     &fdset_used,
-                                     &fdset_size,
-                                     child->afpch_ipc_fd,
-                                     IPC_FD,
-                                     child);
+                    if ((child = dsi_start(&obj, (DSI *)(asev->data[i].private), server_children))) {
+                        if (!(asev_add_fd(asev, child->afpch_ipc_fd, IPC_FD, child))) {
+                            LOG(log_error, logtype_afpd, "out of asev slots");
+
+                            /*
+                             * Close IPC fd here and mark it as unused
+                             */
+                            close(child->afpch_ipc_fd);
+                            child->afpch_ipc_fd = -1;
+
+                            /*
+                             * Being unfriendly here, but we really
+                             * want to get rid of it. The 'child'
+                             * handle gets cleaned up in the SIGCLD
+                             * handler.
+                             */
+                            kill(child->afpch_pid, SIGKILL);
+                        }
                     }
                     break;
 
                 case IPC_FD:
-                    child = (afp_child_t *)polldata[i].data;
+                    child = (afp_child_t *)(asev->data[i].private);
                     LOG(log_debug, logtype_afpd, "main: IPC request from child[%u]", child->afpch_pid);
 
                     if (ipc_server_read(server_children, child->afpch_ipc_fd) != 0) {
-                        fdset_del_fd(&fdset, &polldata, &fdset_used, &fdset_size, child->afpch_ipc_fd);
+                        if (!(asev_del_fd(asev, child->afpch_ipc_fd))) {
+                            LOG(log_error, logtype_afpd, "child[%u]: no IPC fd");
+                        }
                         close(child->afpch_ipc_fd);
                         child->afpch_ipc_fd = -1;
                     }
index 312160b..6d92e15 100644 (file)
@@ -16,6 +16,7 @@
 #include <unistd.h>
 #include <poll.h>
 #include <sys/stat.h>
+#include <stdbool.h>
 
 #include <atalk/unicode.h>
 #include <atalk/bstrlib.h>
@@ -156,26 +157,32 @@ extern int compare_ip(const struct sockaddr *sa1, const struct sockaddr *sa2);
 extern int tokenize_ip_port(const char *ipurl, char **address, char **port);
 
 /* Structures and functions dealing with dynamic pollfd arrays */
-enum fdtype {IPC_FD, LISTEN_FD};
-struct polldata {
-    enum fdtype fdtype; /* IPC fd or listening socket fd                 */
-    void *data;         /* pointer to AFPconfig for listening socket and *
-                         * pointer to afp_child_t for IPC fd             */
+
+enum asev_fdtype {IPC_FD, LISTEN_FD};
+
+/**
+ * atalk socket event data
+ **/
+struct asev_data {
+    enum asev_fdtype fdtype;  /* IPC fd or listening socket fd                 */
+    void            *private; /* pointer to AFPconfig for listening socket and *
+                               * pointer to afp_child_t for IPC fd             */
 };
 
-extern void fdset_add_fd(int maxconns,
-                         struct pollfd **fdsetp,
-                         struct polldata **polldatap,
-                         int *fdset_usedp,
-                         int *fdset_sizep,
-                         int fd,
-                         enum fdtype fdtype,
-                         void *data);
-extern void fdset_del_fd(struct pollfd **fdsetp,
-                         struct polldata **polldatap,
-                         int *fdset_usedp,
-                         int *fdset_sizep,
-                         int fd);
+/**
+ * atalk socket event
+ **/
+struct asev {
+    struct pollfd         *fdset; /* struct pollfd array for poll() */
+    struct asev_data      *data;  /* associated array of data       */
+    int                    max;
+    int                    used;
+};
+
+extern struct asev *asev_init(int max);
+extern bool asev_add_fd(struct asev *sev, int fd, enum asev_fdtype fdtype, void *private);
+extern bool asev_del_fd(struct asev *sev, int fd);
+
 extern int send_fd(int socket, int fd);
 extern int recv_fd(int fd, int nonblocking);
 
index c7ef534..60768a6 100644 (file)
@@ -503,113 +503,112 @@ EC_CLEANUP:
     EC_EXIT;
 }
 
-/*!
- * Add a fd to a dynamic pollfd array that is allocated and grown as needed
- *
- * This uses an additional array of struct polldata which stores type information
- * (enum fdtype) and a pointer to anciliary user data.
- *
- * 1. Allocate the arrays with the size of "maxconns" if *fdsetp is NULL.
- * 2. Fill in both array elements and increase count of used elements
- * 
- * @param maxconns    (r)  maximum number of connections, determines array size
- * @param fdsetp      (rw) pointer to callers pointer to the pollfd array
- * @param polldatap   (rw) pointer to callers pointer to the polldata array
- * @param fdset_usedp (rw) pointer to an int with the number of used elements
- * @param fdset_sizep (rw) pointer to an int which stores the array sizes
- * @param fd          (r)  file descriptor to add to the arrays
- * @param fdtype      (r)  type of fd, currently IPC_FD or LISTEN_FD
- * @param data        (rw) pointer to data the caller want to associate with an fd
- */
-void fdset_add_fd(int maxconns,
-                  struct pollfd **fdsetp,
-                  struct polldata **polldatap,
-                  int *fdset_usedp,
-                  int *fdset_sizep,
-                  int fd,
-                  enum fdtype fdtype,
-                  void *data)
+/**
+ * Allocate and initialize atalk socket event struct
+ **/
+struct asev *asev_init(int max)
 {
-    struct pollfd *fdset = *fdsetp;
-    struct polldata *polldata = *polldatap;
-    int fdset_size = *fdset_sizep;
+    struct asev *asev = calloc(1, sizeof(struct asev));
+
+    if (asev == NULL) {
+        return NULL;
+    }
 
-    LOG(log_debug, logtype_default, "fdset_add_fd: adding fd %i in slot %i", fd, *fdset_usedp);
+    /* Initialize with space for all possibly active fds */
+    asev->fdset = calloc(max, sizeof(struct pollfd));
+    asev->data = calloc(max, sizeof(struct asev_data));
 
-    if (fdset == NULL) { /* 1 */
-        /* Initialize with space for all possibly active fds */
-        fdset = calloc(maxconns, sizeof(struct pollfd));
-        if (! fdset)
-            exit(EXITERR_SYS);
+    if (asev->fdset == NULL || asev->data == NULL) {
+        free(asev->fdset);
+        free(asev->data);
+        free(asev);
+        return NULL;
+    }
 
-        polldata = calloc(maxconns, sizeof(struct polldata));
-        if (! polldata)
-            exit(EXITERR_SYS);
+    asev->max = max;
+    asev->used = 0;
 
-        fdset_size = maxconns;
+    return asev;
+}
 
-        *fdset_sizep = fdset_size;
-        *fdsetp = fdset;
-        *polldatap = polldata;
+/**
+ * Add a fd to a dynamic pollfd array and associated data array
+ *
+ * This uses an additional array of struct polldata which stores type
+ * information (enum fdtype) and a pointer to anciliary user data.
+ **/
+bool asev_add_fd(struct asev *asev,
+                 int fd,
+                 enum asev_fdtype fdtype,
+                 void *private)
+{
+    if (asev == NULL) {
+        return false;
+    }
 
-        LOG(log_debug, logtype_default, "fdset_add_fd: initialized with space for %i conncections", 
-            maxconns);
+    if (!(asev->used < asev->max)) {
+        return false;
     }
 
-    /* 2 */
-    fdset[*fdset_usedp].fd = fd;
-    fdset[*fdset_usedp].events = POLLIN;
-    polldata[*fdset_usedp].fdtype = fdtype;
-    polldata[*fdset_usedp].data = data;
-    (*fdset_usedp)++;
+    asev->fdset[asev->used].fd = fd;
+    asev->fdset[asev->used].events = POLLIN;
+    asev->data[asev->used].fdtype = fdtype;
+    asev->data[asev->used].private = private;
+    asev->used++;
+
+    return true;
 }
 
-/*!
- * Remove a fd from our pollfd array
- *
- * 1. Search fd
- * 2a Matched last (or only) in the set ? null it and return
- * 2b If we remove the last array elemnt, just decrease count
- * 3. If found move all following elements down by one
- * 4. Decrease count of used elements in array
- *
- * This currently doesn't shrink the allocated storage of the array.
+/**
+ * Remove fd from asev
  *
- * @param fdsetp      (rw) pointer to callers pointer to the pollfd array
- * @param polldatap   (rw) pointer to callers pointer to the polldata array
- * @param fdset_usedp (rw) pointer to an int with the number of used elements
- * @param fdset_sizep (rw) pointer to an int which stores the array sizes
- * @param fd          (r)  file descriptor to remove from the arrays
- */
-void fdset_del_fd(struct pollfd **fdsetp,
-                  struct polldata **polldatap,
-                  int *fdset_usedp,
-                  int *fdset_sizep _U_,
-                  int fd)
+ * @returns true if the fd was deleted, otherwise false
+ **/
+bool asev_del_fd(struct asev *asev, int fd)
 {
-    struct pollfd *fdset = *fdsetp;
-    struct polldata *polldata = *polldatap;
-
-    if (*fdset_usedp < 1)
-        return;
-
-    for (int i = 0; i < *fdset_usedp; i++) {
-        if (fdset[i].fd == fd) { /* 1 */
-            if ((i + 1) == *fdset_usedp) { /* 2a */
-                fdset[i].fd = -1;
-                memset(&polldata[i], 0, sizeof(struct polldata));
-            } else if (i < (*fdset_usedp - 1)) { /* 2b */
-                memmove(&fdset[i],
-                        &fdset[i+1],
-                        (*fdset_usedp - i - 1) * sizeof(struct pollfd)); /* 3 */
-                memmove(&polldata[i],
-                        &polldata[i+1],
-                        (*fdset_usedp - i - 1) * sizeof(struct polldata)); /* 3 */
+    int i;
+    int numafter;
+
+    if (asev == NULL) {
+        return false;
+    }
+
+    if (asev->used == 0) {
+        LOG(log_error, logtype_cnid, "asev_del_fd: empty");
+        return false;
+    }
+
+    for (i = 0; i < asev->used; i++) {
+        /*
+         * Scan the array for a matching fd
+         */
+        if (asev->fdset[i].fd == fd) {
+            /*
+             * found fd
+             */
+            if ((i + 1) == asev->used) {
+                /*
+                 * it's the last (or only) array element, simply null it
+                 */
+                asev->fdset[i].fd = -1;
+                asev->data[i].fdtype = 0;
+                asev->data[i].private = NULL;
+            } else {
+                /*
+                 * Move down by one all subsequent elements
+                 */
+                numafter = asev->used - (i + 1);
+                memmove(&asev->fdset[i], &asev->fdset[i+1],
+                        numafter * sizeof(struct pollfd));
+                memmove(&asev->data[i], &asev->data[i+1],
+                        numafter * sizeof(struct asev_data));
             }
-            (*fdset_usedp)--;
-            break;
+            asev->used--;
+            return true;
         }
     }
+
+    return false;
 }
 
 /* Length of the space taken up by a padded control message of length len */