WiFiClient: don’t destroy ClientContext on ::stop

Reported in esp8266#4078. WiFiClient::stopAll, called from a WiFi disconnected event handler, could be called while WiFiClient::connect was in progress. This issue was initially fixed in esp8266#4194, by testing `this` pointer for being non-null in ClientContext::connect. This change delegates deletion of ClientContext to WiFiClient destructor. WiFiClient::stop only calls ClientContext::stop, which closes/aborts the connection.
BenMens · Feb 20, 2018 · ee5a1e2 · ee5a1e2
1 parent 28d8a41
commit ee5a1e2
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 16 deletions.
diff --git a/libraries/ESP8266WiFi/src/WiFiClient.cpp b/libraries/ESP8266WiFi/src/WiFiClient.cpp
@@ -272,8 +272,7 @@ void WiFiClient::stop()
     if (!_client)
         return;
 
-    _client->unref();
-    _client = 0;
+    _client->close();
 }
 
 uint8_t WiFiClient::connected()

diff --git a/libraries/ESP8266WiFi/src/include/ClientContext.h b/libraries/ESP8266WiFi/src/include/ClientContext.h
@@ -107,17 +107,15 @@ class ClientContext
 
     void unref()
     {
-        if(this != 0) {
-            DEBUGV(":ur %d\r\n", _refcnt);
-            if(--_refcnt == 0) {
-                discard_received();
-                close();
-                if(_discard_cb) {
-                    _discard_cb(_discard_cb_arg, this);
-                }
-                DEBUGV(":del\r\n");
-                delete this;
+        DEBUGV(":ur %d\r\n", _refcnt);
+        if(--_refcnt == 0) {
+            discard_received();
+            close();
+            if(_discard_cb) {
+                _discard_cb(_discard_cb_arg, this);
             }
+            DEBUGV(":del\r\n");
+            delete this;
         }
     }
 
@@ -131,13 +129,13 @@ class ClientContext
         _op_start_time = millis();
         // This delay will be interrupted by esp_schedule in the connect callback
         delay(_timeout_ms);
-        // WiFi may have vanished during the delay (#4078)
-        if (!this || !_pcb) {
-            DEBUGV(":vnsh\r\n");
+        _connect_pending = 0;
+        if (!_pcb) {
+            DEBUGV(":cabrt\r\n");
             return 0;
         }
-        _connect_pending = 0;
         if (state() != ESTABLISHED) {
+            DEBUGV(":ctmo\r\n");
             abort();
             return 0;
         }

diff --git a/tests/device/test_WiFiClient/test_WiFiClient.ino b/tests/device/test_WiFiClient/test_WiFiClient.ino
@@ -0,0 +1,57 @@
+#include <Arduino.h>
+#include <BSTest.h>
+#include <test_config.h>
+#include <ESP8266WiFi.h>
+#include <Ticker.h>
+
+extern "C" {
+#include "user_interface.h"
+}
+
+BS_ENV_DECLARE();
+
+void setup()
+{
+    Serial.begin(115200);
+    Serial.setDebugOutput(true);
+    WiFi.persistent(false);
+    WiFi.mode(WIFI_STA);
+    WiFi.begin(STA_SSID, STA_PASS);
+    while (WiFi.status() != WL_CONNECTED) {
+        delay(500);
+    }
+    BS_RUN(Serial);
+}
+
+static void stopAll()
+{
+    WiFiClient::stopAll();
+}
+
+static void disconnectWiFI()
+{
+    wifi_station_disconnect();
+}
+
+/* Some IP address that we can try connecting to, and expect a timeout */
+#define UNREACHABLE_IP "192.168.255.255"
+
+TEST_CASE("WiFiClient::stopAll during WiFiClient::connect", "[wificlient]")
+{
+    WiFiClient client;
+    Ticker t;
+    t.once_ms(500, &stopAll);
+    REQUIRE(client.connect(UNREACHABLE_IP, 1024) == 0);
+}
+
+TEST_CASE("WiFi disconnect during WiFiClient::connect", "[wificlient]")
+{
+    WiFiClient client;
+    Ticker t;
+    t.once_ms(500, &disconnectWiFI);
+    REQUIRE(client.connect(UNREACHABLE_IP, 1024) == 0);
+}
+
+void loop()
+{
+}