diff mbox

[6/6] osmux: Remember the allocated CID and make sure it is release

Message ID 1443950574-75194-6-git-send-email-holger@freyther.de
State Accepted
Headers show

Commit Message

Holger Freyther Oct. 4, 2015, 9:22 a.m. UTC 
From: Holger Hans Peter Freyther <holger@moiji-mobile.com>

There appears to be a leak of CIDs:
 <000b> mgcp_osmux.c:544 All Osmux circuits are in use!

There are paths that a CID had been requested and never released
of the NAT. Remember the allocated CID inside the endpoint so it
can always be released. It is using a new variable as the behavior
for the NAT and MGCP MGW is different.

Fixes: OW#1493
---
 openbsc/include/openbsc/mgcp_internal.h   |  2 ++
 openbsc/include/openbsc/osmux.h           |  2 ++
 openbsc/src/libmgcp/mgcp_osmux.c          | 13 +++++++++++++
 openbsc/src/libmgcp/mgcp_protocol.c       |  4 ++++
 openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c | 18 +++++++++---------
 5 files changed, 30 insertions(+), 9 deletions(-)

Comments

Holger Freyther Oct. 8, 2015, 2:45 p.m. UTC | #1 
> On 04 Oct 2015, at 11:22, Holger Hans Peter Freyther <holger@freyther.de> wrote:
> 
> 
> +		uint8_t allocated_cid;

...

> +	endp->osmux.allocated_cid = -1;


that doesn't mix too well. This means I assigned 255 to the value and then we have
the next funny business in:

        char osmux_extension[strlen("X-Osmux: 255")];

        buf[0] = buf[39] = '\0';
        ret = sscanf(tok, "%*s %s", buf);
        if (ret != 1) {
                LOGP(DMGCP, LOGL_ERROR,
                        "Failed to find Endpoint in: %s\n", tok);
                return;
        }

        if (osmux_cid >= 0)
                sprintf(osmux_extension, "\nX-Osmux: %u", osmux_cid);


1.) osmux_extension doesn't account for the \n
2.) osmux_extension doesn't account for the \0 at the end of the string
3.) we use '%u' so nothing in this method checks if this is a uint8_t.

I will increase the buffer a bit. This would have started to crash on
tripple digit osmux cid's (so on setup of the 101st call).

holger
diff mbox

Patch

diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h
index 1f83659..8bb8f22 100644
--- a/openbsc/include/openbsc/mgcp_internal.h
+++ b/openbsc/include/openbsc/mgcp_internal.h
@@ -192,6 +192,8 @@  struct mgcp_endpoint {
 		/* Osmux state: disabled, activating, active */
 		enum osmux_state state;
 		/* Allocated Osmux circuit ID for this endpoint */
+		uint8_t allocated_cid;
+		/* Used Osmux circuit ID for this endpoint */
 		uint8_t cid;
 		/* handle to batch messages */
 		struct osmux_in_handle *in;
diff --git a/openbsc/include/openbsc/osmux.h b/openbsc/include/openbsc/osmux.h
index 88d045b..82b8fa3 100644
--- a/openbsc/include/openbsc/osmux.h
+++ b/openbsc/include/openbsc/osmux.h
@@ -14,6 +14,8 @@  int osmux_init(int role, struct mgcp_config *cfg);
 int osmux_enable_endpoint(struct mgcp_endpoint *endp, int role,
 			  struct in_addr *addr, uint16_t port);
 void osmux_disable_endpoint(struct mgcp_endpoint *endp);
+void osmux_allocate_cid(struct mgcp_endpoint *endp);
+void osmux_release_cid(struct mgcp_endpoint *endp);
 
 int osmux_xfrm_to_rtp(struct mgcp_endpoint *endp, int type, char *buf, int rc);
 int osmux_xfrm_to_osmux(int type, char *buf, int rc, struct mgcp_endpoint *endp);
diff --git a/openbsc/src/libmgcp/mgcp_osmux.c b/openbsc/src/libmgcp/mgcp_osmux.c
index 30a81cb..2d39b2c 100644
--- a/openbsc/src/libmgcp/mgcp_osmux.c
+++ b/openbsc/src/libmgcp/mgcp_osmux.c
@@ -492,6 +492,19 @@  void osmux_disable_endpoint(struct mgcp_endpoint *endp)
 	osmux_handle_put(endp->osmux.in);
 }
 
