DISPATCH-739: Clear context pointer on qdr_connection_t at close Atomically clear the context pointer of a qdr_connection_t when an AMQP connection closes to prevent the connection being used after it is freed.
Project: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/commit/0fb5b54d Tree: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/tree/0fb5b54d Diff: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/diff/0fb5b54d Branch: refs/heads/master Commit: 0fb5b54d1c69b12b035491554f535840a1db52a9 Parents: ef47800 Author: Alan Conway <[email protected]> Authored: Thu Apr 20 06:39:41 2017 -0400 Committer: Alan Conway <[email protected]> Committed: Thu Apr 20 10:04:06 2017 -0400 ---------------------------------------------------------------------- src/router_core/connections.c | 17 +++++++++++++++-- src/router_core/router_core_private.h | 4 +++- src/router_node.c | 2 ++ src/server.c | 3 ++- 4 files changed, 22 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/0fb5b54d/src/router_core/connections.c ---------------------------------------------------------------------- diff --git a/src/router_core/connections.c b/src/router_core/connections.c index be437a7..08791c2 100644 --- a/src/router_core/connections.c +++ b/src/router_core/connections.c @@ -89,6 +89,7 @@ qdr_connection_t *qdr_connection_opened(qdr_core_t *core, DEQ_INIT(conn->work_list); conn->connection_info->role = conn->role; conn->work_lock = sys_mutex(); + conn->context_lock = sys_mutex(); if (vhost) { conn->tenant_space_len = strlen(vhost) + 1; @@ -116,8 +117,14 @@ void qdr_connection_closed(qdr_connection_t *conn) void qdr_connection_set_context(qdr_connection_t *conn, void *context) { - if (conn) + if (conn) { + /* TODO aconway 2017-04-20: note this could be an atomic pointer store, + * but it must provide a full memory barrier. + */ + sys_mutex_lock(conn->context_lock); conn->user_context = context; + sys_mutex_unlock(conn->context_lock); + } } qdr_connection_info_t *qdr_connection_info(bool is_encrypted, @@ -180,7 +187,13 @@ qdr_connection_info_t *qdr_connection_info(bool is_encrypted, void *qdr_connection_get_context(const qdr_connection_t *conn) { - return conn ? conn->user_context : 0; + void *ret = NULL; + if (conn) { + sys_mutex_lock(conn->context_lock); + ret = conn->user_context; + sys_mutex_unlock(conn->context_lock); + } + return ret; } http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/0fb5b54d/src/router_core/router_core_private.h ---------------------------------------------------------------------- diff --git a/src/router_core/router_core_private.h b/src/router_core/router_core_private.h index bacf531..c60cd9c 100644 --- a/src/router_core/router_core_private.h +++ b/src/router_core/router_core_private.h @@ -517,7 +517,6 @@ struct qdr_connection_t { DEQ_LINKS_N(ACTIVATE, qdr_connection_t); uint64_t identity; qdr_core_t *core; - void *user_context; bool incoming; bool in_activate_list; qdr_connection_role_t role; @@ -535,6 +534,9 @@ struct qdr_connection_t { char *tenant_space; int tenant_space_len; qdr_connection_info_t *connection_info; + + sys_mutex_t *context_lock; + void *user_context; /* Updated from IO thread on close */ }; ALLOC_DECLARE(qdr_connection_t); http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/0fb5b54d/src/router_node.c ---------------------------------------------------------------------- diff --git a/src/router_node.c b/src/router_node.c index 914c736..9761468 100644 --- a/src/router_node.c +++ b/src/router_node.c @@ -761,6 +761,7 @@ static int AMQP_closed_handler(void *type_context, qd_connection_t *conn, void * qdr_connection_t *qdrc = (qdr_connection_t*) qd_connection_get_context(conn); if (qdrc) { + qdr_connection_set_context(qdrc, NULL); qdr_connection_closed(qdrc); qd_connection_set_context(conn, 0); } @@ -869,6 +870,7 @@ static void CORE_link_first_attach(void *context, { qd_router_t *router = (qd_router_t*) context; qd_connection_t *qconn = (qd_connection_t*) qdr_connection_get_context(conn); + if (!qconn) return; /* Connection is already closed */ // // Create a new link to be attached http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/0fb5b54d/src/server.c ---------------------------------------------------------------------- diff --git a/src/server.c b/src/server.c index 42fed58..31322ea 100644 --- a/src/server.c +++ b/src/server.c @@ -1069,9 +1069,9 @@ static void *thread_run(void *arg) qd_policy_socket_close(qd_server->qd->policy, ctx); } - qdpn_connector_free(cxtr); invoke_deferred_calls(ctx, true); // Discard any pending deferred calls sys_mutex_free(ctx->deferred_call_lock); + qdpn_connector_free(cxtr); free_qd_connection(ctx); qd_server->threads_active--; sys_mutex_unlock(qd_server->lock); @@ -1557,6 +1557,7 @@ void qd_connection_set_context(qd_connection_t *conn, void *context) void *qd_connection_get_context(qd_connection_t *conn) { + /* FIXME aconway 2017-04-20: needs to be thread safe with respect to deletion */ return conn->user_context; } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
