You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ac...@apache.org on 2017/04/27 17:38:35 UTC
[10/10] qpid-dispatch git commit: DISPATCH-739: Fix double free bug,
remove _to_free_ lists
DISPATCH-739: Fix double free bug, remove _to_free_ lists
Don't defer freeing sessions/links, just ensure they are freed exactly once.
Project: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/commit/b974b884
Tree: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/tree/b974b884
Diff: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/diff/b974b884
Branch: refs/heads/master
Commit: b974b884a7f9ac7ba2d13508af21d779d4f84beb
Parents: 16980f6
Author: Alan Conway <ac...@redhat.com>
Authored: Tue Apr 25 09:42:17 2017 -0400
Committer: Alan Conway <ac...@redhat.com>
Committed: Thu Apr 27 13:32:36 2017 -0400
----------------------------------------------------------------------
src/container.c | 103 ++++------------------------------------------
src/server.c | 1 -
src/server_private.h | 3 --
3 files changed, 7 insertions(+), 100 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/b974b884/src/container.c
----------------------------------------------------------------------
diff --git a/src/container.c b/src/container.c
index 639ffa2..3add3ac 100644
--- a/src/container.c
+++ b/src/container.c
@@ -84,8 +84,6 @@ struct qd_container_t {
qdc_node_type_list_t node_type_list;
};
-ALLOC_DEFINE(qd_pn_free_link_session_t);
-
static void setup_outgoing_link(qd_container_t *container, pn_link_t *pn_link)
{
qd_node_t *node = container->default_node;
@@ -321,92 +319,6 @@ static void writable_handler(qd_container_t *container, pn_connection_t *conn, q
}
}
-/**
- * Returns true if the free_link already exists in free_link_list, false otherwise
- */
-static bool link_exists(qd_pn_free_link_session_list_t **free_list, pn_link_t *free_link)
-{
- qd_pn_free_link_session_t *free_item = DEQ_HEAD(**free_list);
- while(free_item) {
- if (free_item->pn_link == free_link)
- return true;
- free_item = DEQ_NEXT(free_item);
- }
- return false;
-}
-
-/**
- * Returns true if the free_session already exists in free_session_list, false otherwise
- */
-static bool session_exists(qd_pn_free_link_session_list_t **free_list, pn_session_t *free_session)
-{
- qd_pn_free_link_session_t *free_item = DEQ_HEAD(**free_list);
- while(free_item) {
- if (free_item->pn_session == free_session)
- return true;
- free_item = DEQ_NEXT(free_item);
- }
- return false;
-}
-
-static void add_session_to_free_list(qd_pn_free_link_session_list_t *free_link_session_list, pn_session_t *ssn)
-{
- if (!session_exists(&free_link_session_list, ssn)) {
- qd_pn_free_link_session_t *to_free = new_qd_pn_free_link_session_t();
- DEQ_ITEM_INIT(to_free);
- to_free->pn_session = ssn;
- to_free->pn_link = 0;
- DEQ_INSERT_TAIL(*free_link_session_list, to_free);
- }
-}
-
-
-static void add_link_to_free_list(qd_pn_free_link_session_list_t *free_link_session_list, pn_link_t *pn_link)
-{
- if (!link_exists(&free_link_session_list, pn_link)) {
- qd_pn_free_link_session_t *to_free = new_qd_pn_free_link_session_t();
- DEQ_ITEM_INIT(to_free);
- to_free->pn_link = pn_link;
- to_free->pn_session = 0;
- DEQ_INSERT_TAIL(*free_link_session_list, to_free);
- }
-
-}
-
-
-/*
- * TODO aconway 2017-04-12: IMO this should not be necessary, we should
- * be able to pn_*_free links and sessions directly the handler function.
- * They will not actually be freed from memory till the event, connection,
- * proactor etc. have all released their references.
- *
- * The need for these lists may indicate a router bug, where the router is
- * using links/sessions after they are freed. Investigate and simplify if
- * possible.
- */
-static void conn_event_complete(qd_connection_t *qd_conn)
-{
- qd_pn_free_link_session_t *to_free_link = DEQ_HEAD(qd_conn->free_link_session_list);
- qd_pn_free_link_session_t *to_free_session = DEQ_HEAD(qd_conn->free_link_session_list);
- while(to_free_link) {
- if (to_free_link->pn_link) {
- pn_link_free(to_free_link->pn_link);
- to_free_link->pn_link = 0;
- }
- to_free_link = DEQ_NEXT(to_free_link);
- }
-
- while(to_free_session) {
- if (to_free_session->pn_session) {
- pn_session_free(to_free_session->pn_session);
- to_free_session->pn_session = 0;
- }
- DEQ_REMOVE_HEAD(qd_conn->free_link_session_list);
- free_qd_pn_free_link_session_t(to_free_session);
- to_free_session = DEQ_HEAD(qd_conn->free_link_session_list);
- }
-}
-
void qd_container_handle_event(qd_container_t *container, pn_event_t *event)
{
@@ -475,7 +387,7 @@ void qd_container_handle_event(qd_container_t *container, pn_event_t *event)
}
if (pn_session_state(ssn) == (PN_LOCAL_CLOSED | PN_REMOTE_CLOSED)) {
- add_session_to_free_list(&qd_conn->free_link_session_list,ssn);
+ pn_session_free(ssn);
}
break;
@@ -516,7 +428,7 @@ void qd_container_handle_event(qd_container_t *container, pn_event_t *event)
pn_session_close(ssn);
}
else if (pn_session_state(ssn) == (PN_LOCAL_CLOSED | PN_REMOTE_CLOSED)) {
- add_session_to_free_list(&qd_conn->free_link_session_list,ssn);
+ pn_session_free(ssn);
}
}
break;
@@ -578,7 +490,8 @@ void qd_container_handle_event(qd_container_t *container, pn_event_t *event)
if (pn_link_state(pn_link) & PN_LOCAL_CLOSED) {
if (qd_link->close_sess_with_link && sess)
pn_session_close(sess);
- add_link_to_free_list(&qd_conn->free_link_session_list, pn_link);
+ pn_link_set_context(pn_link, NULL);
+ pn_link_free(pn_link);
}
if (node) {
node->ntype->link_detach_handler(node->context, qd_link, dt);
@@ -590,8 +503,9 @@ void qd_container_handle_event(qd_container_t *container, pn_event_t *event)
case PN_LINK_LOCAL_DETACH:
case PN_LINK_LOCAL_CLOSE:
pn_link = pn_event_link(event);
- if (pn_link_state(pn_link) == (PN_LOCAL_CLOSED | PN_REMOTE_CLOSED)) {
- add_link_to_free_list(&qd_conn->free_link_session_list, pn_link);
+ if (pn_link_state(pn_link) == (PN_LOCAL_CLOSED | PN_REMOTE_CLOSED) && pn_link_get_context(pn_link)) {
+ pn_link_set_context(pn_link, NULL);
+ pn_link_free(pn_link);
}
break;
@@ -625,9 +539,6 @@ void qd_container_handle_event(qd_container_t *container, pn_event_t *event)
default:
break;
}
- if (qd_conn) {
- conn_event_complete(qd_conn);
- }
}
http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/b974b884/src/server.c
----------------------------------------------------------------------
diff --git a/src/server.c b/src/server.c
index 066c862..52651d1 100644
--- a/src/server.c
+++ b/src/server.c
@@ -500,7 +500,6 @@ qd_connection_t *qd_server_connection(qd_server_t *server, qd_server_config_t *c
pn_connection_set_context(ctx->pn_conn, ctx);
DEQ_ITEM_INIT(ctx);
DEQ_INIT(ctx->deferred_calls);
- DEQ_INIT(ctx->free_link_session_list);
sys_mutex_lock(server->lock);
ctx->connection_id = server->next_connection_id++;
sys_mutex_unlock(server->lock);
http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/b974b884/src/server_private.h
----------------------------------------------------------------------
diff --git a/src/server_private.h b/src/server_private.h
index 6e6f1c3..77adf32 100644
--- a/src/server_private.h
+++ b/src/server_private.h
@@ -154,7 +154,6 @@ struct qd_connection_t {
sys_mutex_t *deferred_call_lock;
bool policy_counted;
char *role; //The specified role of the connection, e.g. "normal", "inter-router", "route-container" etc.
- qd_pn_free_link_session_list_t free_link_session_list;
void (*wake)(qd_connection_t*); /* Wake method, different for HTTP vs. proactor */
char rhost[NI_MAXHOST]; /* Remote host numeric IP for incoming connections */
char rhost_port[NI_MAXHOST+NI_MAXSERV]; /* Remote host:port for incoming connections */
@@ -166,7 +165,5 @@ ALLOC_DECLARE(qd_listener_t);
ALLOC_DECLARE(qd_deferred_call_t);
ALLOC_DECLARE(qd_connector_t);
ALLOC_DECLARE(qd_connection_t);
-ALLOC_DECLARE(qd_pn_free_link_session_t);
-
#endif
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org