Fix possible deadlock in unix/socket.c

In process_fd we lock sock->lock and then internal_accept locks mgr->lock,
in isc_sockmgr_render* functions we lock mgr->lock and then lock sock->lock,
that can cause a deadlock when accessing stats. Unlock sock->lock early in
all the internal_{send,recv,connect,accept} functions instead of late
in process_fd.

diff --git a/CHANGES b/CHANGES
index f97b0fe32406e6f00a0b7fb0114ae2a8eddd14bf..bb8c9ae686967c1459ad75c733671448ccd7b415 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -39,6 +39,8 @@
 5418.  [bug]           delv failed to parse deprecated trusted-keys style
                        trust anchors. [GL #1860]
 
+5416.  [bug]           Fix a lock order inversion in unix/socket.c. [GL #1859]
+
 5415.  [test]          Address race in dnssec system test that led to
                        test failures. [GL #1852]
 

diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c
index 548093421f2f427907ecae296aaf843722f8214e..0bb7bdfb3cf9129d9bfbd4d2e219d3385237e78d 100644 (file)
--- a/lib/isc/unix/socket.c
+++ b/lib/isc/unix/socket.c
@@ -2793,6 +2793,7 @@ internal_accept(isc__socket_t *sock) {
        dev = ISC_LIST_HEAD(sock->accept_list);
        if (dev == NULL) {
                unwatch_fd(thread, sock->fd, SELECT_POKE_ACCEPT);
+               UNLOCK(&sock->lock);
                return;
        }
 
@@ -2921,6 +2922,12 @@ internal_accept(isc__socket_t *sock) {
                }
        }
 
+       /*
+        * We need to unlock sock->lock now to be able to lock manager->lock
+        * without risking a deadlock with xmlstats.
+        */
+       UNLOCK(&sock->lock);
+
        /*
         * -1 means the new socket didn't happen.
         */
@@ -3010,6 +3017,7 @@ internal_accept(isc__socket_t *sock) {
 
 soft_error:
        watch_fd(thread, sock->fd, SELECT_POKE_ACCEPT);
+       UNLOCK(&sock->lock);
 
        inc_stats(manager->stats, sock->statsindex[STATID_ACCEPTFAIL]);
        return;
@@ -3066,6 +3074,7 @@ finish:
                unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd,
                           SELECT_POKE_READ);
        }
+       UNLOCK(&sock->lock);
 }
 
 static void
@@ -3105,6 +3114,7 @@ finish:
                unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd,
                           SELECT_POKE_WRITE);
        }
+       UNLOCK(&sock->lock);
 }
 
 /*
@@ -3135,6 +3145,7 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, bool writeable) {
        }
 
        LOCK(&sock->lock);
+
        if (sock->fd < 0) {
                /*
                 * Sock is being closed - the final external reference
@@ -3143,9 +3154,12 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, bool writeable) {
                 * thread->fdlock[lockid] or sock->lock that we're holding.
                 * Just release the locks and bail.
                 */
-               goto unlock;
+               UNLOCK(&sock->lock);
+               UNLOCK(&thread->fdlock[lockid]);
+               return;
        }
 
+       REQUIRE(readable || writeable);
        if (readable) {
                if (sock->listener) {
                        internal_accept(sock);
@@ -3162,9 +3176,9 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, bool writeable) {
                }
        }
 
-unlock:
-       UNLOCK(&sock->lock);
+       /* sock->lock is unlocked in internal_* function */
        UNLOCK(&thread->fdlock[lockid]);
+
        /*
         * Socket destruction might be pending, it will resume
         * after releasing fdlock and sock->lock.
@@ -4938,6 +4952,7 @@ internal_connect(isc__socket_t *sock) {
 finish:
        unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd,
                   SELECT_POKE_CONNECT);
+       UNLOCK(&sock->lock);
 }
 
 isc_result_t