Fixes #1698: prevent use-after-free of qd_link_t on session close #1699
Conversation
| @@ -645,6 +627,7 @@ void qd_container_handle_event(qd_container_t *container, pn_event_t *event, | |||
| 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_list, pn_link); | |||
| qd_link_free((qd_link_t *) pn_link_get_context(pn_link)); | |||
There was a problem hiding this comment.
Instead of calling qd_link_free() once in the handler to PN_LINK_REMOTE_CLOSE and once in PN_LINK_LOCAL_CLOSE, can we call it just once in the qd_conn_event_batch_complete() function where we loop thru the qd_conn->free_link_list ?
In fact, I was wondering if we could have the qd_conn_event_batch_complete() function as the only place we call qd_link_free() and pn_link_free()
There was a problem hiding this comment.
In fact, I was wondering if we could have the
qd_conn_event_batch_complete()function as the only place we callqd_link_free()andpn_link_free()
I realized that the qd_conn_event_batch_complete() function is called only on connection close, so it cannot be the only place where the qd_link_free() and pn_link_free() functions are called. So, please disregard that comment. I assumed that qd_conn_event_batch_complete() is called after every event batch. My bad.
| pn_session_close(ssn); | ||
| } | ||
| else if (pn_session_state(ssn) == (PN_LOCAL_CLOSED | PN_REMOTE_CLOSED)) { | ||
| qd_session_t *qd_ssn = qd_session_from_pn(ssn); | ||
| if (qd_ssn) { | ||
| if (qd_ssn->policy_counted) { |
There was a problem hiding this comment.
Can we please move this policy_counted related code to a new commit so this commit only contains the code that fixes the router crash ?
There was a problem hiding this comment.
I had to add that hacky policy flag because otherwise this patch would break the policy code. The problem is that not all sessions or links are counted by policy: only those that are 1) remotely initiated and 2) policy is in use.
That means the code cannot unconditionally decrement the relevant policy counters since the counters may not have been incremented for that particular session/link (for reasons 1 and 2 above). So I have to add that ugly "hey I'm counted by policy so remember to decrement when I'm done" hack.
Unconditionally incrementing/decrementing the counters will break the policy logic: there's even asserts in the (existing) code to guard against underflow:
skupper-router/src/adaptors/amqp/container.c
Line 551 in 2249924
There was a problem hiding this comment.
That means the code cannot unconditionally decrement the relevant policy counters since the counters may not have been incremented for that particular session/link
Makes sense to me.
…sion close Previously the detach handler might free the qd_link_t but it was not consistent. This patch moves the qd_link_t cleanup to the container.
Previously the detach handler might free the qd_link_t but it was not consistent. This patch moves the qd_link_t cleanup to the container.