issue: 4607536 Fix a race condition

Fixing a race condition, between Internal-Thread
and a thread performing the tcp_listen_input, causing
list corruption and worker crash with double free warning.
And, removing some redundant params.

Signed-off-by: Bashar Abdelgafer <[email protected]>

Author: BasharRadya

src/vma/lwip/tcp.c

@@ -1236,6 +1236,18 @@ tcp_syn_handled(struct tcp_pcb_listen *pcb, tcp_syn_handled_fn syn_handled)
   pcb->syn_handled_cb = syn_handled;
 }
 
+/**
+ * Used for specifying the function that should be when a accepted pcb is ready.
+ *
+ * @param pcb          Listen pcb
+ * @param accepted_pcb Callback function to call when the accepted pcb is ready.
+ */
+void
+tcp_accepted_pcb(struct tcp_pcb_listen *pcb, tcp_accepted_pcb_fn accepted_pcb)
+{
+  pcb->accepted_pcb = accepted_pcb;
+}
+
 /**
  * Used for specifying the function that should be called to clone pcb
  *

src/vma/lwip/tcp.h

@@ -129,6 +129,11 @@ typedef err_t (*tcp_syn_handled_fn)(void *arg, struct tcp_pcb *newpcb, err_t err
  */
 typedef err_t (*tcp_clone_conn_fn)(void *arg, struct tcp_pcb **newpcb, err_t err);
 
+/** Function prototype for tcp new-pcb callback functions.
+ * Called when a new pcb is ready as part of tcp_listen_input handling.
+ * @param newpcb The new connection pcb
+ */
+typedef void (*tcp_accepted_pcb_fn)(struct tcp_pcb *accepted_pcb);
 
 /** Function prototype for tcp receive callback functions. Called when data has
  * been received.
@@ -416,7 +421,7 @@ struct tcp_pcb {
 #ifdef VMA_NO_TCP_PCB_LISTEN_STRUCT
   tcp_syn_handled_fn syn_handled_cb;
   tcp_clone_conn_fn clone_conn;
-
+  tcp_accepted_pcb_fn accepted_pcb;
 #endif /* VMA_NO_TCP_PCB_LISTEN_STRUCT */
 
   /* Delayed ACK control: number of quick acks */
@@ -453,6 +458,7 @@ struct tcp_pcb_listen {
   TCP_PCB_COMMON(struct tcp_pcb_listen);
   tcp_syn_handled_fn syn_handled_cb;
   tcp_clone_conn_fn clone_conn;
+  tcp_accepted_pcb_fn accepted_pcb;
 };
 #endif /* VMA_NO_TCP_PCB_LISTEN_STRUCT */
 
@@ -488,6 +494,7 @@ void             tcp_ip_output          (struct tcp_pcb *pcb, ip_output_fn ip_ou
 void             tcp_accept  		(struct tcp_pcb *pcb, tcp_accept_fn accept);
 void             tcp_syn_handled	(struct tcp_pcb_listen *pcb, tcp_syn_handled_fn syn_handled);
 void             tcp_clone_conn		(struct tcp_pcb_listen *pcb, tcp_clone_conn_fn clone_conn);
+void             tcp_accepted_pcb		(struct tcp_pcb_listen *pcb, tcp_accepted_pcb_fn accepted_pcb);
 void             tcp_recv    		(struct tcp_pcb *pcb, tcp_recv_fn recv);
 void             tcp_sent    		(struct tcp_pcb *pcb, tcp_sent_fn sent);
 void             tcp_poll    		(struct tcp_pcb *pcb, tcp_poll_fn poll, u8_t interval);

src/vma/lwip/tcp_impl.h

@@ -218,6 +218,14 @@ PACK_STRUCT_END
     else (ret) = ERR_ARG;                                           \
   } while (0)
 
+
+#define TCP_EVENT_ACCEPTED_PCB(pcb, newpcb)                                                        \
+do {                                                                                           \
+    if ((pcb)->accepted_pcb != NULL)                                                           \
+        (pcb)->accepted_pcb((newpcb));                                                         \
+} while (0)
+
+
 #define TCP_EVENT_SENT(pcb,space,ret)                          \
   do {                                                         \
     if((pcb)->sent != NULL)                                    \

src/vma/lwip/tcp_in.c

@@ -422,11 +422,13 @@ tcp_listen_input(struct tcp_pcb_listen *pcb, tcp_in_data* in_data)
 
     /* Send a SYN|ACK together with the MSS option. */
     rc = tcp_enqueue_flags(npcb, TCP_SYN | TCP_ACK);
-    if (rc != ERR_OK) {
+    if (rc == ERR_OK) {
+      rc = tcp_output(npcb);
+    }else{
       tcp_abandon(npcb, 0);
-      return rc;
     }
-    return tcp_output(npcb);
+    TCP_EVENT_ACCEPTED_PCB(pcb, npcb);
+    return rc;
   }
   return ERR_OK;
 }

