tcp_trsp: fix lock-order-inversion deadlock in generate_transport_errors#557
Open
hecko wants to merge 1 commit into
Open
tcp_trsp: fix lock-order-inversion deadlock in generate_transport_errors#557hecko wants to merge 1 commit into
hecko wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
The TCP transport can deadlock the SIP worker threads under concurrent transport errors and sending.
When a TCP connection is torn down (
on_read/on_writeerror, idle timeout, or parse error),tcp_trsp_socket::close()runs while holdingsock_mutand callsgenerate_transport_errors(). For each still-queued outgoing message it callstrans_layer::transport_error(), which re-enters the transaction layer (process_rcvd_msg) and takes a transaction-bucket lock:core/sip/tcp_trsp.cpp—close()→generate_transport_errors()→transport_error()core/sip/trans_layer.cpp:1026transport_error()→process_rcvd_msg()→bucket->lock()Meanwhile the session processor / retransmission path holds a transaction-bucket lock and calls
tcp_trsp_socket::send(), which then takessock_mut:core/sip/trans_layer.cpp:2421/2575/2834tr->msg->send(...)(under bucket lock) →tcp_trsp_socket::send()→AmLock _l(sock_mut)The two lock orders are inverted —
sock_mut → bucket(tcp worker) vsbucket → sock_mut(session proc) — a classic lock-order inversion that can wedge the involved threads.Fix
Release
sock_mutat the start ofgenerate_transport_errors(). This is safe becauseclosedis already set before it runs, sosend()will no longer touchsend_q. To keep locking balanced across the paths that reachclose()(which now unlocks the mutex internally),on_read()/on_write()acquire the mutex through a smallAmControlledLockhelper and relinquish ownership on those paths. No functional/behavioural change otherwise.Verified that every
close()call site holdssock_mutand the destructor does not callclose(), so the single deliberate unlock is always balanced. Changed translation unit compiles cleanly.Acknowledgement
Backport of the fix from yeti-switch/sems — commit
d285e2a5("tcp_trsp: fix deadlock between session proc send() and tcp worker generate_transport_errors()") by Michael Furmur. sems-server's transport code is identical to the pre-fix version, so the same deadlock is present here.Generated by Claude Code