From dd8729063c34c0073ae98190b0174bd045d240ca Mon Sep 17 00:00:00 2001 From: goldsimon Date: Fri, 25 Nov 2011 18:36:52 +0100 Subject: [PATCH] fixed bug #31177: tcp timers can corrupt tcp_active_pcbs in some cases --- CHANGELOG | 4 ++ src/core/tcp.c | 77 +++++++++++++++++++++++++++---------- src/core/tcp_in.c | 8 ++-- src/include/lwip/tcp.h | 1 + src/include/lwip/tcp_impl.h | 19 +++++++++ 5 files changed, 85 insertions(+), 24 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3d827ca7..bfef4810 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -69,6 +69,10 @@ HISTORY ++ Bugfixes: + 2011-11-25: Simon Goldschmidt + * tcp.h/.c, tcp_impl.h, tcp_in.c: fixed bug #31177: tcp timers can corrupt + tcp_active_pcbs in some cases + 2011-11-23: Simon Goldschmidt * sys.c: fixed bug #34884: sys_msleep() body needs to be surrounded with '#ifndef sys_msleep' diff --git a/src/core/tcp.c b/src/core/tcp.c index 1fa75153..e981d530 100644 --- a/src/core/tcp.c +++ b/src/core/tcp.c @@ -119,8 +119,11 @@ struct tcp_pcb ** const tcp_pcb_lists[] = {&tcp_listen_pcbs.pcbs, &tcp_bound_pcb /** Only used for temporary storage. */ struct tcp_pcb *tcp_tmp_pcb; +u8_t tcp_active_pcbs_changed; + /** Timer counter to handle calling slow-timer from tcp_tmr() */ static u8_t tcp_timer; +static u8_t tcp_timer_ctr; static u16_t tcp_new_port(void); /** @@ -187,7 +190,7 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data) /* TODO: to which state do we move now? */ /* move to TIME_WAIT since we close actively */ - TCP_RMV(&tcp_active_pcbs, pcb); + TCP_RMV_ACTIVE(pcb); pcb->state = TIME_WAIT; TCP_REG(&tcp_tw_pcbs, pcb); @@ -219,7 +222,7 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data) break; case SYN_SENT: err = ERR_OK; - tcp_pcb_remove(&tcp_active_pcbs, pcb); + TCP_PCB_REMOVE_ACTIVE(pcb); memp_free(MEMP_TCP_PCB, pcb); pcb = NULL; snmp_inc_tcpattemptfails(); @@ -371,7 +374,7 @@ tcp_abandon(struct tcp_pcb *pcb, int reset) errf = pcb->errf; #endif /* LWIP_CALLBACK_API */ errf_arg = pcb->callback_arg; - tcp_pcb_remove(&tcp_active_pcbs, pcb); + TCP_PCB_REMOVE_ACTIVE(pcb); if (pcb->unacked != NULL) { tcp_segs_free(pcb->unacked); } @@ -793,7 +796,7 @@ tcp_connect(struct tcp_pcb *pcb, ip_addr_t *ipaddr, u16_t port, if (old_local_port != 0) { TCP_RMV(&tcp_bound_pcbs, pcb); } - TCP_REG(&tcp_active_pcbs, pcb); + TCP_REG_ACTIVE(pcb); snmp_inc_tcpactiveopens(); tcp_output(pcb); @@ -820,7 +823,9 @@ tcp_slowtmr(void) err = ERR_OK; ++tcp_ticks; + ++tcp_timer_ctr; +tcp_slowtmr_start: /* Steps through all of the active PCBs. */ prev = NULL; pcb = tcp_active_pcbs; @@ -832,6 +837,12 @@ tcp_slowtmr(void) LWIP_ASSERT("tcp_slowtmr: active pcb->state != CLOSED\n", pcb->state != CLOSED); LWIP_ASSERT("tcp_slowtmr: active pcb->state != LISTEN\n", pcb->state != LISTEN); LWIP_ASSERT("tcp_slowtmr: active pcb->state != TIME-WAIT\n", pcb->state != TIME_WAIT); + if (pcb->last_timer == tcp_timer_ctr) { + /* skip this pcb, we have already processed it */ + pcb = pcb->next; + continue; + } + pcb->last_timer = tcp_timer_ctr; pcb_remove = 0; pcb_reset = 0; @@ -957,6 +968,8 @@ tcp_slowtmr(void) /* If the PCB should be removed, do it. */ if (pcb_remove) { struct tcp_pcb *pcb2; + tcp_err_fn err_fn; + void *err_arg; tcp_pcb_purge(pcb); /* Remove PCB from tcp_active_pcbs list. */ if (prev != NULL) { @@ -968,15 +981,22 @@ tcp_slowtmr(void) tcp_active_pcbs = pcb->next; } - TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_ABRT); if (pcb_reset) { tcp_rst(pcb->snd_nxt, pcb->rcv_nxt, &pcb->local_ip, &pcb->remote_ip, pcb->local_port, pcb->remote_port, PCB_ISIPV6(pcb)); } + err_fn = pcb->errf; + err_arg = pcb->callback_arg; pcb2 = pcb; pcb = pcb->next; memp_free(MEMP_TCP_PCB, pcb2); + + tcp_active_pcbs_changed = 0; + TCP_EVENT_ERR(err_fn, err_arg, ERR_ABRT); + if (tcp_active_pcbs_changed) { + goto tcp_slowtmr_start; + } } else { /* get the 'next' element now and work with 'prev' below (in case of abort) */ prev = pcb; @@ -987,7 +1007,11 @@ tcp_slowtmr(void) if (prev->polltmr >= prev->pollinterval) { prev->polltmr = 0; LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: polling application\n")); + tcp_active_pcbs_changed = 0; TCP_EVENT_POLL(prev, err); + if (tcp_active_pcbs_changed) { + goto tcp_slowtmr_start; + } /* if err == ERR_ABRT, 'prev' is already deallocated */ if (err == ERR_OK) { tcp_output(prev); @@ -1043,26 +1067,38 @@ tcp_slowtmr(void) void tcp_fasttmr(void) { - struct tcp_pcb *pcb = tcp_active_pcbs; + struct tcp_pcb *pcb; + + ++tcp_timer_ctr; + +tcp_fasttmr_start: + pcb = tcp_active_pcbs; while(pcb != NULL) { - struct tcp_pcb *next = pcb->next; - /* If there is data which was previously "refused" by upper layer */ - if (pcb->refused_data != NULL) { - if (tcp_process_refused_data(pcb) == ERR_ABRT) { - pcb = NULL; + if (pcb->last_timer != tcp_timer_ctr) { + struct tcp_pcb *next; + pcb->last_timer = tcp_timer_ctr; + /* send delayed ACKs */ + if (pcb->flags & TF_ACK_DELAY) { + LWIP_DEBUGF(TCP_DEBUG, ("tcp_fasttmr: delayed ACK\n")); + tcp_ack_now(pcb); + tcp_output(pcb); + pcb->flags &= ~(TF_ACK_DELAY | TF_ACK_NOW); } - } - /* send delayed ACKs */ - if (pcb && (pcb->flags & TF_ACK_DELAY)) { - LWIP_DEBUGF(TCP_DEBUG, ("tcp_fasttmr: delayed ACK\n")); - tcp_ack_now(pcb); - tcp_output(pcb); - pcb->flags &= ~(TF_ACK_DELAY | TF_ACK_NOW); - } + next = pcb->next; - pcb = next; + /* If there is data which was previously "refused" by upper layer */ + if (pcb->refused_data != NULL) { + tcp_active_pcbs_changed = 0; + tcp_process_refused_data(pcb); + if (tcp_active_pcbs_changed) { + /* application callback has changed the pcb list: restart the loop */ + goto tcp_fasttmr_start; + } + } + pcb = next; + } } } @@ -1312,6 +1348,7 @@ tcp_alloc(u8_t prio) pcb->lastack = iss; pcb->snd_lbb = iss; pcb->tmr = tcp_ticks; + pcb->last_timer = tcp_timer_ctr; pcb->polltmr = 0; diff --git a/src/core/tcp_in.c b/src/core/tcp_in.c index 0dd2c904..6af07211 100644 --- a/src/core/tcp_in.c +++ b/src/core/tcp_in.c @@ -505,7 +505,7 @@ tcp_listen_input(struct tcp_pcb_listen *pcb) npcb->so_options = pcb->so_options & SOF_INHERITED; /* Register the new PCB so that we can begin receiving segments for it. */ - TCP_REG(&tcp_active_pcbs, npcb); + TCP_REG_ACTIVE(npcb); /* Parse any options in the SYN. */ tcp_parseopt(npcb); @@ -755,7 +755,7 @@ tcp_process(struct tcp_pcb *pcb) ("TCP connection closed: FIN_WAIT_1 %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest)); tcp_ack_now(pcb); tcp_pcb_purge(pcb); - TCP_RMV(&tcp_active_pcbs, pcb); + TCP_RMV_ACTIVE(pcb); pcb->state = TIME_WAIT; TCP_REG(&tcp_tw_pcbs, pcb); } else { @@ -772,7 +772,7 @@ tcp_process(struct tcp_pcb *pcb) LWIP_DEBUGF(TCP_DEBUG, ("TCP connection closed: FIN_WAIT_2 %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest)); tcp_ack_now(pcb); tcp_pcb_purge(pcb); - TCP_RMV(&tcp_active_pcbs, pcb); + TCP_RMV_ACTIVE(pcb); pcb->state = TIME_WAIT; TCP_REG(&tcp_tw_pcbs, pcb); } @@ -782,7 +782,7 @@ tcp_process(struct tcp_pcb *pcb) if (flags & TCP_ACK && ackno == pcb->snd_nxt) { LWIP_DEBUGF(TCP_DEBUG, ("TCP connection closed: CLOSING %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest)); tcp_pcb_purge(pcb); - TCP_RMV(&tcp_active_pcbs, pcb); + TCP_RMV_ACTIVE(pcb); pcb->state = TIME_WAIT; TCP_REG(&tcp_tw_pcbs, pcb); } diff --git a/src/include/lwip/tcp.h b/src/include/lwip/tcp.h index 6af71583..4dc1df2a 100644 --- a/src/include/lwip/tcp.h +++ b/src/include/lwip/tcp.h @@ -191,6 +191,7 @@ struct tcp_pcb { /* Timers */ u8_t polltmr, pollinterval; + u8_t last_timer; u32_t tmr; /* receiver variables */ diff --git a/src/include/lwip/tcp_impl.h b/src/include/lwip/tcp_impl.h index 9b745a0d..b068eef2 100644 --- a/src/include/lwip/tcp_impl.h +++ b/src/include/lwip/tcp_impl.h @@ -309,6 +309,7 @@ struct tcp_seg { /* Global variables: */ extern struct tcp_pcb *tcp_input_pcb; extern u32_t tcp_ticks; +extern u8_t tcp_active_pcbs_changed; /* The TCP PCB lists. */ union tcp_listen_pcbs_t { /* List of all TCP PCBs in LISTEN state. */ @@ -395,6 +396,24 @@ extern struct tcp_pcb *tcp_tmp_pcb; /* Only used for temporary storage. */ #endif /* LWIP_DEBUG */ +#define TCP_REG_ACTIVE(npcb) \ + do { \ + TCP_REG(&tcp_active_pcbs, npcb); \ + tcp_active_pcbs_changed = 1; \ + } while (0) + +#define TCP_RMV_ACTIVE(npcb) \ + do { \ + TCP_RMV(&tcp_active_pcbs, npcb); \ + tcp_active_pcbs_changed = 1; \ + } while (0) + +#define TCP_PCB_REMOVE_ACTIVE(pcb) \ + do { \ + tcp_pcb_remove(&tcp_active_pcbs, pcb); \ + tcp_active_pcbs_changed = 1; \ + } while (0) + /* Internal functions: */ struct tcp_pcb *tcp_pcb_copy(struct tcp_pcb *pcb);