[BUG] accept: don't close twice upon error

The stream_sock's accept() used to close the FD upon error, but this
was also sometimes performed by the frontend's accept() called via the
session's accept(). Those interlaced calls were also responsible for the
spaghetti-looking error unrolling code in session.c and stream_sock.c.

Now the frontend must not close the FD anymore, the session is responsible
for that. It also takes care of just closing the FD or also removing from
the FD lists, depending on its state. The socket-level accept() does not
have to care about that anymore.

diff --git a/src/frontend.c b/src/frontend.c
index cbf617448467c310c03a904dadd8db30db01a9ef..cf6606a075f379860347477afcf2b10f2d96ebfb 100644 (file)
--- a/src/frontend.c
+++ b/src/frontend.c
@@ -56,8 +56,9 @@ void get_frt_addr(struct session *s)
 }
 
 /* Finish a session accept() for a proxy (TCP or HTTP). It returns a negative
- * value in case of failure, a positive value in case of success, or zero if
- * it is a success but the session must be closed ASAP.
+ * value in case of a critical failure which must cause the listener to be
+ * disabled, a positive value in case of success, or zero if it is a success
+ * but the session must be closed ASAP (eg: monitoring).
  */
 int frontend_accept(struct session *s)
 {
@@ -89,7 +90,7 @@ int frontend_accept(struct session *s)
        /* Adjust some socket options */
        if ((s->listener->addr.ss_family != AF_UNIX) &&
            setsockopt(cfd, IPPROTO_TCP, TCP_NODELAY, (char *) &one, sizeof(one)) == -1)
-               goto out_delete_cfd;
+               goto out_return;
 
        if (s->fe->options & PR_O_TCP_CLI_KA)
                setsockopt(cfd, SOL_SOCKET, SO_KEEPALIVE, (char *) &one, sizeof(one));
@@ -107,7 +108,7 @@ int frontend_accept(struct session *s)
                /* the captures are only used in HTTP frontends */
                if (unlikely(s->fe->nb_req_cap > 0 &&
                             (s->txn.req.cap = pool_alloc2(s->fe->req_cap_pool)) == NULL))
-                       goto out_delete_cfd;    /* no memory */
+                       goto out_return;        /* no memory */
 
                if (unlikely(s->fe->nb_rsp_cap > 0 &&
                             (s->txn.rsp.cap = pool_alloc2(s->fe->rsp_cap_pool)) == NULL))
@@ -256,8 +257,7 @@ int frontend_accept(struct session *s)
        pool_free2(s->fe->rsp_cap_pool, s->txn.rsp.cap);
  out_free_reqcap:
        pool_free2(s->fe->req_cap_pool, s->txn.req.cap);
- out_delete_cfd:
-       fd_delete(cfd);
+ out_return:
        return -1;
 }
 

diff --git a/src/session.c b/src/session.c
index d718695d43f3af516fe2975252af4f4472103042..9beb06caedb9e130cf73989bedbe0842fc2b50b4 100644 (file)
--- a/src/session.c
+++ b/src/session.c
@@ -47,8 +47,8 @@ struct list sessions;
 
 /* This function is called from the protocol layer accept() in order to instanciate
  * a new session on behalf of a given listener and frontend. It returns a positive
- * value upon success, 0 if the connection needs to be closed and ignored, or a
- * negative value upon critical failure.
+ * value upon success, 0 if the connection can be ignored, or a negative value upon
+ * critical failure. The accepted file descriptor is closed if we return <= 0.
  */
 int session_accept(struct listener *l, int cfd, struct sockaddr_storage *addr)
 {
@@ -56,6 +56,10 @@ int session_accept(struct listener *l, int cfd, struct sockaddr_storage *addr)
        struct session *s;
        struct http_txn *txn;
        struct task *t;
+       int ret;
+
+
+       ret = -1; /* assume unrecoverable error by default */
 
        if (unlikely((s = pool_alloc2(pool2_session)) == NULL))
                goto out_close;
@@ -76,20 +80,20 @@ int session_accept(struct listener *l, int cfd, struct sockaddr_storage *addr)
                     addr->ss_family == AF_INET &&
                     (((struct sockaddr_in *)addr)->sin_addr.s_addr & p->mon_mask.s_addr) == p->mon_net.s_addr)) {
                if (p->mode == PR_MODE_TCP) {
-                       pool_free2(pool2_session, s);
-                       return 0;
+                       ret = 0; /* successful termination */
+                       goto out_free_session;
                }
                s->flags |= SN_MONITOR;
                s->logs.logwait = 0;
        }
 
+       if (unlikely((t = task_new()) == NULL))
+               goto out_free_session;
+
        /* OK, we're keeping the session, so let's properly initialize the session */
        LIST_ADDQ(&sessions, &s->list);
        LIST_INIT(&s->back_refs);
 
-       if (unlikely((t = task_new()) == NULL))
-               goto out_free_session;
-
        s->term_trace = 0;
        s->cli_addr = *addr;
        s->logs.accept_date = date; /* user-visible date for logging */
