Fix deadlock in case of evconnlistener_disable() in parallel with callback

I've got an issue when stop evconnlistener not in the event_base_loop()
thread. evconnlistener_disable() acquired lev->lock, if the same time,
user callbacks is runing, the event thread released lock, after callback
finished, it try to aquire the lock again, I think this makes conflict:

Here is backtraces:

thread 1:
    0  __lll_lock_wait (futex=futex@entry=0x555555559a60, private=0) at lowlevellock.c:52
    1  0x00007ffff7f2a131 in __GI___pthread_mutex_lock (mutex=0x555555559a60) at ../nptl/pthread_mutex_lock.c:115
    2  0x00007ffff7f424c9 in evthread_posix_lock (mode=0, lock_=0x555555559a60) at evthread_pthread.c:79
    3  0x00007ffff7f7dc12 in listener_read_cb (fd=7, what=2, p=0x5555555599a0) at listener.c:439
    4  0x00007ffff7f6d758 in event_persist_closure (base=0x555555559370, ev=0x5555555599d8) at event.c:1645
    5  0x00007ffff7f6da60 in event_process_active_single_queue (base=0x555555559370, activeq=0x5555555597e0,
       max_to_process=2147483647, endtime=0x0) at event.c:1704
    6  0x00007ffff7f6e018 in event_process_active (base=0x555555559370) at event.c:1805
    7  0x00007ffff7f6e92a in event_base_loop (base=0x555555559370, flags=0) at event.c:2047
    8  0x0000555555555449 in main () at test_listen.c:67

thread 2:
    0  futex_wait_cancelable (private=<optimized out>, expected=0, futex_word=0x555555559858) at ../sysdeps/nptl/futex-internal.h:183
    1  __pthread_cond_wait_common (abstime=0x0, clockid=0, mutex=0x555555559800, cond=0x555555559830) at pthread_cond_wait.c:508
    2  __pthread_cond_wait (cond=0x555555559830, mutex=0x555555559800) at pthread_cond_wait.c:638
    3  0x00007ffff7f426f3 in evthread_posix_cond_wait (cond_=0x555555559830, lock_=0x555555559800, tv=0x0) at evthread_pthread.c:162
    4  0x00007ffff7f70bc5 in event_del_nolock_ (ev=0x5555555599d8, blocking=2) at event.c:2934
    5  0x00007ffff7f70748 in event_del_ (ev=0x5555555599d8, blocking=2) at event.c:2821
    6  0x00007ffff7f707a1 in event_del (ev=0x5555555599d8) at event.c:2830
    7  0x00007ffff7f7d76e in event_listener_disable (lev=0x5555555599a0) at listener.c:343
    8  0x00007ffff7f7d6e5 in evconnlistener_disable (lev=0x5555555599a0) at listener.c:325
    9  0x00005555555552c3 in disable_thread (arg=0x5555555599a0) at test_listen.c:27
    10 0x00007ffff7f27609 in start_thread (arg=<optimized out>) at pthread_create.c:477
    11 0x00007ffff7e4e293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

diff --git a/listener.c b/listener.c
index bf55f7b5dfa01cb525237f2c7363950d4c039f41..125c7286f09141abcb58248776f2f2eeeae3176b 100644 (file)
--- a/listener.c
+++ b/listener.c
@@ -433,10 +433,8 @@ listener_read_cb(evutil_socket_t fd, short what, void *p)
                ++lev->refcnt;
                cb = lev->cb;
                user_data = lev->user_data;
-               UNLOCK(lev);
                cb(lev, new_fd, (struct sockaddr*)&ss, (int)socklen,
                    user_data);
-               LOCK(lev);
                if (lev->refcnt == 1) {
                        int freed = listener_decref_and_unlock(lev);
                        EVUTIL_ASSERT(freed);
@@ -458,9 +456,7 @@ listener_read_cb(evutil_socket_t fd, short what, void *p)
                ++lev->refcnt;
                errorcb = lev->errorcb;
                user_data = lev->user_data;
-               UNLOCK(lev);
                errorcb(lev, user_data);
-               LOCK(lev);
                listener_decref_and_unlock(lev);
        } else {
                event_sock_warn(fd, "Error from accept() call");

diff --git a/test/regress_listener.c b/test/regress_listener.c
index 871da4c62ce927e50bbef92e487ca47f2ef3c5dc..adf6697d65c4ff088f3adde31d43326086142c84 100644 (file)
--- a/test/regress_listener.c
+++ b/test/regress_listener.c
@@ -49,6 +49,11 @@
 #include "event2/listener.h"
 #include "event2/event.h"
 #include "event2/util.h"
+#ifndef EVENT__DISABLE_THREAD_SUPPORT
+#include "event2/thread.h"
+#include "regress_thread.h"
+#endif
+
 
 #include "regress.h"
 #include "tinytest.h"
@@ -304,6 +309,127 @@ end:
 }
 #endif
 
