From 1ba48868f614a5c880cc3734c2fe97ce592fc8f6 Mon Sep 17 00:00:00 2001
From: Michael Davidsaver <mdavidsaver@gmail.com>
Date: Mon, 25 Jun 2018 19:46:01 -0700
Subject: [PATCH] PVA server avoid lock order violation

There is a lock order violation when a server force disconnects it's clients while sending a final message to it (or concurrent close, or probably others). The entry points are channelStateChange() and operation send().

Fix is not to hold the channel mutex when destroy()ing operations.

epics::pvAccess::ServerChannelRPCRequesterImpl::destroy
  locks RPC requester mutex
epics::pvAccess::ServerChannel::destroy
  locks channel mutex
epics::pvAccess::ServerChannelRequesterImpl::channelStateChange


epics::pvAccess::ServerChannel::unregisterRequest
  locks channel mutex
epics::pvAccess::ServerChannelRPCRequesterImpl::destroy
  locks RPC requester mutex
epics::pvAccess::ServerChannelRPCRequesterImpl::send
---
 src/server/serverChannelImpl.cpp | 39 ++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/server/serverChannelImpl.cpp b/src/server/serverChannelImpl.cpp
index d4a03c3..c632359 100644
--- a/src/server/serverChannelImpl.cpp
+++ b/src/server/serverChannelImpl.cpp
@@ -64,16 +64,29 @@ Destroyable::shared_pointer ServerChannel::getRequest(const pvAccessID id)
 
 void ServerChannel::destroy()
 {
-    Lock guard(_mutex);
-
-    if (_destroyed) return;
-    _destroyed = true;
-
-    // destroy all requests
-    // take ownership of _requests locally to prevent
-    // removal via unregisterRequest() during iteration
     _requests_t reqs;
-    _requests.swap(reqs);
+    {
+        Lock guard(_mutex);
+
+        if (_destroyed) return;
+        _destroyed = true;
+
+        // destroy all requests
+        // take ownership of _requests locally to prevent
+        // removal via unregisterRequest() during iteration
+        _requests.swap(reqs);
+
+        // close channel security session
+        // TODO try catch
+        _channelSecuritySession->close();
+
+        // ... and the channel
+        // TODO try catch
+        _channel->destroy();
+    }
+    // unlock our before destroy.
+    // our mutex is subordinate to operation mutex
+
     for(_requests_t::const_iterator it=reqs.begin(), end=reqs.end(); it!=end; ++it)
     {
         const _requests_t::mapped_type& req = it->second;
@@ -81,14 +94,6 @@ void ServerChannel::destroy()
         req->destroy();
         // May still be in the send queue
     }
-
-    // close channel security session
-    // TODO try catch
-    _channelSecuritySession->close();
-
-    // ... and the channel
-    // TODO try catch
-    _channel->destroy();
 }
 
 ServerChannel::~ServerChannel()