From 0c4d364cee23a710b588cb61e2e65b119b0a5083 Mon Sep 17 00:00:00 2001
From: Deomid Ryabkov <rojer@rojer.me>
Date: Tue, 14 Jun 2022 21:27:24 +0100
Subject: [PATCH] LWIP: Fix races between TCP errors and queued callbacks

A TCP connection can get closed while there are TCPIP thread callbacks
queued. Add checks to close and write functions to avoid acting on
tcp_pcbs that have already been disposed of.
---
 mongoose.c                                 | 29 +++++++++++++++++++---
 src/common/platforms/lwip/mg_lwip_net_if.c | 29 +++++++++++++++++++---
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/mongoose.c b/mongoose.c
index 74d75b78..a7724f84 100644
--- a/mongoose.c
+++ b/mongoose.c
@@ -15290,10 +15290,6 @@ void mg_lwip_if_connect_udp(struct mg_connection *nc) {
   mg_lwip_netif_run_on_tcpip(mg_lwip_if_connect_udp_tcpip, nc);
 }
 
-static void tcp_close_tcpip(void *arg) {
-  tcp_close((struct tcp_pcb *) arg);
-}
-
 void mg_lwip_handle_accept(struct mg_connection *nc) {
   struct mg_lwip_conn_state *cs = (struct mg_lwip_conn_state *) nc->sock;
   if (cs->pcb.tcp == NULL) return;
@@ -15423,6 +15419,10 @@ static void mg_lwip_tcp_write_tcpip(void *arg) {
   struct mg_connection *nc = ctx->nc;
   struct mg_lwip_conn_state *cs = (struct mg_lwip_conn_state *) nc->sock;
   struct tcp_pcb *tpcb = cs->pcb.tcp;
+  if (tpcb == NULL) {
+    /* Possible if tcp_err cb was invoked while this was queued. */
+    return;
+  }
   size_t len = MIN(tpcb->mss, MIN(ctx->len, tpcb->snd_buf));
   size_t unsent, unacked;
   if (len == 0) {
@@ -15593,6 +15593,27 @@ int mg_lwip_if_create_conn(struct mg_connection *nc) {
   return 1;
 }
 
+extern struct tcp_pcb *tcp_active_pcbs;
+
+static void tcp_close_tcpip(void *arg) {
+  /*
+   * A race with tcp error is possible:
+   *  - mg_lwip_if_destroy_conn is called, schedules this function.
+   *  - RST arrives, mg_lwip_tcp_error_cb is invoked and pcb is destroyed.
+   *  - this function runs.
+   * Since tcp_err doesn't get the pcb pointer, I don't see any other way than
+   * walking the list of active connections. It is ostensibly private but
+   * fortunately for us is not declared static.
+   */
+  struct tcp_pcb *pcb;
+  for (pcb = tcp_active_pcbs; pcb != NULL; pcb = pcb->next) {
+    if (pcb == (struct tcp_pcb *) arg) break;
+  }
+  if (pcb != NULL) {
+    tcp_close(pcb);
+  }
+}
+
 static void udp_remove_tcpip(void *arg) {
   udp_remove((struct udp_pcb *) arg);
 }
diff --git a/src/common/platforms/lwip/mg_lwip_net_if.c b/src/common/platforms/lwip/mg_lwip_net_if.c
index b9e2e067..8ccf4837 100644
--- a/src/common/platforms/lwip/mg_lwip_net_if.c
+++ b/src/common/platforms/lwip/mg_lwip_net_if.c
@@ -365,10 +365,6 @@ void mg_lwip_if_connect_udp(struct mg_connection *nc) {
   mg_lwip_netif_run_on_tcpip(mg_lwip_if_connect_udp_tcpip, nc);
 }
 
-static void tcp_close_tcpip(void *arg) {
-  tcp_close((struct tcp_pcb *) arg);
-}
-
 void mg_lwip_handle_accept(struct mg_connection *nc) {
   struct mg_lwip_conn_state *cs = (struct mg_lwip_conn_state *) nc->sock;
   if (cs->pcb.tcp == NULL) return;
@@ -498,6 +494,10 @@ static void mg_lwip_tcp_write_tcpip(void *arg) {
   struct mg_connection *nc = ctx->nc;
   struct mg_lwip_conn_state *cs = (struct mg_lwip_conn_state *) nc->sock;
   struct tcp_pcb *tpcb = cs->pcb.tcp;
+  if (tpcb == NULL) {
+    /* Possible if tcp_err cb was invoked while this was queued. */
+    return;
+  }
   size_t len = MIN(tpcb->mss, MIN(ctx->len, tpcb->snd_buf));
   size_t unsent, unacked;
   if (len == 0) {
@@ -668,6 +668,27 @@ int mg_lwip_if_create_conn(struct mg_connection *nc) {
   return 1;
 }
 
+extern struct tcp_pcb *tcp_active_pcbs;
+
+static void tcp_close_tcpip(void *arg) {
+  /*
+   * A race with tcp error is possible:
+   *  - mg_lwip_if_destroy_conn is called, schedules this function.
+   *  - RST arrives, mg_lwip_tcp_error_cb is invoked and pcb is destroyed.
+   *  - this function runs.
+   * Since tcp_err doesn't get the pcb pointer, I don't see any other way than
+   * walking the list of active connections. It is ostensibly private but
+   * fortunately for us is not declared static.
+   */
+  struct tcp_pcb *pcb;
+  for (pcb = tcp_active_pcbs; pcb != NULL; pcb = pcb->next) {
+    if (pcb == (struct tcp_pcb *) arg) break;
+  }
+  if (pcb != NULL) {
+    tcp_close(pcb);
+  }
+}
+
 static void udp_remove_tcpip(void *arg) {
   udp_remove((struct udp_pcb *) arg);
 }