From c8f37cb4d698129c12302aaa3f46d468e506a417 Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Thu, 30 Nov 2017 13:43:11 +0100 Subject: [PATCH] mgcp_client: add transaction cleanup So far, if an MGCP message is sent, the transaction gets enqueued, but there is no way to end the transaction other than receiving a valid reply. So, if the caller decides that the transaction timed out and tears down the priv pointer passed to mgcp_client_tx, and if then a late reply arrives, the callback will dereference the invalid priv pointer and cause a segfault. Hence it is possible to crash an mgcp_client program by sending a late response. Furthermore, if no reply ever arrives, we would keep the pending response in the list forever, amounting to a "memory leak". Add mgcp_client_cancel() to discard a pending transaction. The caller can now decide to discard a pending response when it sees fit (e.g. the caller's timeout expired). This needs to be added to OsmoMSC and OsmoBSC. Add mgcp_msg_trans_id() to provide an obvious way to obtain the transaction id from a generated MGCP message. No public API is broken; but refine the negative return code from mgcp_client_rx(): return -ENOENT if no such transaction ID is found, and still -1 if decoding failed. This is mainly for mgcp_client_test. Implement a test for mgcp_client_cancel() in mgcp_client_test.c. Tweak internal mgcp_client_response_pending_get() to take only the transaction id as argument instead of the entire mgcp message struct. Found-by: dexter Related: OS#2695 OS#2696 Change-Id: I16811e168a46a82a05943252a737b3434143f4bd --- include/osmocom/mgcp_client/mgcp_client.h | 2 + src/libosmo-mgcp-client/mgcp_client.c | 47 +++++++++++++++++--- tests/mgcp_client/mgcp_client_test.c | 53 ++++++++++++++++++++++- tests/mgcp_client/mgcp_client_test.err | 13 ++++++ tests/mgcp_client/mgcp_client_test.ok | 19 ++++++++ 5 files changed, 125 insertions(+), 9 deletions(-) diff --git a/include/osmocom/mgcp_client/mgcp_client.h b/include/osmocom/mgcp_client/mgcp_client.h index cf5e8c49d..4caf656a6 100644 --- a/include/osmocom/mgcp_client/mgcp_client.h +++ b/include/osmocom/mgcp_client/mgcp_client.h @@ -93,6 +93,7 @@ int mgcp_response_parse_params(struct mgcp_response *r); int mgcp_client_tx(struct mgcp_client *mgcp, struct msgb *msg, mgcp_response_cb_t response_cb, void *priv); +int mgcp_client_cancel(struct mgcp_client *mgcp, mgcp_trans_id_t trans_id); enum mgcp_connection_mode; @@ -111,6 +112,7 @@ struct msgb *mgcp_msg_dlcx(struct mgcp_client *mgcp, uint16_t rtp_endpoint, OSMO_DEPRECATED("Use mgcp_msg_gen() instead"); struct msgb *mgcp_msg_gen(struct mgcp_client *mgcp, struct mgcp_msg *mgcp_msg); +mgcp_trans_id_t mgcp_msg_trans_id(struct msgb *msg); extern const struct value_string mgcp_client_connection_mode_strs[]; static inline const char *mgcp_client_cmode_name(enum mgcp_connection_mode mode) diff --git a/src/libosmo-mgcp-client/mgcp_client.c b/src/libosmo-mgcp-client/mgcp_client.c index ee8d2675d..2f3d0d120 100644 --- a/src/libosmo-mgcp-client/mgcp_client.c +++ b/src/libosmo-mgcp-client/mgcp_client.c @@ -311,13 +311,11 @@ exit: static struct mgcp_response_pending *mgcp_client_response_pending_get( struct mgcp_client *mgcp, - struct mgcp_response *r) + mgcp_trans_id_t trans_id) { struct mgcp_response_pending *pending; - if (!r) - return NULL; llist_for_each_entry(pending, &mgcp->responses_pending, entry) { - if (pending->trans_id == r->head.trans_id) { + if (pending->trans_id == trans_id) { llist_del(&pending->entry); return pending; } @@ -349,12 +347,12 @@ int mgcp_client_rx(struct mgcp_client *mgcp, struct msgb *msg) return -1; } - pending = mgcp_client_response_pending_get(mgcp, &r); + pending = mgcp_client_response_pending_get(mgcp, r.head.trans_id); if (!pending) { LOGP(DLMGCP, LOGL_ERROR, "Cannot find matching MGCP transaction for trans_id %d\n", r.head.trans_id); - return -1; + return -ENOENT; } mgcp_client_handle_response(mgcp, pending, &r); @@ -560,7 +558,10 @@ struct mgcp_response_pending * mgcp_client_pending_add( * response_cb. NOTE: the response_cb still needs to call * mgcp_response_parse_params(response) to get the parsed parameters -- to * potentially save some CPU cycles, only the head line has been parsed when - * the response_cb is invoked. */ + * the response_cb is invoked. + * Before the priv pointer becomes invalid, e.g. due to transaction timeout, + * mgcp_client_cancel() needs to be called for this transaction. + */ int mgcp_client_tx(struct mgcp_client *mgcp, struct msgb *msg, mgcp_response_cb_t response_cb, void *priv) { @@ -603,6 +604,32 @@ mgcp_tx_error: return -1; } +/* Cancel a pending transaction. + * Should a priv pointer passed to mgcp_client_tx() become invalid, this function must be called. In + * practical terms, if the caller of mgcp_client_tx() wishes to tear down a transaction without having + * received a response this function must be called. The trans_id can be obtained by calling + * mgcp_msg_trans_id() on the msgb produced by mgcp_msg_gen(). + */ +int mgcp_client_cancel(struct mgcp_client *mgcp, mgcp_trans_id_t trans_id) +{ + struct mgcp_response_pending *pending = mgcp_client_response_pending_get(mgcp, trans_id); + if (!pending) { + /* INFO is sufficient, it is not harmful to cancel a transaction twice. */ + LOGP(DLMGCP, LOGL_INFO, "Cannot cancel, no such transaction: %u\n", trans_id); + return -ENOENT; + } + LOGP(DLMGCP, LOGL_INFO, "Canceled transaction %u\n", trans_id); + talloc_free(pending); + return 0; + /* We don't really need to clean up the wqueue: In all sane cases, the msgb has already been sent + * out and is no longer in the wqueue. If it still is in the wqueue, then sending MGCP messages + * per se is broken and the program should notice so by a full wqueue. Even if this was called + * before we had a chance to send out the message and it is still going to be sent, we will just + * ignore the reply to it later. Removing a msgb from the wqueue here would just introduce more + * bug surface in terms of failing to update wqueue API's counters or some such. + */ +} + static struct msgb *mgcp_msg_from_buf(mgcp_trans_id_t trans_id, const char *buf, int len) { @@ -806,6 +833,12 @@ struct msgb *mgcp_msg_gen(struct mgcp_client *mgcp, struct mgcp_msg *mgcp_msg) return msg; } +/* Retrieve the MGCP transaction ID from a msgb generated by mgcp_msg_gen() */ +mgcp_trans_id_t mgcp_msg_trans_id(struct msgb *msg) +{ + return (mgcp_trans_id_t)msg->cb[MSGB_CB_MGCP_TRANS_ID]; +} + struct mgcp_client_conf *mgcp_client_conf_actual(struct mgcp_client *mgcp) { return &mgcp->actual; diff --git a/tests/mgcp_client/mgcp_client_test.c b/tests/mgcp_client/mgcp_client_test.c index 37fe0b832..172faac8e 100644 --- a/tests/mgcp_client/mgcp_client_test.c +++ b/tests/mgcp_client/mgcp_client_test.c @@ -24,6 +24,7 @@ #include #include #include +#include void *ctx; @@ -73,7 +74,7 @@ static struct msgb *from_str(const char *str) static struct mgcp_client_conf conf; struct mgcp_client *mgcp = NULL; -static void reply_to(mgcp_trans_id_t trans_id, int code, const char *comment, +static int reply_to(mgcp_trans_id_t trans_id, int code, const char *comment, int conn_id, const char *params) { static char compose[4096 - 128]; @@ -87,7 +88,7 @@ static void reply_to(mgcp_trans_id_t trans_id, int code, const char *comment, printf("composed response:\n-----\n%s\n-----\n", compose); - mgcp_client_rx(mgcp, from_str(compose)); + return mgcp_client_rx(mgcp, from_str(compose)); } void test_response_cb(struct mgcp_response *response, void *priv) @@ -225,6 +226,51 @@ void test_mgcp_msg(void) msgb_free(msg); } +void test_mgcp_client_cancel() +{ + mgcp_trans_id_t trans_id; + struct msgb *msg; + struct mgcp_msg mgcp_msg = { + .verb = MGCP_VERB_CRCX, + .audio_ip = "192.168.100.23", + .endpoint = "23@mgw", + .audio_port = 1234, + .call_id = 47, + .conn_id = 11, + .conn_mode = MGCP_CONN_RECV_SEND, + .presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID + | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE), + }; + + printf("\n%s():\n", __func__); + fprintf(stderr, "\n%s():\n", __func__); + + if (mgcp) + talloc_free(mgcp); + mgcp = mgcp_client_init(ctx, &conf); + + msg = mgcp_msg_gen(mgcp, &mgcp_msg); + trans_id = mgcp_msg_trans_id(msg); + fprintf(stderr, "- composed msg with trans_id=%u\n", trans_id); + + fprintf(stderr, "- not in queue yet, cannot cancel yet\n"); + OSMO_ASSERT(mgcp_client_cancel(mgcp, trans_id) == -ENOENT); + + fprintf(stderr, "- enqueue\n"); + dummy_mgcp_send(msg); + + fprintf(stderr, "- cancel succeeds\n"); + OSMO_ASSERT(mgcp_client_cancel(mgcp, trans_id) == 0); + + fprintf(stderr, "- late response gets discarded\n"); + OSMO_ASSERT(reply_to(trans_id, 200, "OK", 1, "v=0\r\n") == -ENOENT); + + fprintf(stderr, "- canceling again does nothing\n"); + OSMO_ASSERT(mgcp_client_cancel(mgcp, trans_id) == -ENOENT); + + fprintf(stderr, "%s() done\n", __func__); +} + static const struct log_info_cat log_categories[] = { }; @@ -244,10 +290,13 @@ int main(int argc, char **argv) log_set_use_color(osmo_stderr_target, 0); log_set_print_category(osmo_stderr_target, 1); + log_set_category_filter(osmo_stderr_target, DLMGCP, 1, LOGL_DEBUG); + mgcp_client_conf_init(&conf); test_crcx(); test_mgcp_msg(); + test_mgcp_client_cancel(); printf("Done\n"); fprintf(stderr, "Done\n"); diff --git a/tests/mgcp_client/mgcp_client_test.err b/tests/mgcp_client/mgcp_client_test.err index 24151eeda..8e9f648ef 100644 --- a/tests/mgcp_client/mgcp_client_test.err +++ b/tests/mgcp_client/mgcp_client_test.err @@ -1,2 +1,15 @@ DLMGCP message buffer to small, can not generate MGCP message + +test_mgcp_client_cancel(): +- composed msg with trans_id=1 +- not in queue yet, cannot cancel yet +DLMGCP Cannot cancel, no such transaction: 1 +- enqueue +- cancel succeeds +DLMGCP Canceled transaction 1 +- late response gets discarded +DLMGCP Cannot find matching MGCP transaction for trans_id 1 +- canceling again does nothing +DLMGCP Cannot cancel, no such transaction: 1 +test_mgcp_client_cancel() done Done diff --git a/tests/mgcp_client/mgcp_client_test.ok b/tests/mgcp_client/mgcp_client_test.ok index d4efee4cc..4039bb037 100644 --- a/tests/mgcp_client/mgcp_client_test.ok +++ b/tests/mgcp_client/mgcp_client_test.ok @@ -59,4 +59,23 @@ RSIP 5 23@mgw MGCP 1.0 Overfolow test: + +test_mgcp_client_cancel(): +composed: +----- +CRCX 1 23@mgw MGCP 1.0 +C: 2f +I: 11 +L: p:20, a:AMR, nt:IN +M: sendrecv + +----- +composed response: +----- +200 1 OK +I: 1 + +v=0 + +----- Done