sockets: change closing: netconn is freed when socket is closed, not before

This is necessary to implement fullduplex sockets that are closed asynchronously:
the netconn in the socket must not be freed before all threads have given up
using it.

We now call the first part of 'netconn_delete()' (moved to 'netconn_prepare_delete()')
from lwip_close() and only actually end up calling 'netconn_free()' from
'free_socket()', which might be called later if LWIP_NETCONN_FULLDUPLEX is enabled.

Signed-off-by: goldsimon <goldsimon@gmx.de>

src/api/api_lib.c

@@ -91,6 +91,14 @@
 #define API_MSG_VAR_FREE_ACCEPT(msg)
 #endif /* TCP_LISTEN_BACKLOG */
 
+#if LWIP_NETCONN_FULLDUPLEX
+#define NETCONN_RECVMBOX_WAITABLE(conn) (sys_mbox_valid(&(conn)->recvmbox) && (((conn)->flags & NETCONN_FLAG_MBOXINVALID) == 0))
+#define NETCONN_ACCEPTMBOX_WAITABLE(conn) (sys_mbox_valid(&(conn)->acceptmbox) && (((conn)->flags & (NETCONN_FLAG_MBOXCLOSED|NETCONN_FLAG_MBOXINVALID)) == 0))
+#else
+#define NETCONN_RECVMBOX_WAITABLE(conn)   sys_mbox_valid(&(conn)->recvmbox)
+#define NETCONN_ACCEPTMBOX_WAITABLE(conn) (sys_mbox_valid(&(conn)->acceptmbox) && (((conn)->flags & NETCONN_FLAG_MBOXCLOSED) == 0))
+#endif
+
 static err_t netconn_close_shutdown(struct netconn *conn, u8_t how);
 
 /**
@@ -169,7 +177,7 @@ netconn_new_with_proto_and_callback(enum netconn_type t, u8_t proto, netconn_cal
 
 /**
  * @ingroup netconn_common
- * Close a netconn 'connection' and free its resources.
+ * Close a netconn 'connection' and free all its resources but not the netconn itself.
  * UDP and RAW connection are completely closed, TCP pcbs might still be in a waitstate
  * after this returns.
  *
@@ -177,7 +185,7 @@ netconn_new_with_proto_and_callback(enum netconn_type t, u8_t proto, netconn_cal
  * @return ERR_OK if the connection was deleted
  */
 err_t
-netconn_delete(struct netconn *conn)
+netconn_prepare_delete(struct netconn *conn)
 {
   err_t err;
   API_MSG_VAR_DECLARE(msg);
@@ -205,12 +213,43 @@ netconn_delete(struct netconn *conn)
   if (err != ERR_OK) {
     return err;
   }
-
-  netconn_free(conn);
-
   return ERR_OK;
 }
 
+/**
+ * @ingroup netconn_common
+ * Close a netconn 'connection' and free its resources.
+ * UDP and RAW connection are completely closed, TCP pcbs might still be in a waitstate
+ * after this returns.
+ *
+ * @param conn the netconn to delete
+ * @return ERR_OK if the connection was deleted
+ */
+err_t
+netconn_delete(struct netconn *conn)
+{
+  err_t err;
+
+  /* No ASSERT here because possible to get a (conn == NULL) if we got an accept error */
+  if (conn == NULL) {
+    return ERR_OK;
+  }
+
+#if LWIP_NETCONN_FULLDUPLEX
+  if (conn->flags & NETCONN_FLAG_MBOXINVALID) {
+    /* Already called netconn_prepare_delete() before */
+    err = ERR_OK;
+  } else
+#endif /* LWIP_NETCONN_FULLDUPLEX */
+  {
+    err = netconn_prepare_delete(conn);
+  }
+  if (err == ERR_OK) {
+    netconn_free(conn);
+  }
+  return err;
+}
+
 /**
  * Get the local or remote IP address and port of a netconn.
  * For RAW netconns, this returns the protocol instead of a port!
@@ -447,14 +486,11 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn)
     /* return pending error */
     return err;
   }
