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;