Change in osmo-mgw[master]: Revert "mgcp_ratectr: add stats items to monitor trunk usage"

neels gerrit-no-reply at lists.osmocom.org
Wed Jul 21 17:07:14 UTC 2021


neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/24982 )

Change subject: Revert "mgcp_ratectr: add stats items to monitor trunk usage"
......................................................................

Revert "mgcp_ratectr: add stats items to monitor trunk usage"

This reverts commit 6bad138c96ef0e2a93ef7de42e897880131c0b43.

Reason for revert: heap-use-after-free during 'make check'
in mgcp_test.c test_retransmission()

Change-Id: I96792a719c9c7273676ab9ffe0b9e2aae4c23166
Related: OS#5201
---
M include/osmocom/mgcp/mgcp_ratectr.h
M include/osmocom/mgcp/mgcp_trunk.h
M src/libosmo-mgcp/mgcp_endp.c
M src/libosmo-mgcp/mgcp_ratectr.c
M src/libosmo-mgcp/mgcp_trunk.c
M tests/mgcp/mgcp_test.c
6 files changed, 21 insertions(+), 94 deletions(-)

Approvals:
  neels: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/mgcp/mgcp_ratectr.h b/include/osmocom/mgcp/mgcp_ratectr.h
index 0bd6f88..78c687b 100644
--- a/include/osmocom/mgcp/mgcp_ratectr.h
+++ b/include/osmocom/mgcp/mgcp_ratectr.h
@@ -95,17 +95,3 @@
 
 int mgcp_ratectr_global_alloc(struct mgcp_config *cfg);
 int mgcp_ratectr_trunk_alloc(struct mgcp_trunk *trunk);
-
-/* Trunk-global common stat items */
-enum {
-	TRUNK_STAT_ENDPOINTS_TOTAL,
-	TRUNK_STAT_ENDPOINTS_USED,
-};
-
-struct mgcp_stat_trunk {
-	/* Stat item group which contains general status values of the trunk. */
-	struct osmo_stat_item_group *common;
-};
-
-int mgcp_stat_trunk_alloc(struct mgcp_trunk *trunk);
-
diff --git a/include/osmocom/mgcp/mgcp_trunk.h b/include/osmocom/mgcp/mgcp_trunk.h
index d960428..048ac5b 100644
--- a/include/osmocom/mgcp/mgcp_trunk.h
+++ b/include/osmocom/mgcp/mgcp_trunk.h
@@ -52,9 +52,8 @@
 	unsigned int number_endpoints;
 	struct mgcp_endpoint **endpoints;
 
-	/* rate counters and stat items to measure the trunks overall performance and health */
+	/* global rate counters to measure the trunks overall performance and health */
 	struct mgcp_ratectr_trunk ratectr;
-	struct mgcp_stat_trunk stats;
 
 	union {
 		/* Virtual trunk specific */
diff --git a/src/libosmo-mgcp/mgcp_endp.c b/src/libosmo-mgcp/mgcp_endp.c
index 7df4a80..ae376b0 100644
--- a/src/libosmo-mgcp/mgcp_endp.c
+++ b/src/libosmo-mgcp/mgcp_endp.c
@@ -29,7 +29,6 @@
 
 #include <osmocom/abis/e1_input.h>
 #include <osmocom/mgcp/mgcp_e1.h>
-#include <osmocom/core/stat_item.h>
 
 #define E1_RATE_MAX 64
 #define E1_OFFS_MAX 8
@@ -123,12 +122,6 @@
 	 * RSIP is executed), free them all at once. */
 	mgcp_conn_free_all(endp);
 
-	/* We must only decrement the stat item when the endpoint as actually
-	 * claimed. An endpoint is claimed when a call-id is set */
-	if (endp->callid)
-		osmo_stat_item_dec(osmo_stat_item_group_get_item(endp->trunk->stats.common,
-								 TRUNK_STAT_ENDPOINTS_USED), 1);
-
 	/* Reset endpoint parameters and states */
 	talloc_free(endp->callid);
 	endp->callid = NULL;
@@ -638,8 +631,6 @@
 	 * connection ids) */
 	endp->callid = talloc_strdup(endp, callid);
 	OSMO_ASSERT(endp->callid);
