From 0644c4c08ecc7441b76adc422ed9e5347e198111 Mon Sep 17 00:00:00 2001 From: goldsimon Date: Wed, 27 Jan 2010 17:22:06 +0000 Subject: [PATCH] Fixed bug #27871: Calling tcp_abort() in recv callback can lead to accessing unallocated memory. As a consequence, ERR_ABRT means the application has called tcp_abort()! --- CHANGELOG | 5 +++++ doc/rawapi.txt | 5 +++++ src/core/tcp.c | 37 ++++++++++++++++++++++--------------- src/core/tcp_in.c | 23 +++++++++++++++++++++-- src/include/lwip/tcp.h | 12 +++++++++++- 5 files changed, 64 insertions(+), 18 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 87a19c25..9e7e1e57 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -53,6 +53,11 @@ HISTORY ++ Bugfixes: + 2010-01-27: Simon Goldschmidt + * tcp.h, tcp.c, tcp_in.c: Fixed bug #27871: Calling tcp_abort() in recv + callback can lead to accessing unallocated memory. As a consequence, + ERR_ABRT means the application has called tcp_abort()! + 2010-01-25: Simon Goldschmidt * snmp_structs.h, msg_in.c: Partly fixed bug #22070 (MIB_OBJECT_WRITE_ONLY not implemented in SNMP): write-only or not-accessible are still diff --git a/doc/rawapi.txt b/doc/rawapi.txt index 8eec6e78..662a97e0 100644 --- a/doc/rawapi.txt +++ b/doc/rawapi.txt @@ -278,6 +278,11 @@ again when the connection has been idle for a while. Aborts the connection by sending a RST (reset) segment to the remote host. The pcb is deallocated. This function never fails. + ATTENTION: When calling this from one of the TCP callbacks, make + sure you always return ERR_ABRT (and never return ERR_ABRT otherwise + or you will risk accessing deallocated memory or memory leaks! + + If a connection is aborted because of an error, the application is alerted of this event by the err callback. Errors that might abort a connection are when there is a shortage of memory. The callback diff --git a/src/core/tcp.c b/src/core/tcp.c index ee19f389..1ac824db 100644 --- a/src/core/tcp.c +++ b/src/core/tcp.c @@ -760,20 +760,21 @@ tcp_slowtmr(void) memp_free(MEMP_TCP_PCB, pcb); pcb = pcb2; } else { - - /* We check if we should poll the connection. */ - ++pcb->polltmr; - if (pcb->polltmr >= pcb->pollinterval) { - pcb->polltmr = 0; - LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: polling application\n")); - TCP_EVENT_POLL(pcb, err); - if (err == ERR_OK) { - tcp_output(pcb); - } - } - + /* get the 'next' element now and work with 'prev' below (in case of abort) */ prev = pcb; pcb = pcb->next; + + /* We check if we should poll the connection. */ + ++prev->polltmr; + if (prev->polltmr >= prev->pollinterval) { + prev->polltmr = 0; + LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: polling application\n")); + TCP_EVENT_POLL(prev, err); + /* if err == ERR_ABRT, 'prev' is already deallocated */ + if (err == ERR_OK) { + tcp_output(prev); + } + } } } @@ -823,9 +824,10 @@ tcp_slowtmr(void) void tcp_fasttmr(void) { - struct tcp_pcb *pcb; + struct tcp_pcb *pcb = tcp_active_pcbs; - for(pcb = tcp_active_pcbs; pcb != NULL; pcb = pcb->next) { + 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) { /* Notify again application with data previously received. */ @@ -834,16 +836,21 @@ tcp_fasttmr(void) TCP_EVENT_RECV(pcb, pcb->refused_data, ERR_OK, err); if (err == ERR_OK) { pcb->refused_data = NULL; + } else if (err == ERR_ABRT) { + /* if err == ERR_ABRT, 'pcb' is already deallocated */ + pcb = NULL; } } /* send delayed ACKs */ - if (pcb->flags & TF_ACK_DELAY) { + 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); } + + pcb = next; } } diff --git a/src/core/tcp_in.c b/src/core/tcp_in.c index 742f1ec9..48d857e2 100644 --- a/src/core/tcp_in.c +++ b/src/core/tcp_in.c @@ -280,6 +280,7 @@ tcp_input(struct pbuf *p, struct netif *inp) if (err == ERR_OK) { pcb->refused_data = NULL; } else { + /* if err == ERR_ABRT, 'pcb' is already deallocated */ /* drop incoming packets, because pcb is "full" */ LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_input: drop incoming packets, because pcb is \"full\"\n")); TCP_STATS_INC(tcp.drop); @@ -313,8 +314,11 @@ tcp_input(struct pbuf *p, struct netif *inp) now. */ if (pcb->acked > 0) { TCP_EVENT_SENT(pcb, pcb->acked, err); + if (err == ERR_ABRT) { + goto aborted; + } } - + if (recv_data != NULL) { if(flags & TCP_PSH) { recv_data->flags |= PBUF_FLAG_PUSH; @@ -322,6 +326,9 @@ tcp_input(struct pbuf *p, struct netif *inp) /* Notify application that data has been received. */ TCP_EVENT_RECV(pcb, recv_data, ERR_OK, err); + if (err == ERR_ABRT) { + goto aborted; + } /* If the upper layer can't receive this data, store it */ if (err != ERR_OK) { @@ -334,6 +341,9 @@ tcp_input(struct pbuf *p, struct netif *inp) function with a NULL buffer to indicate EOF. */ if (recv_flags & TF_GOT_FIN) { TCP_EVENT_RECV(pcb, NULL, ERR_OK, err); + if (err == ERR_ABRT) { + goto aborted; + } } tcp_input_pcb = NULL; @@ -346,6 +356,9 @@ tcp_input(struct pbuf *p, struct netif *inp) #endif /* TCP_INPUT_DEBUG */ } } + /* Jump target if pcb has been aborted in a callback (by calling tcp_abort()). + Below this line, 'pcb' may not be dereferenced! */ +aborted: tcp_input_pcb = NULL; @@ -616,6 +629,9 @@ tcp_process(struct tcp_pcb *pcb) /* Call the user specified function to call when sucessfully * connected. */ TCP_EVENT_CONNECTED(pcb, ERR_OK, err); + if (err == ERR_ABRT) { + return ERR_ABRT; + } tcp_ack_now(pcb); } /* received ACK? possibly a half-open connection */ @@ -640,7 +656,10 @@ tcp_process(struct tcp_pcb *pcb) if (err != ERR_OK) { /* If the accept function returns with an error, we abort * the connection. */ - tcp_abort(pcb); + /* Already aborted? */ + if (err != ERR_ABRT) { + tcp_abort(pcb); + } return ERR_ABRT; } old_cwnd = pcb->cwnd; diff --git a/src/include/lwip/tcp.h b/src/include/lwip/tcp.h index 7cb86ab5..dc392ac9 100644 --- a/src/include/lwip/tcp.h +++ b/src/include/lwip/tcp.h @@ -54,7 +54,9 @@ struct tcp_pcb; * * @param arg Additional argument to pass to the callback function (@see tcp_arg()) * @param newpcb The new connection pcb - * @param err An error code if there has been an error accepting + * @param err An error code if there has been an error accepting. + * Only return ERR_ABRT if you have called tcp_abort from within the + * callback function! */ typedef err_t (*tcp_accept_fn)(void *arg, struct tcp_pcb *newpcb, err_t err); @@ -65,6 +67,8 @@ typedef err_t (*tcp_accept_fn)(void *arg, struct tcp_pcb *newpcb, err_t err); * @param tpcb The connection pcb which received data * @param p The received data (or NULL when the connection has been closed!) * @param err An error code if there has been an error receiving + * Only return ERR_ABRT if you have called tcp_abort from within the + * callback function! */ typedef err_t (*tcp_recv_fn)(void *arg, struct tcp_pcb *tpcb, struct pbuf *p, err_t err); @@ -77,6 +81,8 @@ typedef err_t (*tcp_recv_fn)(void *arg, struct tcp_pcb *tpcb, * @param tpcb The connection pcb for which data has been acknowledged * @param len The amount of bytes acknowledged * @return ERR_OK: try to send some data by calling tcp_output + * Only return ERR_ABRT if you have called tcp_abort from within the + * callback function! */ typedef err_t (*tcp_sent_fn)(void *arg, struct tcp_pcb *tpcb, u16_t len); @@ -87,6 +93,8 @@ typedef err_t (*tcp_sent_fn)(void *arg, struct tcp_pcb *tpcb, * @param arg Additional argument to pass to the callback function (@see tcp_arg()) * @param tpcb tcp pcb * @return ERR_OK: try to send some data by calling tcp_output + * Only return ERR_ABRT if you have called tcp_abort from within the + * callback function! */ typedef err_t (*tcp_poll_fn)(void *arg, struct tcp_pcb *tpcb); @@ -109,6 +117,8 @@ typedef void (*tcp_err_fn)(void *arg, err_t err); * @param arg Additional argument to pass to the callback function (@see tcp_arg()) * @param tpcb The connection pcb which is connected * @param err An unused error code, always ERR_OK currently ;-) TODO! + * Only return ERR_ABRT if you have called tcp_abort from within the + * callback function! * * @note When a connection attempt fails, the error callback is currently called! */