From 23a869d9be23a33962c55a1e6f62e6b91b2cbfc4 Mon Sep 17 00:00:00 2001 From: patacongo Date: Thu, 23 Feb 2012 02:07:38 +0000 Subject: (1) Fix a critical memory leak in the TCP read-ahead buffering logic; Add an option to suppress SDIO multi-block transfers in order to work around a buggy SDIO driver git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@4415 42af7a65-404d-4744-a932-0658087f49c3 --- nuttx/net/recvfrom.c | 112 ++++++++++++++++++++++++++--- nuttx/net/uip/uip_internal.h | 12 ++-- nuttx/net/uip/uip_tcpcallback.c | 152 ++++++++++++++++++++++++++-------------- 3 files changed, 213 insertions(+), 63 deletions(-) (limited to 'nuttx/net') diff --git a/nuttx/net/recvfrom.c b/nuttx/net/recvfrom.c index aabfcedfd..ee8f100bf 100644 --- a/nuttx/net/recvfrom.c +++ b/nuttx/net/recvfrom.c @@ -38,6 +38,7 @@ ****************************************************************************/ #include + #ifdef CONFIG_NET #include @@ -103,7 +104,7 @@ struct recvfrom_s * pstate recvfrom state structure * * Returned Value: - * None + * The number of bytes taken from the packet. * * Assumptions: * Running at the interrupt level @@ -111,7 +112,8 @@ struct recvfrom_s ****************************************************************************/ #if defined(CONFIG_NET_UDP) || defined(CONFIG_NET_TCP) -static void recvfrom_newdata(struct uip_driver_s *dev, struct recvfrom_s *pstate) +static size_t recvfrom_newdata(FAR struct uip_driver_s *dev, + FAR struct recvfrom_s *pstate) { size_t recvlen; @@ -137,11 +139,105 @@ static void recvfrom_newdata(struct uip_driver_s *dev, struct recvfrom_s *pstate pstate->rf_buffer += recvlen; pstate->rf_buflen -= recvlen; + return recvlen; +} +#endif /* CONFIG_NET_UDP || CONFIG_NET_TCP */ + +/**************************************************************************** + * Function: recvfrom_newtcpdata + * + * Description: + * Copy the read data from the packet + * + * Parameters: + * dev The sructure of the network driver that caused the interrupt + * pstate recvfrom state structure + * + * Returned Value: + * None. + * + * Assumptions: + * Running at the interrupt level + * + ****************************************************************************/ + +#ifdef CONFIG_NET_TCP +static inline void recvfrom_newtcpdata(FAR struct uip_driver_s *dev, + FAR struct recvfrom_s *pstate) +{ + /* Take as much data from the packet as we can */ + + size_t recvlen = recvfrom_newdata(dev, pstate); + + /* If there is more data left in the packet that we could not buffer, than + * add it to the read-ahead buffers. + */ + + if (recvlen < dev->d_len) + { +#if CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 + FAR struct uip_conn *conn = (FAR struct uip_conn *)pstate->rf_sock->s_conn; + FAR uint8_t *buffer = (FAR uint8_t *)dev->d_appdata + recvlen; + uint16_t buflen = dev->d_len - recvlen; + uint16_t nsaved; + + nsaved = uip_datahandler(conn, buffer, buflen); + + /* There are complicated buffering issues that are not addressed + * properly here. For example, what if up_datahandler() cannot buffer + * the remainder of the packet? In that case, the data will be dropped + * but still ACKed. Therefore it will not be resent. Fixing this could be + * tricky. + */ + +#ifdef CONFIG_DEBUG_NET + if (nsaved < buflen) + { + ndbg("ERROR: packet data not saved (%d bytes)\n", buflen - nsaved); + } +#endif +#else + ndbg("ERROR: packet data lost (%d bytes)\n", dev->d_len - recvlen); +#endif + } + /* Indicate no data in the buffer */ dev->d_len = 0; } -#endif /* CONFIG_NET_UDP || CONFIG_NET_TCP */ +#endif /* CONFIG_NET_TCP */ + +/**************************************************************************** + * Function: recvfrom_newudpdata + * + * Description: + * Copy the read data from the packet + * + * Parameters: + * dev The sructure of the network driver that caused the interrupt + * pstate recvfrom state structure + * + * Returned Value: + * None. + * + * Assumptions: + * Running at the interrupt level + * + ****************************************************************************/ + +#ifdef CONFIG_NET_TCP +static inline void recvfrom_newudpdata(FAR struct uip_driver_s *dev, + FAR struct recvfrom_s *pstate) +{ + /* Take as much data from the packet as we can */ + + (void)recvfrom_newdata(dev, pstate); + + /* Indicate no data in the buffer */ + + dev->d_len = 0; +} +#endif /* CONFIG_NET_TCP */ /**************************************************************************** * Function: recvfrom_readahead @@ -164,9 +260,9 @@ static void recvfrom_newdata(struct uip_driver_s *dev, struct recvfrom_s *pstate #if defined(CONFIG_NET_TCP) && CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 static inline void recvfrom_readahead(struct recvfrom_s *pstate) { - struct uip_conn *conn = (struct uip_conn *)pstate->rf_sock->s_conn; - struct uip_readahead_s *readahead; - size_t recvlen; + FAR struct uip_conn *conn = (FAR struct uip_conn *)pstate->rf_sock->s_conn; + FAR struct uip_readahead_s *readahead; + size_t recvlen; /* Check there is any TCP data already buffered in a read-ahead * buffer. @@ -375,7 +471,7 @@ static uint16_t recvfrom_tcpinterrupt(struct uip_driver_s *dev, void *conn, { /* Copy the data from the packet */ - recvfrom_newdata(dev, pstate); + recvfrom_newtcpdata(dev, pstate); /* Save the sender's address in the caller's 'from' location */ @@ -572,7 +668,7 @@ static uint16_t recvfrom_udpinterrupt(struct uip_driver_s *dev, void *pvconn, { /* Copy the data from the packet */ - recvfrom_newdata(dev, pstate); + recvfrom_newudpdata(dev, pstate); /* We are finished. */ diff --git a/nuttx/net/uip/uip_internal.h b/nuttx/net/uip/uip_internal.h index aab7408da..01c018c51 100644 --- a/nuttx/net/uip/uip_internal.h +++ b/nuttx/net/uip/uip_internal.h @@ -1,8 +1,8 @@ /**************************************************************************** * net/uip/uip_internal.h * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt + * Copyright (C) 2007-2009, 2012 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt * * This logic was leveraged from uIP which also has a BSD-style license: * @@ -154,8 +154,12 @@ EXTERN void uip_tcpinput(struct uip_driver_s *dev); /* Defined in uip_tcpcallback.c *********************************************/ -EXTERN uint16_t uip_tcpcallback(struct uip_driver_s *dev, - struct uip_conn *conn, uint16_t flags); +EXTERN uint16_t uip_tcpcallback(FAR struct uip_driver_s *dev, + FAR struct uip_conn *conn, uint16_t flags); +#if CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 +EXTERN uint16_t uip_datahandler(FAR struct uip_conn *conn, + FAR uint8_t *buffer, uint16_t nbytes); +#endif /* Defined in uip_tcpreadahead.c ********************************************/ diff --git a/nuttx/net/uip/uip_tcpcallback.c b/nuttx/net/uip/uip_tcpcallback.c index 3f6d04c06..099c29bf3 100644 --- a/nuttx/net/uip/uip_tcpcallback.c +++ b/nuttx/net/uip/uip_tcpcallback.c @@ -38,6 +38,7 @@ ****************************************************************************/ #include + #if defined(CONFIG_NET) && defined(CONFIG_NET_TCP) #include @@ -102,18 +103,19 @@ static int uip_readahead(struct uip_readahead_s *readahead, uint8_t *buf, * Function: uip_dataevent * * Description: - * This is the default data event handler that is called when there is no - * user data handler in place + * Handle data that is not accepted by the application because there is no + * listener in place ready to receive the data. * * Assumptions: - * - The called has checked that UIP_NEWDATA is set in flags and that is no + * - The caller has checked that UIP_NEWDATA is set in flags and that is no * other handler available to process the incoming data. * - This function is called at the interrupt level with interrupts disabled. * ****************************************************************************/ static inline uint16_t -uip_dataevent(struct uip_driver_s *dev, struct uip_conn *conn, uint16_t flags) +uip_dataevent(FAR struct uip_driver_s *dev, FAR struct uip_conn *conn, + uint16_t flags) { uint16_t ret; @@ -130,61 +132,25 @@ uip_dataevent(struct uip_driver_s *dev, struct uip_conn *conn, uint16_t flags) if (dev->d_len > 0) { #if CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 - struct uip_readahead_s *readahead1; - struct uip_readahead_s *readahead2 = NULL; - uint16_t recvlen = 0; - uint8_t *buf = dev->d_appdata; - int buflen = dev->d_len; + uint8_t *buffer = dev->d_appdata; + int buflen = dev->d_len; + uint16_t recvlen; #endif nllvdbg("No listener on connection\n"); #if CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 - /* First, we need to determine if we have space to buffer the data. This - * needs to be verified before we actually begin buffering the data. We - * will use any remaining space in the last allocated readahead buffer - * plus as much one additional buffer. It is expected that the size of - * readahead buffers are tuned so that one full packet will always fit - * into one readahead buffer (for example if the buffer size is 420, then - * a readahead buffer of 366 will hold a full packet of TCP data). - */ + /* Save as much data as possible in the read-ahead buffers */ - readahead1 = (struct uip_readahead_s*)conn->readahead.tail; - if ((readahead1 && - (CONFIG_NET_TCP_READAHEAD_BUFSIZE - readahead1->rh_nbytes) > buflen) || - (readahead2 = uip_tcpreadaheadalloc()) != NULL) - { - /* We have buffer space. Now try to append add as much data as possible - * to the last readahead buffer attached to this connection. - */ - - if (readahead1) - { - recvlen = uip_readahead(readahead1, buf, buflen); - if (recvlen > 0) - { - buf += recvlen; - buflen -= recvlen; - } - } + recvlen = uip_datahandler(conn, buffer, buflen); - /* Do we need to buffer into the newly allocated buffer as well? */ - - if (readahead2) - { - readahead2->rh_nbytes = 0; - (void)uip_readahead(readahead2, buf, buflen); - - /* Save the readahead buffer in the connection structure where - * it can be found with recv() is called. - */ - - sq_addlast(&readahead2->rh_node, &conn->readahead); - } + /* There are several complicated buffering issues that are not addressed + * properly here. For example, what if we cannot buffer the entire + * packet? In that case, some data will be accepted but not ACKed. + * Therefore it will be resent and duplicated. Fixing this could be tricky. + */ - nllvdbg("Buffered %d bytes\n", dev->d_len); - } - else + if (recvlen < buflen) #endif { /* There is no handler to receive new data and there are no free @@ -286,4 +252,88 @@ uint16_t uip_tcpcallback(struct uip_driver_s *dev, struct uip_conn *conn, return ret; } +/**************************************************************************** + * Function: uip_datahandler + * + * Description: + * Handle data that is not accepted by the application. This may be called + * either (1) from the data receive logic if it cannot buffer the data, or + * (2) from the TCP event logic is there is no listener in place ready to + * receive the data. + * + * Input Parmeters: + * conn - A pointer to the TCP connection structure + * buffer - A pointer to the buffer to be copied to the read-ahead + * buffers + * buflen - The number of bytes to copy to the read-ahead buffer. + * + * Returned value: + * The number of bytes actually buffered. This could be less than 'nbytes' + * if there is insufficient buffering available. + * + * Assumptions: + * - The caller has checked that UIP_NEWDATA is set in flags and that is no + * other handler available to process the incoming data. + * - This function is called at the interrupt level with interrupts disabled. + * + ****************************************************************************/ + +#if CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 +uint16_t uip_datahandler(FAR struct uip_conn *conn, FAR uint8_t *buffer, + uint16_t buflen) +{ + FAR struct uip_readahead_s *readahead1; + FAR struct uip_readahead_s *readahead2 = NULL; + uint16_t remaining; + uint16_t recvlen = 0; + + /* First, we need to determine if we have space to buffer the data. This + * needs to be verified before we actually begin buffering the data. We + * will use any remaining space in the last allocated readahead buffer + * plus as much one additional buffer. It is expected that the size of + * readahead buffers are tuned so that one full packet will always fit + * into one readahead buffer (for example if the buffer size is 420, then + * a readahead buffer of 366 will hold a full packet of TCP data). + */ + + readahead1 = (FAR struct uip_readahead_s*)conn->readahead.tail; + if ((readahead1 && + (CONFIG_NET_TCP_READAHEAD_BUFSIZE - readahead1->rh_nbytes) > buflen) || + (readahead2 = uip_tcpreadaheadalloc()) != NULL) + { + /* We have buffer space. Now try to append add as much data as possible + * to the last readahead buffer attached to this connection. + */ + + remaining = buflen; + if (readahead1) + { + recvlen = uip_readahead(readahead1, buffer, remaining); + if (recvlen > 0) + { + buffer += recvlen; + remaining -= recvlen; + } + } + + /* Do we need to buffer into the newly allocated buffer as well? */ + + if (readahead2) + { + readahead2->rh_nbytes = 0; + recvlen += uip_readahead(readahead2, buffer, remaining); + + /* Save the readahead buffer in the connection structure where + * it can be found with recv() is called. + */ + + sq_addlast(&readahead2->rh_node, &conn->readahead); + } + } + + nllvdbg("Buffered %d bytes (of %d)\n", recvlen, buflen); + return recvlen; +} +#endif /* CONFIG_NET_NTCP_READAHEAD_BUFFERS > 0 */ + #endif /* CONFIG_NET && CONFIG_NET_TCP */ -- cgit v1.2.3