-	osmo_stat_item_inc(osmo_stat_item_group_get_item(endp->trunk->stats.common,
-							 TRUNK_STAT_ENDPOINTS_USED), 1);
 
 	/* Allocate resources */
 	switch (endp->trunk->trunk_type) {
diff --git a/src/libosmo-mgcp/mgcp_ratectr.c b/src/libosmo-mgcp/mgcp_ratectr.c
index 740a3b0..1f8b233 100644
--- a/src/libosmo-mgcp/mgcp_ratectr.c
+++ b/src/libosmo-mgcp/mgcp_ratectr.c
@@ -24,11 +24,8 @@
 
 #include <errno.h>
 #include <osmocom/core/stats.h>
-#include <osmocom/core/stat_item.h>
 #include <osmocom/mgcp/mgcp_conn.h>
 #include <osmocom/mgcp/mgcp_trunk.h>
-#include <osmocom/mgcp/mgcp_protocol.h>
-#include <osmocom/mgcp/mgcp_endp.h>
 #include <osmocom/mgcp/mgcp_ratectr.h>
 
 static const struct rate_ctr_desc mgcp_general_ctr_desc[] = {
@@ -251,41 +248,3 @@
 	}
 	return 0;
 }
-
-const struct osmo_stat_item_desc trunk_stat_desc[] = {
-	[TRUNK_STAT_ENDPOINTS_TOTAL] = { "endpoints:total",
-					 "Number of endpoints that exist on the trunk",
-					 "", 60, 0 },
-	[TRUNK_STAT_ENDPOINTS_USED] = { "endpoints:used",
-					"Number of endpoints in use",
-					"", 60, 0 },
-};
-
-const struct osmo_stat_item_group_desc trunk_statg_desc = {
-	.group_name_prefix = "trunk",
-	.group_description = "mgw trunk",
-	.class_id = OSMO_STATS_CLASS_GLOBAL,
-	.num_items = ARRAY_SIZE(trunk_stat_desc),
-	.item_desc = trunk_stat_desc,
-};
-
-/*! allocate trunk specific stat items
- *  (called once on trunk initialization).
- *  \param[in] trunk for which the stat items are allocated.
- *  \returns 0 on success, -EINVAL on failure. */
-int mgcp_stat_trunk_alloc(struct mgcp_trunk *trunk)
-{
-	struct mgcp_stat_trunk *stats = &trunk->stats;
-	static unsigned int common_stat_index = 0;
-	char stat_name[256];
-
-	stats->common = osmo_stat_item_group_alloc(trunk, &trunk_statg_desc, common_stat_index);
-	if (!stats->common)
-		return -EINVAL;
-	snprintf(stat_name, sizeof(stat_name), "%s-%u:common", mgcp_trunk_type_strs_str(trunk->trunk_type),
-		 trunk->trunk_nr);
-	osmo_stat_item_group_set_name(stats->common, stat_name);
-	common_stat_index++;
-
-	return 0;
-}
diff --git a/src/libosmo-mgcp/mgcp_trunk.c b/src/libosmo-mgcp/mgcp_trunk.c
index b2d1969..27663b4 100644
--- a/src/libosmo-mgcp/mgcp_trunk.c
+++ b/src/libosmo-mgcp/mgcp_trunk.c
@@ -27,7 +27,6 @@
 #include <osmocom/mgcp/mgcp_trunk.h>
 #include <osmocom/mgcp/mgcp_e1.h>
 #include <osmocom/abis/e1_input.h>
-#include <osmocom/core/stat_item.h>
 
 const struct value_string mgcp_trunk_type_strs[] = {
 	{ MGCP_TRUNK_VIRTUAL,		"virtual" },
@@ -65,7 +64,6 @@
 	llist_add_tail(&trunk->entry, &cfg->trunks);
 
 	mgcp_ratectr_trunk_alloc(trunk);
-	mgcp_stat_trunk_alloc(trunk);
 
 	return trunk;
 }
@@ -129,8 +127,7 @@
 
 	/* make the endpoints we just created available to the MGW code */
 	trunk->number_endpoints = number_endpoints;
