From dd80759bb99c9f748b565488dbb4524ba0de3ffd Mon Sep 17 00:00:00 2001 From: sg Date: Tue, 22 Mar 2016 07:30:44 +0100 Subject: [PATCH] tcp: changed accept handling to be done internally: the application does not have to call tcp_accepted() any more. Instead, when delaying accept (e.g. sockets do), call tcp_backlog_delayed()/tcp_backlog_accepted() (fixes bug #46696) --- CHANGELOG | 5 ++ src/api/api_lib.c | 4 +- src/api/api_msg.c | 49 ++++++++++----- src/apps/httpd/httpd.c | 7 +-- src/core/tcp.c | 100 +++++++++++++++++++++++++------ src/core/tcp_in.c | 20 +++++-- src/include/lwip/priv/api_msg.h | 3 + src/include/lwip/priv/tcp_priv.h | 10 ++-- src/include/lwip/tcp.h | 64 ++++++++++---------- 9 files changed, 176 insertions(+), 86 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 9a460c42..ca9a7d2c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -302,6 +302,11 @@ HISTORY ++ Bugfixes: + 2016-03-22: Simon Goldschmidt + * tcp: changed accept handling to be done internally: the application does not + have to call tcp_accepted() any more. Instead, when delaying accept (e.g. sockets + do), call tcp_backlog_delayed()/tcp_backlog_accepted() (fixes bug #46696) + 2016-03-22: Simon Goldschmidt * dns.c: ignore dns response parsing errors, only abort resolving for correct responses or error responses from correct server (bug #47459) diff --git a/src/api/api_lib.c b/src/api/api_lib.c index bb5eb079..c8a0b736 100644 --- a/src/api/api_lib.c +++ b/src/api/api_lib.c @@ -386,9 +386,9 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn) #if TCP_LISTEN_BACKLOG /* Let the stack know that we have accepted the connection. */ API_MSG_VAR_ALLOC_DONTFAIL(msg); - API_MSG_VAR_REF(msg).conn = conn; + API_MSG_VAR_REF(msg).conn = newconn; /* don't care for the return value of lwip_netconn_do_recv */ - netconn_apimsg(lwip_netconn_do_recv, &API_MSG_VAR_REF(msg)); + netconn_apimsg(lwip_netconn_do_accepted, &API_MSG_VAR_REF(msg)); API_MSG_VAR_FREE(msg); #endif /* TCP_LISTEN_BACKLOG */ diff --git a/src/api/api_msg.c b/src/api/api_msg.c index e056b594..6a1060b3 100644 --- a/src/api/api_msg.c +++ b/src/api/api_msg.c @@ -485,6 +485,9 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err) to the application thread */ newconn->last_err = err; + /* handle backlog counter */ + tcp_backlog_delayed(newpcb); + if (sys_mbox_trypost(&conn->acceptmbox, newconn) != ERR_OK) { /* When returning != ERR_OK, the pcb is aborted in tcp_process(), so do nothing here! */ @@ -501,6 +504,8 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err) sys_mbox_free(&newconn->recvmbox); sys_mbox_set_invalid(&newconn->recvmbox); netconn_free(newconn); + /* handle backlog counter */ + tcp_backlog_accepted(newpcb); return ERR_MEM; } else { /* Register event with callback */ @@ -761,8 +766,8 @@ netconn_drain(struct netconn *conn) struct netconn *newconn = (struct netconn *)mem; /* Only tcp pcbs have an acceptmbox, so no need to check conn->type */ /* pcb might be set to NULL already by err_tcp() */ - if (conn->pcb.tcp != NULL) { - tcp_accepted(conn->pcb.tcp); + if (newconn->pcb.tcp != NULL) { + tcp_backlog_accepted(newconn->pcb.tcp); } /* drain recvmbox */ netconn_drain(newconn); @@ -1430,24 +1435,38 @@ lwip_netconn_do_recv(void *m) msg->err = ERR_OK; if (msg->conn->pcb.tcp != NULL) { if (NETCONNTYPE_GROUP(msg->conn->type) == NETCONN_TCP) { -#if TCP_LISTEN_BACKLOG - if (msg->conn->pcb.tcp->state == LISTEN) { - tcp_accepted(msg->conn->pcb.tcp); - } else -#endif /* TCP_LISTEN_BACKLOG */ - { - u32_t remaining = msg->msg.r.len; - do { - u16_t recved = (remaining > 0xffff) ? 0xffff : (u16_t)remaining; - tcp_recved(msg->conn->pcb.tcp, recved); - remaining -= recved; - } while (remaining != 0); - } + u32_t remaining = msg->msg.r.len; + do { + u16_t recved = (remaining > 0xffff) ? 0xffff : (u16_t)remaining; + tcp_recved(msg->conn->pcb.tcp, recved); + remaining -= recved; + } while (remaining != 0); } } TCPIP_APIMSG_ACK(msg); } +#if TCP_LISTEN_BACKLOG +/** Indicate that a TCP pcb has been accepted + * Called from netconn_accept + * + * @param msg the api_msg_msg pointing to the connection + */ +void +lwip_netconn_do_accepted(void *m) +{ + struct api_msg *msg = (struct api_msg*)m; + + msg->err = ERR_OK; + if (msg->conn->pcb.tcp != NULL) { + if (NETCONNTYPE_GROUP(msg->conn->type) == NETCONN_TCP) { + tcp_backlog_accepted(msg->conn->pcb.tcp); + } + } + TCPIP_APIMSG_ACK(msg); +} +#endif /* TCP_LISTEN_BACKLOG */ + /** * See if more data needs to be written from a previous call to netconn_write. * Called initially from lwip_netconn_do_write. If the first call can't send all data diff --git a/src/apps/httpd/httpd.c b/src/apps/httpd/httpd.c index cd2ca3a7..97c393e2 100644 --- a/src/apps/httpd/httpd.c +++ b/src/apps/httpd/httpd.c @@ -2286,12 +2286,10 @@ static err_t http_accept(void *arg, struct tcp_pcb *pcb, err_t err) { struct http_state *hs; - struct tcp_pcb_listen *lpcb = (struct tcp_pcb_listen*)arg; + LWIP_UNUSED_ARG(arg); LWIP_UNUSED_ARG(err); LWIP_DEBUGF(HTTPD_DEBUG, ("http_accept %p / %p\n", (void*)pcb, arg)); - /* Decrease the listen backlog counter */ - tcp_accepted(lpcb); /* Set priority */ tcp_setprio(pcb, HTTPD_TCP_PRIO); @@ -2343,8 +2341,7 @@ httpd_init(void) LWIP_ASSERT("httpd_init: tcp_bind failed", err == ERR_OK); pcb = tcp_listen(pcb); LWIP_ASSERT("httpd_init: tcp_listen failed", pcb != NULL); - /* initialize callback arg and accept callback */ - tcp_arg(pcb, pcb); + /* initialize accept callback */ tcp_accept(pcb, http_accept); } diff --git a/src/core/tcp.c b/src/core/tcp.c index 0f838c16..27af755a 100644 --- a/src/core/tcp.c +++ b/src/core/tcp.c @@ -147,6 +147,78 @@ tcp_tmr(void) } } +#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG +/** Called when a listen pcb is closed. Iterates one pcb list and removes the + * closed listener pcb from pcb->listener if matching. + */ +static void +tcp_remove_listener(struct tcp_pcb *list, struct tcp_pcb_listen *lpcb) +{ + struct tcp_pcb *pcb; + for (pcb = list; pcb != NULL; pcb = pcb->next) { + if (pcb->listener == lpcb) { + pcb->listener = NULL; + } + } +} +#endif + +/** Called when a listen pcb is closed. Iterates all pcb lists and removes the + * closed listener pcb from pcb->listener if matching. + */ +static void +tcp_listen_closed(struct tcp_pcb *pcb) +{ +#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG + size_t i; + LWIP_ASSERT("pcb != NULL", pcb != NULL); + LWIP_ASSERT("pcb->state == LISTEN", pcb->state == LISTEN); + for (i = 1; i < LWIP_ARRAYSIZE(tcp_pcb_lists); i++) { + tcp_remove_listener(*tcp_pcb_lists[i], (struct tcp_pcb_listen*)pcb); + } +#endif + LWIP_UNUSED_ARG(pcb); +} + +#if TCP_LISTEN_BACKLOG +/** Delay accepting a connection in respect to the listen backlog: + * the number of outstanding connections is increased until + * tcp_backlog_accepted() is called. + * + * ATTENTION: the caller is responsible for calling tcp_backlog_accepted() + * or else the backlog feature will get out of sync! + * + * @param pcb the connection pcb which is not fully accepted yet + */ +void +tcp_backlog_delayed(struct tcp_pcb* pcb) +{ + LWIP_ASSERT("pcb != NULL", pcb != NULL); + if (pcb->listener != NULL) { + pcb->listener->accepts_pending++; + LWIP_ASSERT("accepts_pending != 0", pcb->listener->accepts_pending != 0); + } +} + +/** A delayed-accept a connection is accepted (or closed/aborted): decreases + * the number of outstanding connections after calling tcp_backlog_delayed(). + * + * ATTENTION: the caller is responsible for calling tcp_backlog_accepted() + * or else the backlog feature will get out of sync! + * + * @param pcb the connection pcb which is now fully accepted (or closed/aborted) + */ +void +tcp_backlog_accepted(struct tcp_pcb* pcb) +{ + LWIP_ASSERT("pcb != NULL", pcb != NULL); + if (pcb->listener != NULL) { + LWIP_ASSERT("accepts_pending != 0", pcb->listener->accepts_pending != 0); + pcb->listener->accepts_pending--; + } +} +#endif /* TCP_LISTEN_BACKLOG */ + /** * Closes the TX side of a connection held by the PCB. * For tcp_close(), a RST is sent if the application didn't receive all data @@ -217,6 +289,7 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data) case LISTEN: err = ERR_OK; tcp_pcb_remove(&tcp_listen_pcbs.pcbs, pcb); + tcp_listen_closed(pcb); memp_free(MEMP_TCP_PCB_LISTEN, pcb); pcb = NULL; break; @@ -230,6 +303,7 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data) case SYN_RCVD: err = tcp_send_fin(pcb); if (err == ERR_OK) { + tcp_backlog_accepted(pcb); MIB2_STATS_INC(mib2.tcpattemptfails); pcb->state = FIN_WAIT_1; } @@ -1586,9 +1660,10 @@ tcp_err(struct tcp_pcb *pcb, tcp_err_fn err) void tcp_accept(struct tcp_pcb *pcb, tcp_accept_fn accept) { - /* This function is allowed to be called for both listen pcbs and - connection pcbs. */ - pcb->accept = accept; + if (pcb->state == LISTEN) { + struct tcp_pcb_listen *lpcb = (struct tcp_pcb_listen*)pcb; + lpcb->accept = accept; + } } #endif /* LWIP_CALLBACK_API */ @@ -1628,22 +1703,9 @@ tcp_pcb_purge(struct tcp_pcb *pcb) #if TCP_LISTEN_BACKLOG if (pcb->state == SYN_RCVD) { - /* Need to find the corresponding listen_pcb and decrease its accepts_pending */ - struct tcp_pcb_listen *lpcb; - LWIP_ASSERT("tcp_pcb_purge: pcb->state == SYN_RCVD but tcp_listen_pcbs is NULL", - tcp_listen_pcbs.listen_pcbs != NULL); - for (lpcb = tcp_listen_pcbs.listen_pcbs; lpcb != NULL; lpcb = lpcb->next) { - if ((lpcb->local_port == pcb->local_port) && - (IP_IS_V6_VAL(pcb->local_ip) == IP_IS_V6_VAL(lpcb->local_ip)) && - (ip_addr_isany(&lpcb->local_ip) || - ip_addr_cmp(&pcb->local_ip, &lpcb->local_ip))) { - /* port and address of the listen pcb match the timed-out pcb */ - LWIP_ASSERT("tcp_pcb_purge: listen pcb does not have accepts pending", - lpcb->accepts_pending > 0); - lpcb->accepts_pending--; - break; - } - } + tcp_backlog_accepted(pcb); + /* prevent executing this again: */ + pcb->listener = NULL; } #endif /* TCP_LISTEN_BACKLOG */ diff --git a/src/core/tcp_in.c b/src/core/tcp_in.c index 1fba1dcc..3d5cfa0f 100644 --- a/src/core/tcp_in.c +++ b/src/core/tcp_in.c @@ -579,9 +579,9 @@ tcp_listen_input(struct tcp_pcb_listen *pcb) npcb->rcv_ann_right_edge = npcb->rcv_nxt; npcb->snd_wl1 = seqno - 1;/* initialise to seqno-1 to force window update */ npcb->callback_arg = pcb->callback_arg; -#if LWIP_CALLBACK_API - npcb->accept = pcb->accept; -#endif /* LWIP_CALLBACK_API */ +#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG + npcb->listener = pcb; +#endif /* LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG */ /* inherit socket options */ npcb->so_options = pcb->so_options & SOF_INHERITED; /* Register the new PCB so that we can begin receiving segments @@ -784,10 +784,18 @@ tcp_process(struct tcp_pcb *pcb) pcb->state = ESTABLISHED; LWIP_DEBUGF(TCP_DEBUG, ("TCP connection established %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest)); #if LWIP_CALLBACK_API - LWIP_ASSERT("pcb->accept != NULL", pcb->accept != NULL); + LWIP_ASSERT("pcb->listener->accept != NULL", + (pcb->listener == NULL) || (pcb->listener->accept != NULL)); + if (pcb->listener == NULL) { + /* listen pcb might be closed by now */ + err = ERR_VAL; + } else #endif - /* Call the accept function. */ - TCP_EVENT_ACCEPT(pcb, ERR_OK, err); + { + tcp_backlog_accepted(pcb); + /* Call the accept function. */ + TCP_EVENT_ACCEPT(pcb, ERR_OK, err); + } if (err != ERR_OK) { /* If the accept function returns with an error, we abort * the connection. */ diff --git a/src/include/lwip/priv/api_msg.h b/src/include/lwip/priv/api_msg.h index 6d96d803..771e81ff 100644 --- a/src/include/lwip/priv/api_msg.h +++ b/src/include/lwip/priv/api_msg.h @@ -184,6 +184,9 @@ void lwip_netconn_do_disconnect (void *m); void lwip_netconn_do_listen (void *m); void lwip_netconn_do_send (void *m); void lwip_netconn_do_recv (void *m); +#if TCP_LISTEN_BACKLOG +void lwip_netconn_do_accepted (void *m); +#endif /* TCP_LISTEN_BACKLOG */ void lwip_netconn_do_write (void *m); void lwip_netconn_do_getaddr (void *m); void lwip_netconn_do_close (void *m); diff --git a/src/include/lwip/priv/tcp_priv.h b/src/include/lwip/priv/tcp_priv.h index b5261b44..183cee2b 100644 --- a/src/include/lwip/priv/tcp_priv.h +++ b/src/include/lwip/priv/tcp_priv.h @@ -216,11 +216,11 @@ PACK_STRUCT_END #else /* LWIP_EVENT_API */ -#define TCP_EVENT_ACCEPT(pcb,err,ret) \ - do { \ - if((pcb)->accept != NULL) \ - (ret) = (pcb)->accept((pcb)->callback_arg,(pcb),(err)); \ - else (ret) = ERR_ARG; \ +#define TCP_EVENT_ACCEPT(pcb,err,ret) \ + do { \ + if(((pcb)->listener != NULL) && ((pcb)->listener->accept != NULL)) \ + (ret) = (pcb)->listener->accept((pcb)->callback_arg,(pcb),(err)); \ + else (ret) = ERR_ARG; \ } while (0) #define TCP_EVENT_SENT(pcb,space,ret) \ diff --git a/src/include/lwip/tcp.h b/src/include/lwip/tcp.h index c3a98415..cc51f56c 100644 --- a/src/include/lwip/tcp.h +++ b/src/include/lwip/tcp.h @@ -155,34 +155,38 @@ enum tcp_state { TIME_WAIT = 10 }; -#if LWIP_CALLBACK_API - /* Function to call when a listener has been connected. - * @param arg user-supplied argument (tcp_pcb.callback_arg) - * @param pcb a new tcp_pcb that now is connected - * @param err an error argument (TODO: that is current always ERR_OK?) - * @return ERR_OK: accept the new connection, - * any other err_t aborts the new connection - */ -#define DEF_ACCEPT_CALLBACK tcp_accept_fn accept; -#else /* LWIP_CALLBACK_API */ -#define DEF_ACCEPT_CALLBACK -#endif /* LWIP_CALLBACK_API */ - /** * members common to struct tcp_pcb and struct tcp_listen_pcb */ #define TCP_PCB_COMMON(type) \ type *next; /* for the linked list */ \ void *callback_arg; \ - /* the accept callback for listen- and normal pcbs, if LWIP_CALLBACK_API */ \ - DEF_ACCEPT_CALLBACK \ enum tcp_state state; /* TCP state */ \ u8_t prio; \ /* ports are in host byte order */ \ u16_t local_port -/* the TCP protocol control block */ +/** the TCP protocol control block for listening pcbs */ +struct tcp_pcb_listen { +/** Common members of all PCB types */ + IP_PCB; +/** Protocol specific PCB members */ + TCP_PCB_COMMON(struct tcp_pcb_listen); + +#if LWIP_CALLBACK_API + /* Function to call when a listener has been connected. */ + tcp_accept_fn accept; +#endif /* LWIP_CALLBACK_API */ + +#if TCP_LISTEN_BACKLOG + u8_t backlog; + u8_t accepts_pending; +#endif /* TCP_LISTEN_BACKLOG */ +}; + + +/** the TCP protocol control block */ struct tcp_pcb { /** common PCB members */ IP_PCB; @@ -268,6 +272,10 @@ struct tcp_pcb { struct pbuf *refused_data; /* Data previously received but not yet taken by upper layer */ +#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG + struct tcp_pcb_listen* listener; +#endif /* LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG */ + #if LWIP_CALLBACK_API /* Function to be called when more send buffer space is available. */ tcp_sent_fn sent; @@ -307,18 +315,6 @@ struct tcp_pcb { #endif }; -struct tcp_pcb_listen { -/* Common members of all PCB types */ - IP_PCB; -/* Protocol specific PCB members */ - TCP_PCB_COMMON(struct tcp_pcb_listen); - -#if TCP_LISTEN_BACKLOG - u8_t backlog; - u8_t accepts_pending; -#endif /* TCP_LISTEN_BACKLOG */ -}; - #if LWIP_EVENT_API enum lwip_event { @@ -353,21 +349,21 @@ void tcp_err (struct tcp_pcb *pcb, tcp_err_fn err); #define tcp_sndbuf(pcb) (TCPWND16((pcb)->snd_buf)) #define tcp_sndqueuelen(pcb) ((pcb)->snd_queuelen) #define tcp_nagle_disable(pcb) ((pcb)->flags |= TF_NODELAY) -#define tcp_nagle_enable(pcb) ((pcb)->flags = (tcpflags_t)((pcb)->flags & ~TF_NODELAY)) +#define tcp_nagle_enable(pcb) ((pcb)->flags = (u16_t)((pcb)->flags & ~TF_NODELAY)) #define tcp_nagle_disabled(pcb) (((pcb)->flags & TF_NODELAY) != 0) #if TCP_LISTEN_BACKLOG -#define tcp_accepted(pcb) do { \ - LWIP_ASSERT("pcb->state == LISTEN (called for wrong pcb?)", pcb->state == LISTEN); \ - (((struct tcp_pcb_listen *)(pcb))->accepts_pending--); } while(0) #define tcp_backlog_set(pcb, new_backlog) do { \ LWIP_ASSERT("pcb->state == LISTEN (called for wrong pcb?)", (pcb)->state == LISTEN); \ ((struct tcp_pcb_listen *)(pcb))->backlog = ((new_backlog) ? (new_backlog) : 1); } while(0) +void tcp_backlog_delayed(struct tcp_pcb* pcb); +void tcp_backlog_accepted(struct tcp_pcb* pcb); #else /* TCP_LISTEN_BACKLOG */ -#define tcp_accepted(pcb) LWIP_ASSERT("pcb->state == LISTEN (called for wrong pcb?)", \ - (pcb)->state == LISTEN) #define tcp_backlog_set(pcb, new_backlog) +#define tcp_backlog_delayed(pcb) +#define tcp_backlog_accepted(pcb) #endif /* TCP_LISTEN_BACKLOG */ +//#define tcp_accepted(pcb) /* compatibility define, not needed any more */ void tcp_recved (struct tcp_pcb *pcb, u16_t len); err_t tcp_bind (struct tcp_pcb *pcb, const ip_addr_t *ipaddr,