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 556be2e..8bf5750 100644
--- a/include/types/listener.h
+++ b/include/types/listener.h
@@ -198,7 +198,7 @@
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 3656c82..8b167bd 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -151,12 +151,16 @@
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 @@
}
/* 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 @@
{
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 @@
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 @@
* 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;