[PATCH] osmo-mgw[master]: mgcp_client: add transaction cleanup

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Thu Nov 30 13:02:23 UTC 2017


Review at  https://gerrit.osmocom.org/5110

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.

Found-by: dexter
Change-Id: I16811e168a46a82a05943252a737b3434143f4bd
---
M include/osmocom/mgcp_client/mgcp_client.h
M src/libosmo-mgcp-client/mgcp_client.c
M tests/mgcp_client/mgcp_client_test.c
M tests/mgcp_client/mgcp_client_test.err
M tests/mgcp_client/mgcp_client_test.ok
5 files changed, 124 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/10/5110/1

diff --git a/include/osmocom/mgcp_client/mgcp_client.h b/include/osmocom/mgcp_client/mgcp_client.h
index 1a6cbce..f404131 100644
--- a/include/osmocom/mgcp_client/mgcp_client.h
+++ b/include/osmocom/mgcp_client/mgcp_client.h
@@ -92,6 +92,7 @@
 
 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;
 
@@ -110,6 +111,7 @@
 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 2047637..96ad359 100644
--- a/src/libosmo-mgcp-client/mgcp_client.c
+++ b/src/libosmo-mgcp-client/mgcp_client.c
@@ -260,13 +260,11 @@
 
 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;
 		}
@@ -292,12 +290,12 @@
 		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);
@@ -503,7 +501,10 @@
  * 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)
 {
@@ -544,6 +545,31 @@
 	/* Pass NULL to response cb to indicate an error */
 	mgcp_client_handle_response(mgcp, pending, NULL);
 	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) {
+		LOGP(DLMGCP, LOGL_ERROR, "Cannot cancel, no such transaction: %u\n", trans_id);
+		return -ENOENT;
+	}
+	LOGP(DLMGCP, LOGL_ERROR, "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,
@@ -751,6 +777,12 @@
 	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 513f345..0de449a 100644
--- a/tests/mgcp_client/mgcp_client_test.c
+++ b/tests/mgcp_client/mgcp_client_test.c
@@ -24,6 +24,7 @@
 #include <osmocom/core/application.h>
 #include <osmocom/mgcp_client/mgcp_client.h>
 #include <osmocom/mgcp_client/mgcp_client_internal.h>
+#include <errno.h>
 
 void *ctx;
 
@@ -73,7 +74,7 @@
 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 @@
 
 	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 @@
 	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 at 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 @@
 	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 24151ee..8e9f648 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 d4efee4..4039bb0 100644
--- a/tests/mgcp_client/mgcp_client_test.ok
+++ b/tests/mgcp_client/mgcp_client_test.ok
@@ -59,4 +59,23 @@
 
 Overfolow test:
 
+
+test_mgcp_client_cancel():
+composed:
+-----
+CRCX 1 23 at 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

-- 
To view, visit https://gerrit.osmocom.org/5110
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I16811e168a46a82a05943252a737b3434143f4bd
Gerrit-PatchSet: 1
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>



More information about the gerrit-log mailing list