From 1ac9792d725fb369cde63bcbcb1984c86ee76edb Mon Sep 17 00:00:00 2001
From: hecko <hecko@users.noreply.github.com>
Date: Mon, 13 Jul 2026 03:31:30 +0000
Subject: [PATCH] tcp_trsp: fix lock-order-inversion deadlock in
 generate_transport_errors

When a TCP connection is torn down (on_read/on_write error, idle timeout,
parse error), tcp_trsp_socket::close() runs while holding sock_mut and calls
generate_transport_errors(). For every still-queued outgoing message that in
turn calls trans_layer::transport_error(), which re-enters the transaction
layer (process_rcvd_msg) and takes a transaction-bucket lock.

Meanwhile the session processor / transaction retransmission path holds a
transaction-bucket lock and calls tcp_trsp_socket::send(), which then takes
sock_mut. The two lock orders are inverted (sock_mut -> bucket vs
bucket -> sock_mut), so under concurrent TCP transport errors and sending
the daemon can deadlock, wedging the affected SIP worker threads.

Release sock_mut at the start of generate_transport_errors(). This is safe
because 'closed' is already set, so send() no longer touches send_q. To keep
lock/unlock balanced, on_read()/on_write() now hold the mutex via a small
AmControlledLock helper and give up ownership on the paths that reach close().

Backport of the fix from yeti-switch/sems (commit d285e2a5, Michael Furmur).
The transport code in sems-server is identical to the pre-fix version, so the
same deadlock is present here.
---
 core/AmThread.h       | 22 ++++++++++++++++++++++
 core/sip/tcp_trsp.cpp | 26 +++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/core/AmThread.h b/core/AmThread.h
index b4277321f..e280c5947 100644
--- a/core/AmThread.h
+++ b/core/AmThread.h
@@ -64,6 +64,28 @@ class AmLock
   }
 };
 
+/**
+ * \brief  Simple lock class with the ability to release mutex ownership
+ *
+ * Behaves like AmLock, but release_ownership() lets a callee (e.g. a
+ * function that unlocks the mutex itself) prevent the destructor from
+ * unlocking a second time.
+ */
+class AmControlledLock
+{
+  AmMutex& m;
+  bool ownership;
+public:
+  AmControlledLock(AmMutex& _m) : m(_m), ownership(true) {
+    m.lock();
+  }
+  ~AmControlledLock(){
+    if(ownership)
+      m.unlock();
+  }
+  void release_ownership() { ownership = false; }
+};
+
 /**
  * \brief Shared variable.
  *
diff --git a/core/sip/tcp_trsp.cpp b/core/sip/tcp_trsp.cpp
index fa38938fa..dbe91729f 100644
--- a/core/sip/tcp_trsp.cpp
+++ b/core/sip/tcp_trsp.cpp
@@ -298,6 +298,14 @@ void tcp_trsp_socket::close()
 
 void tcp_trsp_socket::generate_transport_errors()
 {
+  /* Avoid a lock-order inversion deadlock between the session processor
+     and the tcp worker: transport_error() below re-enters the transaction
+     layer and takes a transaction-bucket lock, while send() takes the
+     bucket lock first and then sock_mut. It is safe to release sock_mut
+     here because 'closed' is already set, so send() will not touch send_q
+     anymore. Callers of close() must therefore not unlock sock_mut again. */
+  sock_mut.unlock();
+
   while(!send_q.empty()) {
 
     msg_buf* msg = send_q.front();
@@ -320,13 +328,18 @@ void tcp_trsp_socket::on_read(short ev)
 
   {// locked section
 
+    // close() unlocks sock_mut internally (see generate_transport_errors),
+    // so every path that reaches close() must release ownership of the lock
+    // to avoid unlocking it a second time.
+    AmControlledLock _l(sock_mut);
+
     if(ev & EV_TIMEOUT) {
       DBG("************ idle timeout: closing connection **********");
       close();
+      _l.release_ownership();
       return;
     }
 
-    AmLock _l(sock_mut);
     DBG("on_read (connected = %i)",connected);
 
     bytes = ::read(sd,get_input(),get_input_free_space());
@@ -339,16 +352,19 @@ void tcp_trsp_socket::on_read(short ev)
       case ENOTCONN:
 	DBG("connection has been closed (sd=%i)",sd);
 	close();
+	_l.release_ownership();
 	return;
 
       case ETIMEDOUT:
 	DBG("transmission timeout (sd=%i)",sd);
 	close();
+	_l.release_ownership();
 	return;
 
       default:
 	DBG("unknown error (%i): %s",errno,strerror(errno));
 	close();
+	_l.release_ownership();
 	return;
       }
     }
@@ -356,6 +372,7 @@ void tcp_trsp_socket::on_read(short ev)
       // connection closed
       DBG("connection has been closed (sd=%i)",sd);
       close();
+      _l.release_ownership();
       return;
     }
   }// end of - locked section
@@ -369,7 +386,7 @@ void tcp_trsp_socket::on_read(short ev)
     DBG("Error while parsing input: closing connection!");
     sock_mut.lock();
     close();
-    sock_mut.unlock();
+    // close() releases sock_mut via generate_transport_errors()
   }
 }
 
@@ -443,11 +460,13 @@ int tcp_trsp_socket::parse_input()
 
 void tcp_trsp_socket::on_write(short ev)
 {
-  AmLock _l(sock_mut);
+  AmControlledLock _l(sock_mut);
 
   DBG("on_write (connected = %i)",connected);
   if(!connected) {
     if(on_connect(ev) != 0) {
+      // on_connect() may have called close(), which already released sock_mut
+      _l.release_ownership();
       return;
     }
   }
@@ -475,6 +494,7 @@ void tcp_trsp_socket::on_write(short ev)
 	ERROR("unforseen error: close connection (%i/%s)",
 	      errno,strerror(errno));
 	close();
+	_l.release_ownership();
 	break;
       }
       return;