net/udp: Fix memory leak with UDP + write buffer is closed. Also update TODO and comments and refresh a configuration.

2018-01-23 11:54:03 -06:00 · 2018-01-23 11:54:03 -06:00 · 82cb799bb6
commit 82cb799bb6
parent f2017bded8
6 changed files with 49 additions and 31 deletions
--- a/30
+++ b/30
@ -1,4 +1,4 @@
-NuttX TODO List (Last updated January 21, 2018)
+NuttX TODO List (Last updated January 23, 2018)
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

 This file summarizes known NuttX bugs, limitations, inconsistencies with
@ -19,7 +19,7 @@ nuttx/:
  (8)  Kernel/Protected Build
  (3)  C++ Support
  (6)  Binary loaders (binfmt/)
- (17)  Network (net/, drivers/net)
+ (16)  Network (net/, drivers/net)
  (4)  USB (drivers/usbdev, drivers/usbhost)
  (0)  Other drivers (drivers/)
 (12)  Libraries (libc/, libm/)
@ -1075,23 +1075,19 @@ o Network (net/, drivers/net)
  Title:       POLL/SELECT ON TCP/UDP SOCKETS NEEDS READ-AHEAD
  Description: poll()/select() only works for availability of buffered TCP/UDP
               read data (when read-ahead is enabled).  The way writing is
-               handled in the network layer, all sockets must wait when send and
-               cannot be notified when they can send without waiting.
+               handled in the network layer, either (1) If CONFIG_UDP/TCP_WRITE_BUFFERS=y
+               then we never have to wait to send; otherwise, we always have
+               to wait to send.  So it is impossible to notify the caller
+               when it can send without waiting.
+
+               An exception "never having to wait" is the case where we are
+               out of memory for use in write buffering.  In that case, the
+               blocking send()/sendto() would have to wait for the memory
+               to become available.
  Status:      Open, probably will not be fixed.
  Priority:    Medium... this does effect porting of applications that expect
               different behavior from poll()/select()

-  Title:       SOCKETS DO NOT ALWAYS SUPPORT O_NONBLOCK
-  Description: sockets do not support all modes for O_NONBLOCK. Sockets
-               support nonblocking operations only (1) for TCP/IP non-
-               blocking read operations when read-ahead buffering is
-               enabled, (2) TCP/IP accept() operations when TCP/IP
-               connection backlog is enabled, (2) UDP/IP read() operations
-               when UDP read-ahead is enabled, and (3) non-blocking
-               operations on Unix domain sockets.
-  Status:      Open
-  Priority:    Low.
-
  Title:       INTERFACES TO LEAVE/JOIN IGMP MULTICAST GROUP
  Description: The interfaces used to leave/join IGMP multicast groups is non-standard.
               RFC3678 (IGMPv3) suggests ioctl() commands to do this (SIOCSIPMSFILTER) but
@ -1124,8 +1120,8 @@ o Network (net/, drivers/net)
               however.  Others support the address filtering interfaces but
               have never been verifed:

-               C5471, LM3S, ez80, DM0x90 NIC, PIC: Do not support address
-                 filtering.
+               C5471, LM3S, ez80, DM0x90 NIC, PIC, LPC54: Do not support
+                 address filtering.
               Kinetis, LPC17xx, LPC43xx:  Untested address filter support

  Status:      Open
--- a/configs/viewtool-stm32f107/README.txt
+++ b/configs/viewtool-stm32f107/README.txt
@ -630,12 +630,6 @@ Configurations
    6. USB support is disabled by default.  See the section above entitled,
       "USB Interface"

-    STATUS.  The first time I build the configuration, I get some undefined
-    external references.  No idea why.  Simply cleaning the apps/ directory
-    and rebuilding fixes the problem:
-
-      make apps_clean all
-
  nsh:

    This configuration directory provide the basic NuttShell (NSH).
--- a/configs/viewtool-stm32f107/netnsh/defconfig
+++ b/configs/viewtool-stm32f107/netnsh/defconfig
@ -1,5 +1,4 @@
 # CONFIG_NSH_ARGCAT is not set
-# CONFIG_NSH_CMDOPT_DF_H is not set
 # CONFIG_NSH_CMDOPT_HEXDUMP is not set
 # CONFIG_NSH_CMDPARMS is not set
 CONFIG_ARCH_BOARD_VIEWTOOL_STM32F107=y
@ -23,9 +22,15 @@ CONFIG_NET_ARP_SEND=y
 CONFIG_NET_BROADCAST=y
 CONFIG_NET_ETH_MTU=650
 CONFIG_NET_ETH_TCP_RECVWNDO=624
+CONFIG_NET_HOSTNAME="Viewtool-STM32F107"
 CONFIG_NET_ICMP_SOCKET=y
 CONFIG_NET_ICMP=y