src/vma/sock/sockinfo_tcp.cpp

@@ -491,6 +491,7 @@ bool sockinfo_tcp::prepare_to_close(bool process_shutdown /* = false */)
 			tcp_accept(&m_pcb, 0);
 			tcp_syn_handled((struct tcp_pcb_listen*)(&m_pcb), 0);
 			tcp_clone_conn((struct tcp_pcb_listen*)(&m_pcb), 0);
+			tcp_accepted_pcb((struct tcp_pcb_listen*)(&m_pcb), 0);
 			prepare_listen_to_close(); //close pending to accept sockets
 		} else {
 			tcp_recv(&m_pcb, sockinfo_tcp::rx_drop_lwip_cb);
@@ -2452,7 +2453,7 @@ int sockinfo_tcp::listen(int backlog)
 	tcp_accept(&m_pcb, sockinfo_tcp::accept_lwip_cb);
 	tcp_syn_handled((struct tcp_pcb_listen*)(&m_pcb), sockinfo_tcp::syn_received_lwip_cb);
 	tcp_clone_conn((struct tcp_pcb_listen*)(&m_pcb), sockinfo_tcp::clone_conn_cb);
-
+	tcp_accepted_pcb((struct tcp_pcb_listen*)(&m_pcb), sockinfo_tcp::accepted_pcb_cb);
 	bool success = attach_as_uc_receiver(ROLE_TCP_SERVER);
 
 	if (!success) {
@@ -2699,6 +2700,11 @@ sockinfo_tcp *sockinfo_tcp::accept_clone()
                 return 0;
         }
 
+		// This method is called from a flow which assumes that the socket is locked
+		// (tcp_listen_input, L3_level_tcp_input).
+		// Since we created a new socket and we are about to add it to the timers,
+		// we need to make sure it is also locked for further processing.
+		si->lock_tcp_con();
         si->m_parent = this;
 
         si->m_sock_state = TCP_SOCK_BOUND;
@@ -2943,6 +2949,15 @@ err_t sockinfo_tcp::clone_conn_cb(void *arg, struct tcp_pcb **newpcb, err_t err)
 	return ret_val;
 }
 
+void sockinfo_tcp::accepted_pcb_cb(struct tcp_pcb *accepted_pcb)
+{
+    // A new pcb is always locked. When this callback is called the new pcb is ready
+    // and all related processing is done. Now it must be unlocked.
+    sockinfo_tcp *accepted_sock = reinterpret_cast<sockinfo_tcp *>(accepted_pcb->my_container);
+    ASSERT_LOCKED(accepted_sock->m_tcp_con_lock);
+    accepted_sock->unlock_tcp_con();
+}
+
 err_t sockinfo_tcp::syn_received_lwip_cb(void *arg, struct tcp_pcb *newpcb, err_t err)
 {
 	sockinfo_tcp *listen_sock = (sockinfo_tcp *)((arg));
@@ -2978,6 +2993,10 @@ err_t sockinfo_tcp::syn_received_lwip_cb(void *arg, struct tcp_pcb *newpcb, err_
 	if (!is_new_offloaded) {
 		new_sock->setPassthrough();
 		set_tcp_state(&new_sock->m_pcb, CLOSED);
+		// This method is called from a flow (tcp_listen_input, L3_level_tcp_input) which priorly
+		// called clone_conn_cb which creates a locked new socket. Before we call to close() we need
+		// to unlock the socket, so close() can perform as a regular close() call.
+		new_sock->unlock_tcp_con();
 		close(new_sock->get_fd());
 		listen_sock->m_tcp_con_lock.lock();
 		return ERR_ABRT;
@@ -3019,6 +3038,11 @@ err_t sockinfo_tcp::syn_received_drop_lwip_cb(void *arg, struct tcp_pcb *newpcb,
 		tcp_arg(&(new_sock->m_pcb), new_sock);
 		new_sock->abort_connection();
 	}
+	// This method is called from a flow (tcp_listen_input, L3_level_tcp_input) which priorly called
+	// clone_conn_cb which creates a locked new socket. Before we call to close() we need to unlock
+	// the socket, so close() can perform as a regular close() call.
+	new_sock->unlock_tcp_con();
+
 	close(new_sock->get_fd());
 
 	listen_sock->m_tcp_con_lock.lock();

src/vma/sock/sockinfo_tcp.h

@@ -308,6 +308,11 @@ class sockinfo_tcp : public sockinfo, public timer_handler
 
 	static err_t clone_conn_cb(void *arg, struct tcp_pcb **newpcb, err_t err);
 
+
+	// Called by L3_level_tcp_input to unlock a new pcb/socket.
+	// @param newpcb The new pcb. Can be nullptr.
+	static void accepted_pcb_cb(struct tcp_pcb *newpcb);
+
 	int accept_helper(struct sockaddr *__addr, socklen_t *__addrlen, int __flags = 0);
 
 	// clone socket in accept call