(properly) check whether ClientContext::connect()'s delay is interrupted by disconnected WiFi

libraries/ESP8266WiFi/src/WiFiClient.cpp

@@ -66,7 +66,7 @@ WiFiClient::~WiFiClient()
 {
     WiFiClient::_remove(this);
     if (_client)
-        _client->unref();
+        unuse_result(_client->unref());
 }
 
 WiFiClient::WiFiClient(const WiFiClient& other)
@@ -82,7 +82,7 @@ WiFiClient::WiFiClient(const WiFiClient& other)
 WiFiClient& WiFiClient::operator=(const WiFiClient& other)
 {
    if (_client)
-        _client->unref();
+        unuse_result(_client->unref());
     _client = other._client;
     _timeout = other._timeout;
     _localPort = other._localPort;
@@ -139,7 +139,9 @@ int WiFiClient::connect(IPAddress ip, uint16_t port)
     _client->setTimeout(_timeout);
     int res = _client->connect(&addr, port);
     if (res == 0) {
-        _client->unref();
+        // _client may be NULL because of asynchronous ::stop,
+        // see ClientContext::unref() which checks this
+        unuse_result(_client->unref());
         _client = nullptr;
         return 0;
     }
@@ -272,7 +274,7 @@ void WiFiClient::stop()
     if (!_client)
         return;
 
-    _client->unref();
+    unuse_result(_client->unref());
     _client = 0;
 }
 

libraries/ESP8266WiFi/src/include/ClientContext.h

@@ -26,11 +26,12 @@ class WiFiClient;
 
 typedef void (*discard_cb_t)(void*, ClientContext*);
 
-extern "C" void esp_yield();
 extern "C" void esp_schedule();
 
 #include "DataSource.h"
 
+#define unuse_result(x) ({ __typeof__(x) z = x; (void)sizeof(z); })
+
 class ClientContext
 {
 public:
@@ -105,8 +106,15 @@ class ClientContext
         DEBUGV(":ref %d\r\n", _refcnt);
     }
 
-    void unref()
+    // unref() return true if 'this->' is deleted
+    bool unref() __attribute__((warn_unused_result))
     {
+        // 'if(this != 0)' deserves an explanation:
+        //     It is very valid because it happens and is true after
+        //     *asynchronous* WiFiClient::stop() which calls ::unref then
+        //     nullify me=WiFiClient::_client *during* for example
+        //     WiFiClient::connect(). see _delay_destroyed_this().
+        //     (would WiFiClient:: check _client instead?)
         if(this != 0) {
             DEBUGV(":ur %d\r\n", _refcnt);
             if(--_refcnt == 0) {
@@ -117,8 +125,10 @@ class ClientContext
                 }
                 DEBUGV(":del\r\n");
                 delete this;
+                return true;
             }
         }
+        return false;
     }
 
     int connect(ip_addr_t* addr, uint16_t port)
@@ -130,12 +140,9 @@ class ClientContext
         _connect_pending = 1;
         _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");
+        if (_delay_destroyed_this(_timeout_ms))
+            // we were deleted during the delay, 'this->' is now invalid
             return 0;
-        }
         _connect_pending = 0;
         if (state() != ESTABLISHED) {
             abort();
@@ -310,7 +317,8 @@ class ClientContext
 
         while (state() == ESTABLISHED && tcp_sndbuf(_pcb) != TCP_SND_BUF && --tries) {
             _write_some();
-            delay(1); // esp_ schedule+yield
+            if (_delay_destroyed_this(1))
+                return;
         }
     }
 
@@ -383,18 +391,33 @@ class ClientContext
 
 protected:
 
+    // _delay_destroyed_this() fixes #4078
+    // where delay() is interrupted by disconnected AP WiFi event
+    // during which WiFiClient stops all => 'this->' becomes invalid
+    bool _delay_destroyed_this (unsigned long ms) __attribute__((warn_unused_result))
+    {
+        ref();
+        delay(ms);
+        bool destroyed = unref();
+        return destroyed;
+    }
+
     bool _is_timeout()
     {
         return millis() - _op_start_time > _timeout_ms;
     }
 
-    void _notify_error()
+    bool _notify_error_destroyed_this() __attribute__((warn_unused_result))
     {
         if (_connect_pending || _send_waiting) {
-            esp_schedule();
+            return _delay_destroyed_this(0); // was: esp_schedule()
         }
+        return false;
     }
 
+    // warning:
+    // please ensure all _write_from_source callers
+    // straightly return from the instance
     size_t _write_from_source(DataSource* ds)
     {
         assert(_datasource == nullptr);
@@ -417,7 +440,9 @@ class ClientContext
             }
 
             ++_send_waiting;
-            esp_yield();
+            if (_delay_destroyed_this(0)) // calls esp_yield()
+                // 'this->' is no more, hence the warning above
+                return 0;
         } while(true);
         _send_waiting = 0;
         return _written;
@@ -512,9 +537,9 @@ class ClientContext
         (void) err;
         if(pb == 0) { // connection closed
             DEBUGV(":rcl\r\n");
-            _notify_error();
-            abort();
-            return ERR_ABRT;
+            if (!_notify_error_destroyed_this())
+                abort();
+            return ERR_ABRT; // return from instance
         }
 
         if(_rx_buf) {
@@ -537,7 +562,8 @@ class ClientContext
         tcp_recv(_pcb, NULL);
         tcp_err(_pcb, NULL);
         _pcb = NULL;
-        _notify_error();
+
+        unuse_result(_notify_error_destroyed_this()); // return from instance
     }
 
     err_t _connected(struct tcp_pcb *pcb, err_t err)