+void osmux_release_cid(struct mgcp_endpoint *endp)
+{
+	if (endp->osmux.allocated_cid >= 0)
+		osmux_put_cid(endp->osmux.allocated_cid);
+	endp->osmux.allocated_cid = -1;
+}
+
+void osmux_allocate_cid(struct mgcp_endpoint *endp)
+{
+	osmux_release_cid(endp);
+	endp->osmux.allocated_cid = osmux_get_cid();
+}
+
 /* We don't need to send the dummy load for osmux so often as another endpoint
  * may have already punched the hole in the firewall. This approach is simple
  * though.
diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c
index e2bda3a..42ce8bb 100644
--- a/openbsc/src/libmgcp/mgcp_protocol.c
+++ b/openbsc/src/libmgcp/mgcp_protocol.c
@@ -1314,6 +1314,7 @@  int mgcp_endpoints_allocate(struct mgcp_trunk_config *tcfg)
 		return -1;
 
 	for (i = 0; i < tcfg->number_endpoints; ++i) {
+		tcfg->endpoints[i].osmux.allocated_cid = -1;
 		tcfg->endpoints[i].ci = CI_UNUSED;
 		tcfg->endpoints[i].cfg = tcfg->cfg;
 		tcfg->endpoints[i].tcfg = tcfg;
@@ -1354,6 +1355,9 @@  void mgcp_release_endp(struct mgcp_endpoint *endp)
 	if (endp->osmux.state == OSMUX_STATE_ENABLED)
 		osmux_disable_endpoint(endp);
 
+	/* release the circuit ID if it had been allocated */
+	osmux_release_cid(endp);
+
 	memset(&endp->taps, 0, sizeof(endp->taps));
 }
 
diff --git a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c
index aad59d4..bd8f7a4 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c
@@ -515,7 +515,6 @@  static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int
 	struct nat_sccp_connection *sccp;
 	struct mgcp_endpoint *mgcp_endp;
 	struct msgb *bsc_msg;
-	int osmux_cid = -1;
 
 	nat = tcfg->cfg->data;
 	bsc_endp = &nat->bsc_endpoints[endpoint];
@@ -555,8 +554,9 @@  static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int
 	/* Allocate a Osmux circuit ID */
 	if (state == MGCP_ENDP_CRCX) {
 		if (nat->mgcp_cfg->osmux && sccp->bsc->cfg->osmux) {
-			osmux_cid = osmux_get_cid();
-			if (osmux_cid < 0 && nat_osmux_only(nat->mgcp_cfg, sccp->bsc->cfg)) {
+			osmux_allocate_cid(mgcp_endp);
+			if (mgcp_endp->osmux.allocated_cid < 0 &&
+				nat_osmux_only(nat->mgcp_cfg, sccp->bsc->cfg)) {
 				LOGP(DMGCP, LOGL_ERROR,
 					"Rejecting usage of endpoint\n");
 				return MGCP_POLICY_REJECT;
@@ -567,7 +567,8 @@  static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int
 	/* we need to generate a new and patched message */
 	bsc_msg = bsc_mgcp_rewrite((char *) nat->mgcp_msg, nat->mgcp_length,
 				   sccp->bsc_endp, mgcp_bts_src_addr(mgcp_endp),
-				   mgcp_endp->bts_end.local_port, osmux_cid,
+				   mgcp_endp->bts_end.local_port,
+				   mgcp_endp->osmux.allocated_cid,
 				   &mgcp_endp->net_end.codec.payload_type,
 				   nat->sdp_ensure_amr_mode_set);
 	if (!bsc_msg) {
@@ -587,10 +588,10 @@  static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int
 		/* Annotate the allocated Osmux CID until the bsc confirms that
 		 * it agrees to use Osmux for this voice flow.
 		 */
-		if (osmux_cid >= 0 &&
+		if (mgcp_endp->osmux.allocated_cid >= 0 &&
 		    mgcp_endp->osmux.state != OSMUX_STATE_ENABLED) {
 			mgcp_endp->osmux.state = OSMUX_STATE_ACTIVATING;
-			mgcp_endp->osmux.cid = osmux_cid;
+			mgcp_endp->osmux.cid = mgcp_endp->osmux.allocated_cid;
 		}
 
 		socklen_t len = sizeof(sock);
@@ -612,7 +613,7 @@  static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int
 
 		/* libmgcp clears the MGCP endpoint for us */
 		if (mgcp_endp->osmux.state == OSMUX_STATE_ENABLED)
-			osmux_put_cid(mgcp_endp->osmux.cid);
+			osmux_release_cid(mgcp_endp);
 
 		return MGCP_POLICY_CONT;
 	} else {
@@ -681,8 +682,7 @@  static void bsc_mgcp_osmux_confirm(struct mgcp_endpoint *endp, const char *str)
 	     osmux_cid);
 	return;
 err:
-	osmux_put_cid(endp->osmux.cid);
-	endp->osmux.cid = -1;
+	osmux_release_cid(endp);
 	endp->osmux.state = OSMUX_STATE_DISABLED;
 }