-	osmo_stat_item_set(osmo_stat_item_group_get_item(trunk->stats.common, TRUNK_STAT_ENDPOINTS_TOTAL),
-			   trunk->number_endpoints);
+
 	return 0;
 }
 
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index 26fcc2a..bcbcc02 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -1065,20 +1065,20 @@
 	int i;
 	struct mgcp_endpoint endp;
 	struct mgcp_endpoint *endpoints[1];
-	struct mgcp_config *cfg;
-	struct mgcp_trunk *trunk;
+	struct mgcp_config cfg = {0};
+	struct mgcp_trunk trunk;
 
 	printf("Testing packet loss calculation.\n");
 
 	memset(&endp, 0, sizeof(endp));
-	cfg = mgcp_config_alloc();
-	trunk = mgcp_trunk_alloc(cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID);
-	endp.cfg = cfg;
+	memset(&trunk, 0, sizeof(trunk));
+
+	endp.cfg = &cfg;
 	endp.type = &ep_typeset.rtp;
-	trunk->v.vty_number_endpoints = 1;
-	trunk->endpoints = endpoints;
-	trunk->endpoints[0] = &endp;
-	endp.trunk = trunk;
+	trunk.v.vty_number_endpoints = 1;
+	trunk.endpoints = endpoints;
+	trunk.endpoints[0] = &endp;
+	endp.trunk = &trunk;
 	INIT_LLIST_HEAD(&endp.conns);
 
 	for (i = 0; i < ARRAY_SIZE(pl_test_dat); ++i) {
@@ -1116,8 +1116,6 @@
 		mgcp_conn_free_all(&endp);
 	}
 
-	talloc_free(trunk);
-	talloc_free(cfg);
 }
 
 int mgcp_parse_stats(struct msgb *msg, uint32_t *ps, uint32_t *os,
@@ -1299,10 +1297,10 @@
 {
 	int i;
 
-	struct mgcp_trunk *trunk;
+	struct mgcp_trunk trunk;
 	struct mgcp_endpoint endp;
 	struct mgcp_endpoint *endpoints[1];
-	struct mgcp_config *cfg;
+	struct mgcp_config cfg = {0};
 	struct mgcp_rtp_state state;
 	struct mgcp_rtp_end *rtp;
 	struct osmo_sockaddr addr = { 0 };
@@ -1320,8 +1318,7 @@
 	       patch_ssrc ? ", patch SSRC" : "",
 	       patch_ts ? ", patch timestamps" : "");
 
-	cfg = mgcp_config_alloc();
-	trunk = mgcp_trunk_alloc(cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID);
+	memset(&trunk, 0, sizeof(trunk));
 	memset(&endp, 0, sizeof(endp));
 	memset(&state, 0, sizeof(state));
 
@@ -1330,16 +1327,16 @@
 	state.in_stream.err_ts_ctr = &test_ctr_in;
 	state.out_stream.err_ts_ctr = &test_ctr_out;
 
-	endp.cfg = cfg;
+	endp.cfg = &cfg;
 	endp.type = &ep_typeset.rtp;
 
-	trunk->v.vty_number_endpoints = 1;
-	trunk->endpoints = endpoints;
-	trunk->endpoints[0] = &endp;
-	trunk->force_constant_ssrc = patch_ssrc;
-	trunk->force_aligned_timing = patch_ts;
+	trunk.v.vty_number_endpoints = 1;
+	trunk.endpoints = endpoints;
+	trunk.endpoints[0] = &endp;
+	trunk.force_constant_ssrc = patch_ssrc;
+	trunk.force_aligned_timing = patch_ts;
 
-	endp.trunk = trunk;
+	endp.trunk = &trunk;
 
 	INIT_LLIST_HEAD(&endp.conns);
 	_conn = mgcp_conn_alloc(NULL, &endp, MGCP_CONN_TYPE_RTP,
@@ -1398,8 +1395,6 @@
 
 	force_monotonic_time_us = -1;
 	mgcp_conn_free_all(&endp);
-	talloc_free(trunk);
-	talloc_free(cfg);
 }
 
 static void test_multilple_codec(void)

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

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I96792a719c9c7273676ab9ffe0b9e2aae4c23166
Gerrit-Change-Number: 24982
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210721/2bc8af54/attachment.htm>


More information about the gerrit-log mailing list