+CONFIG_NET_ICMPv6_NEIGHBOR=y
+CONFIG_NET_ICMPv6_SOCKET=y
+CONFIG_NET_ICMPv6=y
+CONFIG_NET_IPv6=y
 CONFIG_NET_MAX_LISTENPORTS=40
+CONFIG_NET_ROUTE=y
 CONFIG_NET_SOCKOPTS=y
 CONFIG_NET_TCP_CONNS=40
 CONFIG_NET_TCP_WRITE_BUFFERS=y
@ -56,9 +61,9 @@ CONFIG_SCHED_HPWORK=y
 CONFIG_SCHED_HPWORKPRIORITY=192
 CONFIG_SCHED_HPWORKSTACKSIZE=1024
 CONFIG_SDCLONE_DISABLE=y
-CONFIG_START_DAY=21
-CONFIG_START_MONTH=9
-CONFIG_START_YEAR=2009
+CONFIG_START_DAY=23
+CONFIG_START_MONTH=1
+CONFIG_START_YEAR=2018
 CONFIG_STM32_ETHMAC=y
 CONFIG_STM32_JTAG_FULL_ENABLE=y
 CONFIG_STM32_PHYSR_100MBPS=0x0000
@ -70,6 +75,7 @@ CONFIG_STM32_PWR=y
 CONFIG_STM32_RMII_EXTCLK=y
 CONFIG_STM32_USART1=y
 CONFIG_SYSTEM_PING=y
+CONFIG_SYSTEM_PING6=y
 CONFIG_TASK_NAME_SIZE=0
 CONFIG_USART1_SERIAL_CONSOLE=y
 CONFIG_USER_ENTRYPOINT="nsh_main"
--- a/net/inet/inet_close.c
+++ b/net/inet/inet_close.c
@ -345,7 +345,7 @@ static inline int tcp_close_disconnect(FAR struct socket *psock)

 #ifdef CONFIG_NET_TCP_WRITE_BUFFERS
  /* If we have a semi-permanent write buffer callback in place, then
-   * is needs to be be nullifed.
+   * is needs to be be nullified.
   *
   * Commit f1ef2c6cdeb032eaa1833cc534a63b50c5058270:
   * "When a socket is closed, it should make sure that any pending write
@ -357,6 +357,9 @@ static inline int tcp_close_disconnect(FAR struct socket *psock)
   *  data.  However, to be able to actually send any new data, the send
   *  callback must be left.  The callback should be freed later when the
   *  socket is actually destroyed."
+   *
+   * REVISIT:  Where and how exactly is s_sndcb ever freed?  Is there a
+   * memory leak here?
   */

  psock->s_sndcb = NULL;
@ -548,7 +551,18 @@ int inet_close(FAR struct socket *psock)

          if (conn->crefs <= 1)
            {
-              /* Yes... free the connection structure */
+              /* Yes... */
+
+#ifdef CONFIG_NET_UDP_WRITE_BUFFERS
+              /* Free any semi-permanent write buffer callback in place. */
+
+              if (psock->s_sndcb != NULL)
+                {
+                  udp_callback_free(conn->dev, conn, psock->s_sndcb);
+                  psock->s_sndcb = NULL;
+                }
+#endif
+              /* And free the connection structure */

              conn->crefs = 0;
              udp_free(psock->s_conn);
--- a/net/tcp/tcp_netpoll.c
+++ b/net/tcp/tcp_netpoll.c
@ -112,7 +112,11 @@ static uint16_t tcp_poll_eventhandler(FAR struct net_driver_s *dev,
          eventset |= POLLIN & info->fds->events;
        }

-      /* A poll is a sign that we are free to send data. */
+      /* A poll is a sign that we are free to send data.
+       * REVISIT: This is bogus:  If CONFIG_TCP_WRITE_BUFFERS=y then
+       * we never have to wait to send; otherwise, we always have to
+       * wait to send.  Receiving a poll is irrelevant.
+       */

      if ((flags & TCP_POLL) != 0)
        {
--- a/net/udp/udp_netpoll.c
+++ b/net/udp/udp_netpoll.c
@ -111,7 +111,11 @@ static uint16_t udp_poll_eventhandler(FAR struct net_driver_s *dev,
          eventset |= (POLLIN & info->fds->events);
        }

-      /*  poll is a sign that we are free to send data. */
+      /* A poll is a sign that we are free to send data.
+       * REVISIT: This is bogus:  If CONFIG_UDP_WRITE_BUFFERS=y then
+       * we never have to wait to send; otherwise, we always have to
+       * wait to send.  Receiving a poll is irrelevant.
+       */

      if ((flags & UDP_POLL) != 0)
        {