From 280919b3ec08842a11f2ef06aa61c591c6404586 Mon Sep 17 00:00:00 2001
From: Michael Davidsaver <mdavidsaver@gmail.com>
Date: Sat, 8 Aug 2020 16:09:35 -0700
Subject: [PATCH] server: adjust handling of invalid SID

Downgrade severity as invalid SIDs are an
unavoidable possibility after server
channel destroy.
---
 src/serverconn.cpp       | 15 +++++++++------
 src/serverconn.h         |  1 -
 src/serverget.cpp        |  5 +++--
 src/serverintrospect.cpp |  5 +++--
 src/servermon.cpp        | 13 ++++++++++---
 5 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/src/serverconn.cpp b/src/serverconn.cpp
index e55b5e2..e7bc079 100644
--- a/src/serverconn.cpp
+++ b/src/serverconn.cpp
@@ -81,8 +81,11 @@ ServerConn::~ServerConn()
 const std::shared_ptr<ServerChan>& ServerConn::lookupSID(uint32_t sid)
 {
     auto it = chanBySID.find(sid);
-    if(it==chanBySID.end())
-        throw std::runtime_error(SB()<<"Client "<<peerName<<" non-existant SID "<<sid);
+    if(it==chanBySID.end()) {
+        static decltype (it->second) empty{};
+        return empty;
+        //throw std::runtime_error(SB()<<"Client "<<peerName<<" non-existant SID "<<sid);
+    }
     return it->second;
 }
 
@@ -216,15 +219,15 @@ void ServerConn::handle_DESTROY_REQUEST()
         throw std::runtime_error("Error decoding DestroyOp");
 
     auto& chan = lookupSID(sid);
-
     auto it = opByIOID.find(ioid);
-    auto n = chan->opByIOID.erase(ioid);
 
-    if(it==opByIOID.end() || n!=1) {
+    if(!chan || it==opByIOID.end() || 1!=chan->opByIOID.erase(ioid)) {
         log_warn_printf(connsetup, "Client %s can't destroy non-existant op %u:%u\n",
                    peerName.c_str(), unsigned(sid), unsigned(ioid));
 
-    } else {
+    }
+
+    if(it!=opByIOID.end()) {
         auto op = it->second;
         opByIOID.erase(it);
         op->state = ServerOp::Dead;
diff --git a/src/serverconn.h b/src/serverconn.h
index d0b43fd..1dc3767 100644
--- a/src/serverconn.h
+++ b/src/serverconn.h
@@ -28,7 +28,6 @@ namespace pvxs {namespace impl {
 struct ServIface;
 struct ServerConn;
 struct ServerChan;
-struct ServerChan;
 
 // base for tracking in-progress operations.  cf. ServerConn::opByIOID and ServerChan::opByIOID
 struct ServerOp
diff --git a/src/serverget.cpp b/src/serverget.cpp
index 6f14c23..8d11f1d 100644
--- a/src/serverget.cpp
+++ b/src/serverget.cpp
@@ -378,8 +378,9 @@ void ServerConn::handle_GPR(pva_app_msg_t cmd)
 
         auto& chan = lookupSID(sid);
 
-        if(opByIOID.find(ioid)!=opByIOID.end()) {
-            log_err_printf(connsetup, "Client %s reuses existing ioid %u\n", peerName.c_str(), unsigned(ioid));
+        if(!chan || opByIOID.find(ioid)!=opByIOID.end()) {
+            log_err_printf(connsetup, "Client %s reuses existing sid %u ioid %u\n",
+                           peerName.c_str(), unsigned(sid), unsigned(ioid));
             bev.reset();
             return;
         }
diff --git a/src/serverintrospect.cpp b/src/serverintrospect.cpp
index a4a5da8..bc8e2c7 100644
--- a/src/serverintrospect.cpp
+++ b/src/serverintrospect.cpp
@@ -154,8 +154,9 @@ void ServerConn::handle_GET_FIELD()
 
     auto& chan = lookupSID(sid);
 
-    if(opByIOID.find(ioid)!=opByIOID.end()) {
-        log_err_printf(connsetup, "Client %s reuses existing ioid %d\n", peerName.c_str(), unsigned(ioid));
+    if(!chan || opByIOID.find(ioid)!=opByIOID.end()) {
+        log_err_printf(connsetup, "Client %s reuses existing sid %u ioid %d\n",
+                       peerName.c_str(), unsigned(sid), unsigned(ioid));
         return;
     }
 
diff --git a/src/servermon.cpp b/src/servermon.cpp
index db74b07..dea26f1 100644
--- a/src/servermon.cpp
+++ b/src/servermon.cpp
@@ -472,8 +472,9 @@ void ServerConn::handle_MONITOR()
 
         auto& chan = lookupSID(sid);
 
-        if(opByIOID.find(ioid)!=opByIOID.end()) {
-            log_err_printf(connsetup, "Client %s reuses existing ioid %u\n", peerName.c_str(), unsigned(ioid));
+        if(!chan || opByIOID.find(ioid)!=opByIOID.end()) {
+            log_err_printf(connsetup, "Client %s reuses existing sid %u ioid %u\n",
+                           peerName.c_str(), unsigned(sid), unsigned(ioid));
             bev.reset();
             return;
         }
@@ -540,7 +541,13 @@ void ServerConn::handle_MONITOR()
             return;
         }
 
-        auto& chan = lookupSID(sid);
+        auto chan(op->chan.lock());
+        if(!chan || chan->sid!=sid) {
+            log_err_printf(connio, "Client %s MONITOR inconsistent sid %u:%u ioid %u\n",
+                           peerName.c_str(), unsigned(sid), unsigned(chan ? chan->sid : -1), ioid);
+            bev.reset();
+            return;
+        }
 
         // pvAccessCPP won't accept ack and start/stop in the same message,
         // although it will accept destroy in any !INIT message.