From 646eab7bd8761a7ff89c627e37c0f27c49cef32f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Villac=C3=ADs=20Lasso?= <a_villacis@palosanto.com>
Date: Thu, 14 Jan 2021 15:13:17 -0500
Subject: [PATCH] Fix self-deadlock onConnect introduced by ul-gh/fix_#884
 branch

Commit 344062ae5be9dab80dd602458857f3a5ec5dfb3c intended to protect the
client list for AsyncEventSource from concurrent access by wrapping
accesses with locking. However, this introduces a self-deadlock scenario
if application code in the onConnect callback (now called with the
client list lock held) tries to broadcast a message to existing
connections in the same AsyncEventSource (for example, to alert of a
new incoming connection). The broadcast call tries to take the lock but
blocks because the lock is already taken, by the same task, before
calling the callback. Fixed by making use of the AsyncWebLockGuard class
which is tailor-made to address this scenario.
---
 src/AsyncEventSource.cpp | 19 ++++++-------------
 src/AsyncEventSource.h   |  2 +-
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/AsyncEventSource.cpp b/src/AsyncEventSource.cpp
index c2f8187..90317af 100644
--- a/src/AsyncEventSource.cpp
+++ b/src/AsyncEventSource.cpp
@@ -298,38 +298,34 @@ void AsyncEventSource::_addClient(AsyncEventSourceClient * client){
     client->write((const char *)temp, 2053);
     free(temp);
   }*/
-  _client_queue_lock.lock();
+  AsyncWebLockGuard l(_client_queue_lock);
   _clients.add(client);
   if(_connectcb)
     _connectcb(client);
-  _client_queue_lock.unlock();
 }
 
 void AsyncEventSource::_handleDisconnect(AsyncEventSourceClient * client){
-  _client_queue_lock.lock();
+  AsyncWebLockGuard l(_client_queue_lock);
   _clients.remove(client);
-  _client_queue_lock.unlock();
 }
 
 void AsyncEventSource::close(){
   // While the whole loop is not done, the linked list is locked and so the
   // iterator should remain valid even when AsyncEventSource::_handleDisconnect()
   // is called very early
-  _client_queue_lock.lock();
+  AsyncWebLockGuard l(_client_queue_lock);
   for(const auto &c: _clients){
     if(c->connected())
       c->close();
   }
-  _client_queue_lock.unlock();
 }
 
 // pmb fix
 size_t AsyncEventSource::avgPacketsWaiting() const {
   size_t aql = 0;
   uint32_t nConnectedClients = 0;
-  _client_queue_lock.lock();
+  AsyncWebLockGuard l(_client_queue_lock);
   if (_clients.isEmpty()) {
-    _client_queue_lock.unlock();
     return 0;
   }
   for(const auto &c: _clients){
@@ -338,29 +334,26 @@ size_t AsyncEventSource::avgPacketsWaiting() const {
       ++nConnectedClients;
     }
   }
-  _client_queue_lock.unlock();
   return ((aql) + (nConnectedClients/2)) / (nConnectedClients); // round up
 }
 
 void AsyncEventSource::send(
     const char *message, const char *event, uint32_t id, uint32_t reconnect){
   String ev = generateEventMessage(message, event, id, reconnect);
-  _client_queue_lock.lock();
+  AsyncWebLockGuard l(_client_queue_lock);
   for(const auto &c: _clients){
     if(c->connected()) {
       c->_write(ev.c_str(), ev.length());
     }
   }
-  _client_queue_lock.unlock();
 }
 
 size_t AsyncEventSource::count() const {
   size_t n_clients;
-  _client_queue_lock.lock();
+  AsyncWebLockGuard l(_client_queue_lock);
   n_clients = _clients.count_if([](AsyncEventSourceClient *c){
                                     return c->connected();
                                 });
-  _client_queue_lock.unlock();
   return n_clients;
 }
 
diff --git a/src/AsyncEventSource.h b/src/AsyncEventSource.h
index cd0afbc..abb1829 100644
--- a/src/AsyncEventSource.h
+++ b/src/AsyncEventSource.h
@@ -104,7 +104,7 @@ class AsyncEventSource: public AsyncWebHandler {
     LinkedList<AsyncEventSourceClient *> _clients;
     // Same as for individual messages, protect mutations of _clients list
     // since simultaneous access from different tasks is possible
-    AsyncPlainLock _client_queue_lock;
+    AsyncWebLock _client_queue_lock;
     ArEventHandlerFunction _connectcb;
 	ArAuthorizeConnectHandler _authorizeConnectHandler;
   public: