Change in osmo-msc[master]: libmsc: fix memory leak (struct msgb) in msc_i_ran_enc()

[fixeria]

fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-msc/+/26460 )


Change subject: libmsc: fix memory leak (struct msgb) in msc_i_ran_enc()
......................................................................

libmsc: fix memory leak (struct msgb) in msc_i_ran_enc()

Function msc_i_ran_enc() calls msc_role_ran_encode(), but unlike the
other callers of this function it does not free() the encoded message.

A simple solution would be to call msgb_free(), like it's done in
the other places.  But a more elegant solution is to modify function
msc_role_ran_encode(), so that it attaches the msgb to OTC_SELECT.
This way there is no need to call msgb_free() here and there.

This change fixes a memleak observed while running ttcn3-msc-test.

Change-Id: I741e082badc32ba9a97c1495c894e1d22e122e3a
Related: OS#5340
---
M src/libmsc/msc_a.c
M src/libmsc/msc_a_remote.c
M src/libmsc/msc_t.c
M src/libmsc/msub.c
4 files changed, 7 insertions(+), 14 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/60/26460/1

diff --git a/src/libmsc/msc_a.c b/src/libmsc/msc_a.c
index 74721d2..c9b0572 100644
--- a/src/libmsc/msc_a.c
+++ b/src/libmsc/msc_a.c
@@ -1659,12 +1659,9 @@
 		.an_proto = msc_a->c.ran->an_proto,
 		.msg = msc_role_ran_encode(msc_a->c.fi, ran_msg),
 	};
-	int rc;
 	if (!an_apdu.msg)
 		return -EIO;
-	rc = _msub_role_dispatch(msc_a->c.msub, to_role, to_role_event, &an_apdu, file, line);
-	msgb_free(an_apdu.msg);
-	return rc;
+	return _msub_role_dispatch(msc_a->c.msub, to_role, to_role_event, &an_apdu, file, line);
 }
 
 int msc_a_tx_dtap_to_i(struct msc_a *msc_a, struct msgb *dtap)
diff --git a/src/libmsc/msc_a_remote.c b/src/libmsc/msc_a_remote.c
index 84eff07..e4474f4 100644
--- a/src/libmsc/msc_a_remote.c
+++ b/src/libmsc/msc_a_remote.c
@@ -179,8 +179,6 @@
 		return;
 
 	msc_a_remote_msg_up_to_remote_msc(msc_a, MSC_ROLE_T, OSMO_GSUP_MSGT_E_PREPARE_HANDOVER_ERROR, &an_apdu);
-	msgb_free(an_apdu.msg);
-	return;
 }
 
 /*     [MSC-A---------------------]            [MSC-B---------------------]
diff --git a/src/libmsc/msc_t.c b/src/libmsc/msc_t.c
index af0ddaa..43bc74e 100644
--- a/src/libmsc/msc_t.c
+++ b/src/libmsc/msc_t.c
@@ -145,7 +145,6 @@
 		return;
 
 	msub_role_dispatch(msc_t->c.msub, MSC_ROLE_A, MSC_A_EV_FROM_T_PREPARE_HANDOVER_FAILURE, &an_apdu);
-	msgb_free(an_apdu.msg);
 }
 
 static int msc_t_ho_request_decode_and_store_cb(struct osmo_fsm_inst *msc_t_fi, void *data,
@@ -238,7 +237,6 @@
 static int msc_t_send_stored_ho_request__decode_cb(struct osmo_fsm_inst *msc_t_fi, void *data,
 						  const struct ran_msg *ran_dec)
 {
-	int rc;
 	struct an_apdu an_apdu;
 	struct msc_t *msc_t = msc_t_priv(msc_t_fi);
 	struct osmo_sockaddr_str *rtp_ran_local = data;
@@ -263,9 +261,7 @@
 	};
 	if (!an_apdu.msg)
 		return -EIO;
-	rc = msc_t_down_l2_co(msc_t, &an_apdu, true);
-	msgb_free(an_apdu.msg);
-	return rc;
+	return msc_t_down_l2_co(msc_t, &an_apdu, true);
 }
 
 /* The MGW endpoint is created, we know our AoIP Transport Layer Address and can send the Handover Request to the RAN
@@ -472,9 +468,7 @@
 	if (!an_apdu.msg)
 		return -EIO;
 	/* Send to remote MSC via msc_a_remote role */
-	rc = msub_role_dispatch(msc_t->c.msub, MSC_ROLE_A, MSC_A_EV_FROM_T_PREPARE_HANDOVER_RESPONSE, &an_apdu);
-	msgb_free(an_apdu.msg);
-	return rc;
+	return msub_role_dispatch(msc_t->c.msub, MSC_ROLE_A, MSC_A_EV_FROM_T_PREPARE_HANDOVER_RESPONSE, &an_apdu);
 }
 
 static int msc_t_wait_ho_request_ack_decode_cb(struct osmo_fsm_inst *msc_t_fi, void *data,
diff --git a/src/libmsc/msub.c b/src/libmsc/msub.c
index 112703a..e4dd332 100644
--- a/src/libmsc/msub.c
+++ b/src/libmsc/msub.c
@@ -544,6 +544,8 @@
 	*conn_p = NULL;
 }
 
+/* NOTE: the resulting message buffer will be attached to OTC_SELECT, so its lifetime
+ * is limited by the current select() loop iteration.  Use talloc_steal() to avoid this. */
 struct msgb *msc_role_ran_encode(struct osmo_fsm_inst *fi, const struct ran_msg *ran_msg)
 {
 	struct msc_role_common *c = fi->priv;
@@ -556,6 +558,8 @@
 	msg = c->ran->ran_encode(fi, ran_msg);
 	if (!msg)
 		LOGPFSML(fi, LOGL_ERROR, "Failed to encode %s\n", ran_msg_type_name(ran_msg->msg_type));
+	else
+		talloc_steal(OTC_SELECT, msg);
 	return msg;
 }
 

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/26460
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I741e082badc32ba9a97c1495c894e1d22e122e3a
Gerrit-Change-Number: 26460
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211207/6d652c0d/attachment.htm>