@@ -119,20 +123,16 @@ int session_accept(struct listener *l, int cfd, struct sockaddr_storage *addr)
         * even initializing the stream interfaces.
         */
        if ((l->options & LI_O_TCP_RULES) && !tcp_exec_req_rules(s)) {
-               if (s->stkctr1_entry || s->stkctr2_entry)
-                       session_store_counters(s);
-               task_free(t);
-               LIST_DEL(&s->list);
-               pool_free2(pool2_session, s);
                /* let's do a no-linger now to close with a single RST. */
                setsockopt(cfd, SOL_SOCKET, SO_LINGER, (struct linger *) &nolinger, sizeof(struct linger));
-               p->feconn--;
-               return 0;
+               ret = 0; /* successful termination */
+               goto out_free_task;
        }
 
        /* This session was accepted, count it now */
        if (p->feconn > p->counters.feconn_max)
                p->counters.feconn_max = p->feconn;
+
        proxy_inc_fe_sess_ctr(l, p);
        if (s->stkctr1_entry) {
                void *ptr;
@@ -274,23 +274,12 @@ int session_accept(struct listener *l, int cfd, struct sockaddr_storage *addr)
        fdinfo[cfd].peerlen  = sizeof(s->cli_addr);
        EV_FD_SET(cfd, DIR_RD);
 
-       if (p->accept) {
-               int ret = p->accept(s);
-               if (unlikely(ret < 0))
-                       goto out_free_rep;
-
-               if (unlikely(ret == 0)) {
-                       /* work is finished, we can release everything (eg: monitoring) */
-                       pool_free2(pool2_buffer, s->rep);
-                       pool_free2(pool2_buffer, s->req);
-                       if (s->stkctr1_entry || s->stkctr2_entry)
-                               session_store_counters(s);
-                       task_free(t);
-                       LIST_DEL(&s->list);
-                       pool_free2(pool2_session, s);
-                       p->feconn--;
-                       return 0;
-               }
+       if (p->accept && (ret = p->accept(s)) <= 0) {
+               /* Either we had an unrecoverable error (<0) or work is
+                * finished (=0, eg: monitoring), in both situations,
+                * we can release everything and close.
+                */
+               goto out_free_rep;
        }
 
        /* it is important not to call the wakeup function directly but to
@@ -310,11 +299,15 @@ int session_accept(struct listener *l, int cfd, struct sockaddr_storage *addr)
        if (s->stkctr1_entry || s->stkctr2_entry)
                session_store_counters(s);
        task_free(t);
- out_free_session:
        LIST_DEL(&s->list);
+ out_free_session:
        pool_free2(pool2_session, s);
  out_close:
-       return -1;
+       if (fdtab[cfd].state != FD_STCLOSE)
+               fd_delete(cfd);
+       else
+               close(cfd);
+       return ret;
 }
 
 /*

diff --git a/src/stream_sock.c b/src/stream_sock.c
index 46dfeba9ba0ae607bb8042938d25100fe8182a6c..28ef81697daf88e3cab7c23dafec9d2e7825e7c1 100644 (file)
--- a/src/stream_sock.c
+++ b/src/stream_sock.c
@@ -1191,7 +1191,8 @@ int stream_sock_accept(int fd)
                        send_log(p, LOG_EMERG,
                                 "Proxy %s reached the configured maximum connection limit. Please check the global 'maxconn' value.\n",
                                 p->id);
-                       goto out_close;
+                       close(cfd);
+                       return 0;
                }
 
                jobs++;
@@ -1205,24 +1206,23 @@ int stream_sock_accept(int fd)
                }
 
                ret = l->accept(l, cfd, &addr);
-               if (unlikely(ret < 0)) {
-                       /* critical error encountered, generally a resource shortage */
+               if (unlikely(ret <= 0)) {
+                       /* The connection was closed by session_accept(). Either
+                        * we just have to ignore it (ret == 0) or it's a critical
+                        * error due to a resource shortage, and we must stop the
+                        * listener (ret < 0).
+                        */
+                       jobs--;
+                       actconn--;
+                       l->nbconn--;
+                       if (ret == 0) /* successful termination */
+                               continue;
+
                        if (p) {
                                disable_listener(l);
                                p->state = PR_STIDLE;
                        }
-                       jobs--;
-                       actconn--;
-                       l->nbconn--;
-                       goto out_close;
-               }
-               else if (unlikely(ret == 0)) {
-                       /* ignore this connection */
-                       jobs--;
-                       actconn--;
-                       l->nbconn--;
-                       close(cfd);
-                       continue;
+                       return 0;
                }
 
                if (l->nbconn >= l->maxconn) {
@@ -1231,13 +1231,9 @@ int stream_sock_accept(int fd)
                }
        } /* end of while (p->feconn < p->maxconn) */
        return 0;
-
-       /* Error unrolling */
- out_close:
-       close(cfd);
-       return 0;
 }
 
+
 /* Prepare a stream interface to be used in socket mode. */
 void stream_sock_prepare_interface(struct stream_interface *si)
 {