From: Willy Tarreau Date: Thu, 11 Nov 2010 09:56:04 +0000 (+0100) Subject: [BUG] accept: don't close twice upon error X-Git-Tag: v1.5-dev8~376 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=abe8ea5c1df16e81479644397ed516e391bf630a;p=thirdparty%2Fhaproxy.git [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 cbf6174484..cf6606a075 100644 --- 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 d718695d43..9beb06caed 100644 --- 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 46dfeba9ba..28ef81697d 100644 --- 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) {