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 <[email protected]> Authored: Tue Apr 25 09:42:17 2017 -0400 Committer: Alan Conway <[email protected]> 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: [email protected] For additional commands, e-mail: [email protected]