-  if (conn->flags & NETCONN_FLAG_MBOXCLOSED) {
+  if (!NETCONN_ACCEPTMBOX_WAITABLE(conn)) {
     /* don't accept if closed: this might block the application task
        waiting on acceptmbox forever! */
     return ERR_CLSD;
   }
-  if (!sys_mbox_valid(&conn->acceptmbox)) {
-    return ERR_CLSD;
-  }
 
   API_MSG_VAR_ALLOC_ACCEPT(msg);
 
@@ -531,7 +567,7 @@ netconn_recv_data(struct netconn *conn, void **new_buf, u8_t apiflags)
   *new_buf = NULL;
   LWIP_ERROR("netconn_recv: invalid conn",    (conn != NULL),    return ERR_ARG;);
 
-  if (!sys_mbox_valid(&conn->recvmbox)) {
+  if (!NETCONN_RECVMBOX_WAITABLE(conn)) {
     err_t err = netconn_err(conn);
     if (err != ERR_OK) {
       /* return pending error */
@@ -641,7 +677,7 @@ netconn_recv_data_tcp(struct netconn *conn, struct pbuf **new_buf, u8_t apiflags
   msg = NULL;
 #endif
 
-  if (!sys_mbox_valid(&conn->recvmbox)) {
+  if (!NETCONN_RECVMBOX_WAITABLE(conn)) {
     /* This only happens when calling this function more than once *after* receiving FIN */
     return ERR_CONN;
   }

src/api/api_msg.c

@@ -65,6 +65,12 @@
   netconn_clear_flags(conn, NETCONN_FLAG_IN_NONBLOCKING_CONNECT); }} while(0)
 #define IN_NONBLOCKING_CONNECT(conn) netconn_is_flag_set(conn, NETCONN_FLAG_IN_NONBLOCKING_CONNECT)
 
+#if LWIP_NETCONN_FULLDUPLEX
+#define NETCONN_MBOX_VALID(conn, mbox) (sys_mbox_valid(mbox) && ((conn->flags & NETCONN_FLAG_MBOXINVALID) == 0))
+#else
+#define NETCONN_MBOX_VALID(conn, mbox) sys_mbox_valid(mbox)
+#endif
+
 /* forward declarations */
 #if LWIP_TCP
 #if LWIP_TCPIP_CORE_LOCKING
@@ -78,6 +84,8 @@ static err_t lwip_netconn_do_writemore(struct netconn *conn  WRITE_DELAYED_PARAM
 static err_t lwip_netconn_do_close_internal(struct netconn *conn  WRITE_DELAYED_PARAM);
 #endif
 
+static void netconn_drain(struct netconn *conn);
+
 #if LWIP_TCPIP_CORE_LOCKING
 #define TCPIP_APIMSG_ACK(m)
 #else /* LWIP_TCPIP_CORE_LOCKING */
@@ -145,7 +153,7 @@ recv_raw(void *arg, struct raw_pcb *pcb, struct pbuf *p,
   LWIP_UNUSED_ARG(addr);
   conn = (struct netconn *)arg;
 
-  if ((conn != NULL) && sys_mbox_valid(&conn->recvmbox)) {
+  if ((conn != NULL) && NETCONN_MBOX_VALID(conn, &conn->recvmbox)) {
 #if LWIP_SO_RCVBUF
     int recv_avail;
     SYS_ARCH_GET(conn->recv_avail, recv_avail);
@@ -218,10 +226,10 @@ recv_udp(void *arg, struct udp_pcb *pcb, struct pbuf *p,
 
 #if LWIP_SO_RCVBUF
   SYS_ARCH_GET(conn->recv_avail, recv_avail);
-  if (!sys_mbox_valid(&conn->recvmbox) ||
+  if (!NETCONN_MBOX_VALID(conn, &conn->recvmbox) ||
       ((recv_avail + (int)(p->tot_len)) > conn->recv_bufsize)) {
 #else  /* LWIP_SO_RCVBUF */
-  if (!sys_mbox_valid(&conn->recvmbox)) {
+  if (!NETCONN_MBOX_VALID(conn, &conn->recvmbox)) {
 #endif /* LWIP_SO_RCVBUF */
     pbuf_free(p);
     return;
@@ -286,7 +294,7 @@ recv_tcp(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err)
   }
   LWIP_ASSERT("recv_tcp: recv for wrong pcb!", conn->pcb.tcp == pcb);
 
-  if (!sys_mbox_valid(&conn->recvmbox)) {
+  if (!NETCONN_MBOX_VALID(conn, &conn->recvmbox)) {
     /* recvmbox already deleted */
     if (p != NULL) {
       tcp_recved(pcb, p->tot_len);
@@ -441,12 +449,12 @@ err_tcp(void *arg, err_t err)
 
   mbox_msg = lwip_netconn_err_to_msg(err);
   /* pass error message to recvmbox to wake up pending recv */
-  if (sys_mbox_valid(&conn->recvmbox)) {
+  if (NETCONN_MBOX_VALID(conn, &conn->recvmbox)) {
     /* use trypost to prevent deadlock */
     sys_mbox_trypost(&conn->recvmbox, mbox_msg);
   }
   /* pass error message to acceptmbox to wake up pending accept */
-  if (sys_mbox_valid(&conn->acceptmbox)) {
+  if (NETCONN_MBOX_VALID(conn, &conn->acceptmbox)) {
     /* use trypost to preven deadlock */
     sys_mbox_trypost(&conn->acceptmbox, mbox_msg);
   }
@@ -516,7 +524,7 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err)
   if (conn == NULL) {
     return ERR_VAL;
   }
-  if (!sys_mbox_valid(&conn->acceptmbox)) {
+  if (!NETCONN_MBOX_VALID(conn, &conn->acceptmbox)) {
     LWIP_DEBUGF(API_MSG_DEBUG, ("accept_function: acceptmbox already deleted\n"));
     return ERR_VAL;
   }
@@ -772,6 +780,12 @@ void
 netconn_free(struct netconn *conn)
 {
   LWIP_ASSERT("PCB must be deallocated outside this function", conn->pcb.tcp == NULL);
+
+#if LWIP_NETCONN_FULLDUPLEX
+  /* in fullduplex, netconn is drained here */
+  netconn_drain(conn);
+#endif /* LWIP_NETCONN_FULLDUPLEX */
+
   LWIP_ASSERT("recvmbox must be deallocated before calling this function",
               !sys_mbox_valid(&conn->recvmbox));
 #if LWIP_TCP
@@ -800,7 +814,11 @@ netconn_drain(struct netconn *conn)
 {
   void *mem;
 
-  /* This runs in tcpip_thread, so we don't need to lock against rx packets */
+  /* This runs when mbox and netconn are marked as closed,
+     so we don't need to lock against rx packets */
+#if LWIP_NETCONN_FULLDUPLEX
+  LWIP_ASSERT("netconn marked closed", conn->flags & NETCONN_FLAG_MBOXINVALID);
+#endif /* LWIP_NETCONN_FULLDUPLEX */
 
   /* Delete and drain the recvmbox. */
   if (sys_mbox_valid(&conn->recvmbox)) {
@@ -845,6 +863,15 @@ netconn_drain(struct netconn *conn)
 #endif /* LWIP_TCP */
 }
 
+#if LWIP_NETCONN_FULLDUPLEX
+static void
+netconn_mark_mbox_invalid(struct netconn *conn)
+{
+  /* Prevent new calls/threads from reading from the mbox */
+  conn->flags |= NETCONN_FLAG_MBOXINVALID;
+}
+#endif /* LWIP_NETCONN_FULLDUPLEX */
+
 #if LWIP_TCP
 /**
  * Internal helper function to close a TCP netconn: since this sometimes
@@ -1083,8 +1110,12 @@ lwip_netconn_do_delconn(void *m)
     LWIP_ASSERT("blocking connect in progress",
                 (state != NETCONN_CONNECT) || IN_NONBLOCKING_CONNECT(msg->conn));
     msg->err = ERR_OK;
-    /* Drain and delete mboxes */
+#if LWIP_NETCONN_FULLDUPLEX
+    /* Mark mboxes invalid */
+    netconn_mark_mbox_invalid(msg->conn);
+#else /* LWIP_NETCONN_FULLDUPLEX */
     netconn_drain(msg->conn);
+#endif /* LWIP_NETCONN_FULLDUPLEX */
 
     if (msg->conn->pcb.tcp != NULL) {
 
@@ -1904,8 +1935,12 @@ lwip_netconn_do_close(void *m)
     } else {
 #endif /* LWIP_NETCONN_FULLDUPLEX */
       if (msg->msg.sd.shut & NETCONN_SHUT_RD) {
-        /* Drain and delete mboxes */
+#if LWIP_NETCONN_FULLDUPLEX
+        /* Mark mboxes invalid */
+        netconn_mark_mbox_invalid(msg->conn);
+#else /* LWIP_NETCONN_FULLDUPLEX */
         netconn_drain(msg->conn);
+#endif /* LWIP_NETCONN_FULLDUPLEX */
       }
       LWIP_ASSERT("already writing or closing", msg->conn->current_msg == NULL);
       msg->conn->state = NETCONN_CLOSE;

src/api/sockets.c

@@ -297,8 +297,9 @@ static void lwip_setsockopt_callback(void *arg);
 #endif
 static int lwip_getsockopt_impl(int s, int level, int optname, void *optval, socklen_t *optlen);
 static int lwip_setsockopt_impl(int s, int level, int optname, const void *optval, socklen_t optlen);
-static int free_socket_locked(struct lwip_sock *sock, int is_tcp, union lwip_sock_lastdata *lastdata);
+static int free_socket_locked(struct lwip_sock *sock, int is_tcp, struct netconn **conn,
-static void free_socket_free_elements(int is_tcp, union lwip_sock_lastdata *lastdata);
+                              union lwip_sock_lastdata *lastdata);
+static void free_socket_free_elements(int is_tcp, struct netconn *conn, union lwip_sock_lastdata *lastdata);
 
 #if LWIP_IPV4 && LWIP_IPV6
 static void
@@ -378,6 +379,7 @@ done_socket(struct lwip_sock *sock)
   SYS_ARCH_DECL_PROTECT(lev);
   int freed = 0;
   int is_tcp = 0;
+  struct netconn *conn = NULL;
   union lwip_sock_lastdata lastdata;
   LWIP_ASSERT("sock != NULL", sock != NULL);
 
@@ -388,13 +390,13 @@ done_socket(struct lwip_sock *sock)
       /* free the socket */
       sock->fd_used = 1;
       is_tcp = sock->fd_free_pending & LWIP_SOCK_FD_FREE_TCP;
-      freed = free_socket_locked(sock, is_tcp, &lastdata);
+      freed = free_socket_locked(sock, is_tcp, &conn, &lastdata);
     }
   }
   SYS_ARCH_UNPROTECT(lev);
 
   if (freed) {
-    free_socket_free_elements(is_tcp, &lastdata);
+    free_socket_free_elements(is_tcp, conn, &lastdata);
   }
 }
 
@@ -539,10 +541,12 @@ alloc_socket(struct netconn *newconn, int accepted)
  *
  * @param sock the socket to free
  * @param is_tcp != 0 for TCP sockets, used to free lastdata
+ * @param conn the socekt's netconn is stored here, must be freed externally
  * @param lastdata lastdata is stored here, must be freed externally
  */
 static int
-free_socket_locked(struct lwip_sock *sock, int is_tcp, union lwip_sock_lastdata *lastdata)
+free_socket_locked(struct lwip_sock *sock, int is_tcp, struct netconn **conn,
+                   union lwip_sock_lastdata *lastdata)
 {
 #if LWIP_NETCONN_FULLDUPLEX
   LWIP_ASSERT("sock->fd_used > 0", sock->fd_used > 0);
@@ -551,10 +555,13 @@ free_socket_locked(struct lwip_sock *sock, int is_tcp, union lwip_sock_lastdata
     sock->fd_free_pending = LWIP_SOCK_FD_FREE_FREE | (is_tcp ? LWIP_SOCK_FD_FREE_TCP : 0);
     return 0;
   }
-#endif
+#else /* LWIP_NETCONN_FULLDUPLEX */
+  LWIP_UNUSED_ARG(is_tcp);
+#endif /* LWIP_NETCONN_FULLDUPLEX */
 
   *lastdata = sock->lastdata;
   sock->lastdata.pbuf = NULL;
+  *conn = sock->conn;
   sock->conn = NULL;
   return 1;
 }
@@ -562,7 +569,7 @@ free_socket_locked(struct lwip_sock *sock, int is_tcp, union lwip_sock_lastdata
 /** Free a socket's leftover members.
  */
 static void
-free_socket_free_elements(int is_tcp, union lwip_sock_lastdata *lastdata)
+free_socket_free_elements(int is_tcp, struct netconn *conn, union lwip_sock_lastdata *lastdata)
 {
   if (lastdata->pbuf != NULL) {
     if (is_tcp) {
@@ -571,6 +578,10 @@ free_socket_free_elements(int is_tcp, union lwip_sock_lastdata *lastdata)
       netbuf_delete(lastdata->netbuf);
     }
   }
+  if (conn != NULL) {
+    /* netconn_prepare_delete() has already been called, here we only free the conn */
+    netconn_delete(conn);
+  }
 }
 
 /** Free a socket. The socket's netconn must have been
@@ -583,18 +594,19 @@ static void
 free_socket(struct lwip_sock *sock, int is_tcp)
 {
   int freed;
+  struct netconn *conn;
   union lwip_sock_lastdata lastdata;
   SYS_ARCH_DECL_PROTECT(lev);
 
   /* Protect socket array */
   SYS_ARCH_PROTECT(lev);
 
-  freed = free_socket_locked(sock, is_tcp, &lastdata);
+  freed = free_socket_locked(sock, is_tcp, &conn, &lastdata);
   SYS_ARCH_UNPROTECT(lev);
   /* don't use 'sock' after this line, as another task might have allocated it */
 
   if (freed) {
-    free_socket_free_elements(is_tcp, &lastdata);
+    free_socket_free_elements(is_tcp, conn, &lastdata);
   }
 }
 
@@ -785,7 +797,7 @@ lwip_close(int s)
   lwip_socket_drop_registered_mld6_memberships(s);
 #endif /* LWIP_IPV6_MLD */
 
-  err = netconn_delete(sock->conn);
+  err = netconn_prepare_delete(sock->conn);
   if (err != ERR_OK) {
     sock_set_errno(sock, err_to_errno(err));
     done_socket(sock);

src/include/lwip/api.h

@@ -73,6 +73,10 @@ extern "C" {
 #define NETCONN_FLAG_NON_BLOCKING             0x02
 /** Was the last connect action a non-blocking one? */
 #define NETCONN_FLAG_IN_NONBLOCKING_CONNECT   0x04
+#if LWIP_NETCONN_FULLDUPLEX
+  /** The mbox of this netconn is being deallocated, don't use it anymore */
+#define NETCONN_FLAG_MBOXINVALID              0x08
+#endif /* LWIP_NETCONN_FULLDUPLEX */
 /** If a nonblocking write has been rejected before, poll_tcp needs to
     check if the netconn is writable again */
 #define NETCONN_FLAG_CHECK_WRITESPACE         0x10
@@ -302,6 +306,7 @@ struct netvector {
 #define netconn_new_with_callback(t, c) netconn_new_with_proto_and_callback(t, 0, c)
 struct netconn *netconn_new_with_proto_and_callback(enum netconn_type t, u8_t proto,
                                              netconn_callback callback);
+err_t   netconn_prepare_delete(struct netconn *conn);
 err_t   netconn_delete(struct netconn *conn);
 /** Get the type of a netconn (as enum netconn_type). */
 #define netconn_type(conn) (conn->type)