From 65b79e9af187177b29aaec0c6228a53f5f3a89fe Mon Sep 17 00:00:00 2001 From: Cody Piersall Date: Thu, 6 Jun 2019 20:58:27 -0500 Subject: [PATCH] Fixes surprising possible ordering of nng_pipe_notify events. On nng_pipe_notify events, it was possible for events to fire in an unexpected order: pre-connect, post-*remove*, and finally post-*connect*. This can cause errors in the wild if a resource is attained in pre-connect and released in post-remove, as the resource cannot be used in the post-connect event if the race is exercised. Now, events will fire strictly in the order of pre-connect, post-connect, and post-remove. If the pipe is closed in pre-connect, post-connect and post-remove will not be called. --- src/core/pipe.c | 8 +++++++- src/core/socket.c | 21 ++++++++++++++++----- src/core/socket.h | 2 ++ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/core/pipe.c b/src/core/pipe.c index 44957defd..34676f1b9 100644 --- a/src/core/pipe.c +++ b/src/core/pipe.c @@ -59,11 +59,17 @@ nni_pipe_sys_fini(void) static void pipe_destroy(nni_pipe *p) { + nni_sock *s; + nni_mtx *pipe_cbs_mtx; if (p == NULL) { return; } + s = p->p_sock; + pipe_cbs_mtx = nni_sock_pipe_cbs_mtx(s); + nni_mtx_lock(pipe_cbs_mtx); nni_pipe_run_cb(p, NNG_PIPE_EV_REM_POST); + nni_mtx_unlock(pipe_cbs_mtx); // Make sure any unlocked holders are done with this. // This happens during initialization for example. @@ -372,4 +378,4 @@ nni_pipe_bump_error(nni_pipe *p, int err) } else { nni_listener_bump_error(p->p_listener, err); } -} \ No newline at end of file +} diff --git a/src/core/socket.c b/src/core/socket.c index 0224a812f..8be63190c 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1425,11 +1425,15 @@ nni_dialer_add_pipe(nni_dialer *d, void *tpipe) nni_stat_inc_atomic(&s->s_stats.s_npipes, 1); nni_stat_inc_atomic(&d->d_stats.s_npipes, 1); + // grab the pipe_cbs_mtx for the entire time between calling pre-connect + // callbacks and post-connect callbacks. This ensures that the post-remove + // callback cannot fire before the post-connect callback. + nni_mtx_lock(&s->s_pipe_cbs_mtx); nni_pipe_run_cb(p, NNG_PIPE_EV_ADD_PRE); - nni_mtx_lock(&s->s_mx); if (p->p_closed) { nni_mtx_unlock(&s->s_mx); + nni_mtx_unlock(&s->s_pipe_cbs_mtx); nni_stat_inc_atomic(&d->d_stats.s_reject, 1); nni_stat_inc_atomic(&s->s_stats.s_reject, 1); nni_pipe_rele(p); @@ -1437,6 +1441,7 @@ nni_dialer_add_pipe(nni_dialer *d, void *tpipe) } if (p->p_proto_ops.pipe_start(p->p_proto_data) != 0) { nni_mtx_unlock(&s->s_mx); + nni_mtx_unlock(&s->s_pipe_cbs_mtx); nni_stat_inc_atomic(&d->d_stats.s_reject, 1); nni_stat_inc_atomic(&s->s_stats.s_reject, 1); nni_pipe_close(p); @@ -1444,8 +1449,8 @@ nni_dialer_add_pipe(nni_dialer *d, void *tpipe) return; } nni_mtx_unlock(&s->s_mx); - nni_pipe_run_cb(p, NNG_PIPE_EV_ADD_POST); + nni_mtx_unlock(&s->s_pipe_cbs_mtx); nni_pipe_rele(p); } @@ -1534,11 +1539,13 @@ nni_listener_add_pipe(nni_listener *l, void *tpipe) nni_stat_inc_atomic(&l->l_stats.s_npipes, 1); nni_stat_inc_atomic(&s->s_stats.s_npipes, 1); + nni_mtx_lock(&s->s_pipe_cbs_mtx); nni_pipe_run_cb(p, NNG_PIPE_EV_ADD_PRE); nni_mtx_lock(&s->s_mx); if (p->p_closed) { nni_mtx_unlock(&s->s_mx); + nni_mtx_unlock(&s->s_pipe_cbs_mtx); nni_stat_inc_atomic(&l->l_stats.s_reject, 1); nni_stat_inc_atomic(&s->s_stats.s_reject, 1); nni_pipe_rele(p); @@ -1546,6 +1553,7 @@ nni_listener_add_pipe(nni_listener *l, void *tpipe) } if (p->p_proto_ops.pipe_start(p->p_proto_data) != 0) { nni_mtx_unlock(&s->s_mx); + nni_mtx_unlock(&s->s_pipe_cbs_mtx); nni_stat_inc_atomic(&l->l_stats.s_reject, 1); nni_stat_inc_atomic(&s->s_stats.s_reject, 1); nni_pipe_close(p); @@ -1555,6 +1563,7 @@ nni_listener_add_pipe(nni_listener *l, void *tpipe) nni_mtx_unlock(&s->s_mx); nni_pipe_run_cb(p, NNG_PIPE_EV_ADD_POST); + nni_mtx_unlock(&s->s_pipe_cbs_mtx); nni_pipe_rele(p); } @@ -1632,19 +1641,16 @@ nni_pipe_run_cb(nni_pipe *p, nng_pipe_ev ev) nng_pipe_cb cb; void * arg; - nni_mtx_lock(&s->s_pipe_cbs_mtx); if (!p->p_cbs) { if (ev == NNG_PIPE_EV_ADD_PRE) { // First event, after this we want all other events. p->p_cbs = true; } else { - nni_mtx_unlock(&s->s_pipe_cbs_mtx); return; } } cb = s->s_pipe_cbs[ev].cb_fn; arg = s->s_pipe_cbs[ev].cb_arg; - nni_mtx_unlock(&s->s_pipe_cbs_mtx); if (cb != NULL) { nng_pipe pid; @@ -1653,6 +1659,11 @@ nni_pipe_run_cb(nni_pipe *p, nng_pipe_ev ev) } } +extern +nni_mtx *nni_sock_pipe_cbs_mtx(nni_sock *s) { + return &s->s_pipe_cbs_mtx; +} + void nni_pipe_remove(nni_pipe *p) { diff --git a/src/core/socket.h b/src/core/socket.h index b06f4d47e..6af3b876d 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -62,6 +62,8 @@ extern uint32_t nni_sock_flags(nni_sock *); // types.) The second argument is a mask of events for which the callback // should be executed. extern void nni_sock_set_pipe_cb(nni_sock *sock, int, nng_pipe_cb, void *); +// the pipe_cbs_mtx must be held whenever pipe callback functions are called. +extern nni_mtx *nni_sock_pipe_cbs_mtx(nni_sock *); // nni_ctx_open is used to open/create a new context structure. // Contexts are not supported by most protocols, but for those that do,