+#ifndef EVENT__DISABLE_THREAD_SUPPORT
+
+static THREAD_FN
+disable_thread(void * arg)
+{
+       struct evconnlistener *lev = (struct evconnlistener *)arg;
+       evconnlistener_disable(lev);
+       return NULL;
+}
+
+static void
+acceptcb_for_thread_test(struct evconnlistener *listener, evutil_socket_t fd,
+    struct sockaddr *addr, int socklen, void *arg)
+{
+       THREAD_T *threadid_arg;
+       THREAD_T threadid;
+       struct timeval delay = { .tv_sec = 0, .tv_usec = 5000 };
+
+       threadid_arg = (THREAD_T *)arg;
+
+       evutil_closesocket(fd);
+
+       /* We need to run evconnlistener_disable() from disable_thread
+        * in parallel with processing this callback to trigger deadlock regression
+        */
+       THREAD_START(threadid, disable_thread, listener);
+       evutil_usleep_(&delay);
+
+       *threadid_arg = threadid;
+}
+
+static void
+regress_listener_disable_in_thread(void *arg)
+{
+       struct basic_test_data *data = arg;
+       struct event_base *base = data->base;
+       struct evconnlistener *listener = NULL;
+       struct sockaddr_in sin;
+       struct sockaddr_storage ss;
+       ev_socklen_t slen = sizeof(ss);
+       evutil_socket_t fd = EVUTIL_INVALID_SOCKET;
+       unsigned int flags = LEV_OPT_CLOSE_ON_FREE|LEV_OPT_REUSEABLE|LEV_OPT_THREADSAFE;
+       THREAD_T threadid;
+
+       memset(&sin, 0, sizeof(sin));
+       sin.sin_family = AF_INET;
+       sin.sin_addr.s_addr = htonl(0x7f000001); /* 127.0.0.1 */
+       sin.sin_port = 0; /* "You pick!" */
+
+
+       listener = evconnlistener_new_bind(base, acceptcb_for_thread_test, (void*)(&threadid),
+               flags, -1, (struct sockaddr *)&sin, sizeof(sin));
+
+       tt_assert(listener);
+
+       tt_assert(getsockname(evconnlistener_get_fd(listener),
+               (struct sockaddr*)&ss, &slen) == 0);
+
+       tt_assert(evutil_socket_connect_(&fd, (struct sockaddr*)&ss, slen) != -1);
+
+       event_base_loop(base, EVLOOP_ONCE);
+
+       THREAD_JOIN(threadid);
+end:
+       if (listener)
+               evconnlistener_free(listener);
+}
+
+static void
+errorcb_for_thread_test(struct evconnlistener *listener, void *arg)
+{
+       THREAD_T *threadid_arg;
+       THREAD_T threadid;
+       struct timeval delay = { .tv_sec = 0, .tv_usec = 5000 };
+
+       threadid_arg = (THREAD_T *)arg;
+
+       /* We need to run evconnlistener_disable() from disable_thread
+        * in parallel with processing this callback to trigger deadlock regression
+        */
+       THREAD_START(threadid, disable_thread, listener);
+       evutil_usleep_(&delay);
+
+       *threadid_arg = threadid;
+}
+
+static void
+acceptcb_for_thread_test_error(struct evconnlistener *listener, evutil_socket_t fd,
+    struct sockaddr *addr, int socklen, void *arg)
+{
+}
+
+static void
+regress_listener_disable_in_thread_error(void *arg)
+{
+       struct basic_test_data *data = arg;
+       struct event_base *base = data->base;
+       struct evconnlistener *listener = NULL;
+       unsigned int flags = LEV_OPT_CLOSE_ON_FREE|LEV_OPT_REUSEABLE|LEV_OPT_THREADSAFE;
+       THREAD_T threadid;
+
+       /* send, so that pair[0] will look 'readable'*/
+       tt_int_op(send(data->pair[1], "hello", 5, 0), >, 0);
+
+       /* Start a listener with a bogus socket. */
+       listener = evconnlistener_new(base, acceptcb_for_thread_test_error, (void*)&threadid,
+           flags, 0,
+           data->pair[0]);
+       tt_assert(listener);
+
+       evconnlistener_set_error_cb(listener, errorcb_for_thread_test);
+
+       event_base_loop(base, EVLOOP_ONCE);
+
+       THREAD_JOIN(threadid);
+end:
+       if (listener)
+               evconnlistener_free(listener);
+}
+#endif
+
 struct testcase_t listener_testcases[] = {
 
        { "randport", regress_pick_a_port, TT_FORK|TT_NEED_BASE,
@@ -332,6 +458,16 @@ struct testcase_t listener_testcases[] = {
        { "immediate_close", regress_listener_immediate_close,
          TT_FORK|TT_NEED_BASE, &basic_setup, NULL, },
 
+#ifndef EVENT__DISABLE_THREAD_SUPPORT
+       { "disable_in_thread", regress_listener_disable_in_thread,
+               TT_FORK|TT_NEED_BASE|TT_NEED_THREADS,
+               &basic_setup, NULL, },
+
+       { "disable_in_thread_error", regress_listener_disable_in_thread_error,
+               TT_FORK|TT_NEED_BASE|TT_NEED_THREADS|TT_NEED_SOCKETPAIR,
+               &basic_setup, NULL, },
+#endif
+
        END_OF_TESTCASES,
 };