This is an automated email from the ASF dual-hosted git repository.
chug pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/qpid-dispatch.git
The following commit(s) were added to refs/heads/main by this push:
new 269bfd4 DISPATCH-2194: Fix CONNECTOR - ENTITY_CACHE lock inversion
269bfd4 is described below
commit 269bfd46f3ac43ef6d1199f8cb271d117fd1ed9a
Author: Chuck Rolke <c...@apache.org>
AuthorDate: Fri Jul 9 16:26:59 2021 -0400
DISPATCH-2194: Fix CONNECTOR - ENTITY_CACHE lock inversion
The two locks are taken in both orders.
* entity_cache first, connector second is routinely used by management
entity updates in a pattern shared by all entities.
* connector first, entity_cache second is used only when a connector
creates or deletes an associated connection. The allocation and
disposal of the connection causes an implicit entity_cache lock.
The fix is to avoid the connector - entity_cache lock order by allocating
the connection before taking the connector lock and by freeing the
connection after releasing the connector lock.
This patch also corrects an improper free call in a failure path.
---
include/qpid/dispatch/server.h | 29 +++++++++++++++++++
src/connection_manager.c | 9 ++++--
src/server.c | 66 +++++++++++++++++++++++++++++++++++-------
3 files changed, 90 insertions(+), 14 deletions(-)
diff --git a/include/qpid/dispatch/server.h b/include/qpid/dispatch/server.h
index faf8366..7f71912 100644
--- a/include/qpid/dispatch/server.h
+++ b/include/qpid/dispatch/server.h
@@ -569,6 +569,35 @@ void qd_connection_invoke_deferred(qd_connection_t *conn,
qd_deferred_t call, vo
/**
+ * Schedule a call to be invoked on a thread that has ownership of this
connection
+ * when it will be safe for the callback to perform operations related to this
connection.
+ * A qd_deferred_call_t object has been allocated before hand to avoid taking
+ * the ENTITY_CACHE lock.
+ *
+ * @param conn Connection object
+ * @param call The function to be invoked on the connection's thread
+ * @param context The context to be passed back in the callback
+ * @param dct Pointer to preallocated qd_deferred_call_t object
+ */
+void qd_connection_invoke_deferred_impl(qd_connection_t *conn, qd_deferred_t
call, void *context, void *dct);
+
+
+/**
+ * Allocate a qd_deferred_call_t object
+ */
+void *qd_connection_new_qd_deferred_call_t();
+
+
+/**
+ * Deallocate a qd_deferred_call_t object
+ *
+ * @param dct Pointer to preallocated qd_deferred_call_t object
+ */
+void qd_connection_free_qd_deferred_call_t(void *dct);
+
+
+
+/**
* Listen for incoming connections, return true if listening succeeded.
*/
bool qd_listener_listen(qd_listener_t *l);
diff --git a/src/connection_manager.c b/src/connection_manager.c
index 6ef14cd..c77999b 100644
--- a/src/connection_manager.c
+++ b/src/connection_manager.c
@@ -1081,16 +1081,19 @@ void
qd_connection_manager_delete_connector(qd_dispatch_t *qd, void *impl)
// cannot free the timer while holding ct->lock since the
// timer callback may be running during the call to qd_timer_free
qd_timer_t *timer = 0;
+ void *dct = qd_connection_new_qd_deferred_call_t();
sys_mutex_lock(ct->lock);
timer = ct->timer;
ct->timer = 0;
ct->state = CXTR_STATE_DELETED;
qd_connection_t *conn = ct->qd_conn;
if (conn && conn->pn_conn) {
- qd_connection_invoke_deferred(conn, deferred_close, conn->pn_conn);
+ qd_connection_invoke_deferred_impl(conn, deferred_close,
conn->pn_conn, dct);
+ sys_mutex_unlock(ct->lock);
+ } else {
+ sys_mutex_unlock(ct->lock);
+ qd_connection_free_qd_deferred_call_t(dct);
}
- sys_mutex_unlock(ct->lock);
-
qd_timer_free(timer);
DEQ_REMOVE(qd->connection_manager->connectors, ct);
qd_connector_decref(ct);
diff --git a/src/server.c b/src/server.c
index 4910f37..24e73ee 100644
--- a/src/server.c
+++ b/src/server.c
@@ -561,11 +561,13 @@ static void connection_wake(qd_connection_t *ctx) {
if (ctx->pn_conn) pn_connection_wake(ctx->pn_conn);
}
-/* Construct a new qd_connection. Thread safe. */
-qd_connection_t *qd_server_connection(qd_server_t *server, qd_server_config_t
*config)
+/* Construct a new qd_connection. Thread safe.
+ * Does not allocate any managed objects and therefore
+ * does not take ENTITY_CACHE lock.
+ */
+qd_connection_t *qd_server_connection_impl(qd_server_t *server,
qd_server_config_t *config, qd_connection_t *ctx)
{
- qd_connection_t *ctx = new_qd_connection_t();
- if (!ctx) return NULL;
+ assert(ctx);
ZERO(ctx);
ctx->pn_conn = pn_connection();
ctx->deferred_call_lock = sys_mutex();
@@ -574,7 +576,7 @@ qd_connection_t *qd_server_connection(qd_server_t *server,
qd_server_config_t *c
if (ctx->pn_conn) pn_connection_free(ctx->pn_conn);
if (ctx->deferred_call_lock) sys_mutex_free(ctx->deferred_call_lock);
free(ctx->role);
- free(ctx);
+ free_qd_connection_t(ctx);
return NULL;
}
ctx->server = server;
@@ -591,6 +593,16 @@ qd_connection_t *qd_server_connection(qd_server_t *server,
qd_server_config_t *c
return ctx;
}
+/* Construct a new qd_connection. Thread safe.
+ * Allocates a qd_connection_t object and therefore
+ * takes ENTITY_CACHE lock.
+ */
+qd_connection_t *qd_server_connection(qd_server_t *server, qd_server_config_t
*config)
+{
+ qd_connection_t *ctx = new_qd_connection_t();
+ if (!ctx) return NULL;
+ return qd_server_connection_impl(server, config, ctx);
+}
static void on_accept(pn_event_t *e, qd_listener_t *listener)
{
@@ -1156,11 +1168,10 @@ static qd_failover_item_t
*qd_connector_get_conn_info(qd_connector_t *ct) {
/* Timer callback to try/retry connection open */
-static void try_open_lh(qd_connector_t *connector)
+static void try_open_lh(qd_connector_t *connector, qd_connection_t *connection)
{
- // Allocate a new connection. No other thread will touch this
// connection until pn_proactor_connect is called below
- qd_connection_t *qd_conn = qd_server_connection(connector->server,
&connector->config);
+ qd_connection_t *qd_conn = qd_server_connection_impl(connector->server,
&connector->config, connection);
if (!qd_conn) { /* Try again later */
qd_log(connector->server->log_source, QD_LOG_CRITICAL, "Allocation
failure connecting to %s",
connector->config.host_port);
@@ -1323,12 +1334,24 @@ static void try_open_cb(void *context)
{
qd_connector_t *ct = (qd_connector_t*) context;
- sys_mutex_lock(ct->lock); /* TODO aconway 2017-05-09: this lock looks
too big */
+ // Allocate connection before taking connector lock to avoid
+ // CONNECTOR - ENTITY_CACHE lock inversion deadlock window.
+ qd_connection_t *ctx = new_qd_connection_t();
+ if (!ctx) {
+ qd_log(ct->server->log_source, QD_LOG_CRITICAL, "Allocation failure
connecting to %s",
+ ct->config.host_port);
+ ct->delay = 10000;
+ ct->state = CXTR_STATE_CONNECTING;
+ qd_timer_schedule(ct->timer, ct->delay);
+ return;
+ }
+
+ sys_mutex_lock(ct->lock);
if (ct->state == CXTR_STATE_CONNECTING || ct->state == CXTR_STATE_INIT) {
// else deleted or failed - on failed wait until after connection is
freed
// and state is set to CXTR_STATE_CONNECTING (timer is rescheduled
then)
- try_open_lh(ct);
+ try_open_lh(ct, ctx);
}
sys_mutex_unlock(ct->lock);
@@ -1584,7 +1607,16 @@ void qd_connection_invoke_deferred(qd_connection_t
*conn, qd_deferred_t call, vo
if (!conn)
return;
- qd_deferred_call_t *dc = new_qd_deferred_call_t();
+ qd_connection_invoke_deferred_impl(conn, call, context,
new_qd_deferred_call_t());
+}
+
+
+void qd_connection_invoke_deferred_impl(qd_connection_t *conn, qd_deferred_t
call, void *context, void *dct)
+{
+ if (!conn)
+ return;
+
+ qd_deferred_call_t *dc = (qd_deferred_call_t*)dct;
DEQ_ITEM_INIT(dc);
dc->call = call;
dc->context = context;
@@ -1599,6 +1631,18 @@ void qd_connection_invoke_deferred(qd_connection_t
*conn, qd_deferred_t call, vo
}
+void *qd_connection_new_qd_deferred_call_t()
+{
+ return new_qd_deferred_call_t();
+}
+
+
+void qd_connection_free_qd_deferred_call_t(void *dct)
+{
+ free_qd_deferred_call_t((qd_deferred_call_t *)dct);
+}
+
+
qd_listener_t *qd_server_listener(qd_server_t *server)
{
qd_listener_t *li = new_qd_listener_t();
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org
For additional commands, e-mail: commits-h...@qpid.apache.org
