BUG/MEDIUM: listener: Fix how unlimited number of consecutive accepts is handled

There is a bug when global.tune.maxaccept is set to -1 (no limit). It is pretty
visible with one process (nbproc sets to 1). The functions listener_accept() and
accept_queue_process() don't expect to handle negative maxaccept values. So
instead of accepting incoming connections without any limit, none are never
accepted and HAProxy loop infinitly in the scheduler.

When there are 2 or more processes, the bug is a bit more subtile. The limit for
a listener is set to 1. So only one connection is accepted at a time by a given
listener. This happens because the listener's maxaccept value is an unsigned
integer. In check_config_validity(), it is first set to UINT_MAX (-1 casted in
an unsigned integer), and then some calculations on it leads to an integer
overflow.

To fix the bug, the listener's maxaccept value is now a signed integer. So, if a
negative value is set for global.tune.maxaccept, we keep it untouched for the
listener and no calculation is made on it. Then, in the listener code, this
signed value is casted to a unsigned one. It simplifies all tests instead of
dealing with negative values. So, it limits the number of connections accepted
at a time to UINT_MAX at most. But, honestly, it not an issue.

This patch must be backported to 1.9 and 1.8.

diff --git a/include/types/listener.h b/include/types/listener.h
index 556be2e659272e32cc11a2569cbeb9b9513009cd..8bf57500c310497488d30e78a10974ef37ff96cf 100644 (file)
--- a/include/types/listener.h
+++ b/include/types/listener.h
@@ -198,7 +198,7 @@ struct listener {
        int nbconn;                     /* current number of connections on this listener */
        int maxconn;                    /* maximum connections allowed on this listener */
        unsigned int backlog;           /* if set, listen backlog */
-       unsigned int maxaccept;         /* if set, max number of connections accepted at once */
+       int maxaccept;         /* if set, max number of connections accepted at once (-1 when disabled) */
        int (*accept)(struct listener *l, int fd, struct sockaddr_storage *addr); /* upper layer's accept() */
        enum obj_type *default_target;  /* default target to use for accepted sessions or NULL */
        /* cache line boundary */

diff --git a/src/listener.c b/src/listener.c
index 3656c821b4b7d729daf38c11bfb05ee3ca2e50e5..8b167bd7215d8797ce16a88e429929cc6919bd71 100644 (file)
--- a/src/listener.c
+++ b/src/listener.c
@@ -151,12 +151,16 @@ static struct task *accept_queue_process(struct task *t, void *context, unsigned
        struct accept_queue_ring *ring = context;
        struct listener *li;
        struct sockaddr_storage addr;
-       int max_accept = global.tune.maxaccept ? global.tune.maxaccept : 64;
+       unsigned int max_accept;
        int addr_len;
        int ret;
        int fd;
 
-       while (max_accept--) {
+       /* if global.tune.maxaccept is -1, then max_accept is UINT_MAX. It
+        * is not really illimited, but it is probably enough.
+        */
+       max_accept = global.tune.maxaccept ? global.tune.maxaccept : 64;
+       for (; max_accept; max_accept--) {
                addr_len = sizeof(addr);
                fd = accept_queue_pop_sc(ring, &li, &addr, &addr_len);
                if (fd < 0)
@@ -183,7 +187,7 @@ static struct task *accept_queue_process(struct task *t, void *context, unsigned
        }
 
        /* ran out of budget ? Let's come here ASAP */
-       if (max_accept < 0)
+       if (!max_accept)
                task_wakeup(t, TASK_WOKEN_IO);
 
        return t;
@@ -598,7 +602,7 @@ void listener_accept(int fd)
 {
        struct listener *l = fdtab[fd].owner;
        struct proxy *p;
-       int max_accept;
+       unsigned int max_accept;
        int next_conn = 0;
        int next_feconn = 0;
        int next_actconn = 0;
@@ -612,6 +616,10 @@ void listener_accept(int fd)
        if (!l)
                return;
        p = l->bind_conf->frontend;
+
+       /* if l->maxaccept is -1, then max_accept is UINT_MAX. It is not really
+        * illimited, but it is probably enough.
+        */
        max_accept = l->maxaccept ? l->maxaccept : 1;
 
        if (!(l->options & LI_O_UNLIMITED) && global.sps_lim) {
@@ -672,7 +680,7 @@ void listener_accept(int fd)
         * worst case. If we fail due to system limits or temporary resource
         * shortage, we try again 100ms later in the worst case.
         */
-       for (; max_accept-- > 0; next_conn = next_feconn = next_actconn = 0) {
+       for (; max_accept; next_conn = next_feconn = next_actconn = 0, max_accept--) {
                struct sockaddr_storage addr;
                socklen_t laddr = sizeof(addr);
                unsigned int count;