From: Holger Hans Peter Freyther holger@moiji-mobile.com
* Test that one can get an id * That they are assigned predicatble right now * That returning them will make the number of used ones go down * That allocating more will fail --- openbsc/include/openbsc/osmux.h | 1 + openbsc/src/libmgcp/mgcp_osmux.c | 14 ++++++++++++++ openbsc/tests/mgcp/mgcp_test.c | 26 ++++++++++++++++++++++++++ 3 files changed, 41 insertions(+)
diff --git a/openbsc/include/openbsc/osmux.h b/openbsc/include/openbsc/osmux.h index 8c01fd0..0e727d5 100644 --- a/openbsc/include/openbsc/osmux.h +++ b/openbsc/include/openbsc/osmux.h @@ -22,6 +22,7 @@ int osmux_send_dummy(struct mgcp_endpoint *endp);
int osmux_get_cid(void); void osmux_put_cid(uint8_t osmux_cid); +int osmux_used_cid(void);
enum osmux_state { OSMUX_STATE_DISABLED = 0, diff --git a/openbsc/src/libmgcp/mgcp_osmux.c b/openbsc/src/libmgcp/mgcp_osmux.c index 90b7368..b0ef69f 100644 --- a/openbsc/src/libmgcp/mgcp_osmux.c +++ b/openbsc/src/libmgcp/mgcp_osmux.c @@ -532,6 +532,20 @@ int osmux_send_dummy(struct mgcp_endpoint *endp) /* bsc-nat allocates/releases the Osmux circuit ID */ static uint8_t osmux_cid_bitmap[16];
+int osmux_used_cid(void) +{ + int i, j, used = 0; + + for (i = 0; i < sizeof(osmux_cid_bitmap) / 8; i++) { + for (j = 0; j < 8; j++) { + if (osmux_cid_bitmap[i] & (1 << j)) + used += 1; + } + } + + return used; +} + int osmux_get_cid(void) { int i, j; diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index b2cb938..7b5de31 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -1175,6 +1175,31 @@ static void test_no_name(void) talloc_free(cfg); }
+static void test_osmux_cid(void) +{ + int id, i; + + OSMO_ASSERT(osmux_used_cid() == 0); + id = osmux_get_cid(); + OSMO_ASSERT(id == 0); + OSMO_ASSERT(osmux_used_cid() == 1); + osmux_put_cid(id); + OSMO_ASSERT(osmux_used_cid() == 0); + + for (i = 0; i < 16; ++i) { + id = osmux_get_cid(); + OSMO_ASSERT(id == i); + OSMO_ASSERT(osmux_used_cid() == i + 1); + } + + id = osmux_get_cid(); + OSMO_ASSERT(id == -1); + + for (i = 0; i < 256; ++i) + osmux_put_cid(i); + OSMO_ASSERT(osmux_used_cid() == 0); +} + int main(int argc, char **argv) { osmo_init_logging(&log_info); @@ -1193,6 +1218,7 @@ int main(int argc, char **argv) test_multilple_codec(); test_no_cycle(); test_no_name(); + test_osmux_cid();
printf("Done\n"); return EXIT_SUCCESS;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
sizeof(uint8_t) == 1 and there is no need to create an array with 16 bytes and then only use the first two of them. This means the CID range is from 0 to 127 and we should be able to extend this to 256 by changing the array size to 32. Update the testcase now that we can have more than 16 calls with Osmux. --- openbsc/src/libmgcp/mgcp_osmux.c | 4 ++-- openbsc/tests/mgcp/mgcp_test.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/openbsc/src/libmgcp/mgcp_osmux.c b/openbsc/src/libmgcp/mgcp_osmux.c index b0ef69f..30a81cb 100644 --- a/openbsc/src/libmgcp/mgcp_osmux.c +++ b/openbsc/src/libmgcp/mgcp_osmux.c @@ -536,7 +536,7 @@ int osmux_used_cid(void) { int i, j, used = 0;
- for (i = 0; i < sizeof(osmux_cid_bitmap) / 8; i++) { + for (i = 0; i < sizeof(osmux_cid_bitmap); i++) { for (j = 0; j < 8; j++) { if (osmux_cid_bitmap[i] & (1 << j)) used += 1; @@ -550,7 +550,7 @@ int osmux_get_cid(void) { int i, j;
- for (i = 0; i < sizeof(osmux_cid_bitmap) / 8; i++) { + for (i = 0; i < sizeof(osmux_cid_bitmap); i++) { for (j = 0; j < 8; j++) { if (osmux_cid_bitmap[i] & (1 << j)) continue; diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 7b5de31..ec86c55 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -1186,7 +1186,7 @@ static void test_osmux_cid(void) osmux_put_cid(id); OSMO_ASSERT(osmux_used_cid() == 0);
- for (i = 0; i < 16; ++i) { + for (i = 0; i < 128; ++i) { id = osmux_get_cid(); OSMO_ASSERT(id == i); OSMO_ASSERT(osmux_used_cid() == i + 1);
From: Holger Hans Peter Freyther holger@moiji-mobile.com
* Print number of used CIDs for the system * Hopefully this is just the beginning --- openbsc/src/libmgcp/mgcp_vty.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index fc2f818..2b76436 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -226,6 +226,9 @@ DEFUN(show_mcgp, show_mgcp_cmd, llist_for_each_entry(trunk, &g_cfg->trunks, entry) dump_trunk(vty, trunk, show_stats);
+ if (g_cfg->osmux) + vty_out(vty, "Osmux used CID: %d%s", osmux_used_cid(), VTY_NEWLINE); + return CMD_SUCCESS; }
From: Holger Hans Peter Freyther holger@moiji-mobile.com
Some systems only want to use Osmux. In case only Osmux should be used fail if it has not be offered/acked.
Client:
Verified On, Off and Only with X-Osmux: 3 and without this field. <000b> mgcp_protocol.c:823 Osmux only and no osmux offered on 0x14 <000b> mgcp_protocol.c:884 Resource error on 0x14
NAT:
Not tested and implemented
Fixes: OW#1492 --- openbsc/include/openbsc/osmux.h | 6 +++++ openbsc/src/libmgcp/mgcp_protocol.c | 4 ++++ openbsc/src/libmgcp/mgcp_vty.c | 45 +++++++++++++++++++++++++------------ 3 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/openbsc/include/openbsc/osmux.h b/openbsc/include/openbsc/osmux.h index 0e727d5..88d045b 100644 --- a/openbsc/include/openbsc/osmux.h +++ b/openbsc/include/openbsc/osmux.h @@ -30,4 +30,10 @@ enum osmux_state { OSMUX_STATE_ENABLED, };
+enum osmux_usage { + OSMUX_USAGE_OFF = 0, + OSMUX_USAGE_ON = 1, + OSMUX_USAGE_ONLY = 2, +}; + #endif diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index ab98be1..e2bda3a 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -818,6 +818,10 @@ mgcp_header_done: if (osmux_cid >= 0) { endp->osmux.cid = osmux_cid; endp->osmux.state = OSMUX_STATE_ACTIVATING; + } else if(endp->cfg->osmux == OSMUX_USAGE_ONLY) { + LOGP(DMGCP, LOGL_ERROR, + "Osmux only and no osmux offered on 0x%x\n", ENDPOINT_NUMBER(endp)); + goto error2; }
endp->allocated = 1; diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index 2b76436..c478b0a 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -139,8 +139,19 @@ static int config_write_mgcp(struct vty *vty) if (g_cfg->bts_force_ptime > 0) vty_out(vty, " rtp force-ptime %d%s", g_cfg->bts_force_ptime, VTY_NEWLINE); vty_out(vty, " transcoder-remote-base %u%s", g_cfg->transcoder_remote_base, VTY_NEWLINE); - vty_out(vty, " osmux %s%s", - g_cfg->osmux == 1 ? "on" : "off", VTY_NEWLINE); + + switch (g_cfg->osmux) { + case OSMUX_USAGE_ON: + vty_out(vty, " osmux on%s", VTY_NEWLINE); + break; + case OSMUX_USAGE_ONLY: + vty_out(vty, " osmux only%s", VTY_NEWLINE); + break; + case OSMUX_USAGE_OFF: + default: + vty_out(vty, " osmux off%s", VTY_NEWLINE); + break; + } if (g_cfg->osmux) { vty_out(vty, " osmux batch-factor %d%s", g_cfg->osmux_batch, VTY_NEWLINE); @@ -1249,18 +1260,24 @@ DEFUN(reset_all_endp, reset_all_endp_cmd, #define OSMUX_STR "RTP multiplexing\n" DEFUN(cfg_mgcp_osmux, cfg_mgcp_osmux_cmd, - "osmux (on|off)", - OSMUX_STR "Enable OSMUX\n" "Disable OSMUX\n") -{ - if (strcmp(argv[0], "on") == 0) { - g_cfg->osmux = 1; - if (g_cfg->trunk.audio_loop) { - vty_out(vty, "Cannot use `loop' with `osmux'.%s", - VTY_NEWLINE); - return CMD_WARNING; - } - } else if (strcmp(argv[0], "off") == 0) - g_cfg->osmux = 0; + "osmux (on|off|only)", + OSMUX_STR "Enable OSMUX\n" "Disable OSMUX\n" "Only use OSMUX\n") +{ + if (strcmp(argv[0], "off") == 0) { + g_cfg->osmux = OSMUX_USAGE_OFF; + return CMD_SUCCESS; + } + + if (strcmp(argv[0], "on") == 0) + g_cfg->osmux = OSMUX_USAGE_ON; + else if (strcmp(argv[0], "only") == 0) + g_cfg->osmux = OSMUX_USAGE_ONLY; + + if (g_cfg->trunk.audio_loop) { + vty_out(vty, "Cannot use `loop' with `osmux'.%s", + VTY_NEWLINE); + return CMD_WARNING; + }
return CMD_SUCCESS; }
From: Holger Hans Peter Freyther holger@moiji-mobile.com
Extend the osmux only setting from the MGCP MGW to the NAT. This is applied when an endpoint is allocated and/or when the allocation is confirmed by the remote system.
Not tested. The impact should only be when the new option is being used.
Fixes: OW#1492 --- openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c | 32 ++++++++++++++++++++++++++++--- openbsc/src/osmo-bsc_nat/bsc_nat_vty.c | 22 ++++++++++++++------- 2 files changed, 44 insertions(+), 10 deletions(-)
diff --git a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c index f19cb4c..aad59d4 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c +++ b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c @@ -499,6 +499,15 @@ struct nat_sccp_connection *bsc_mgcp_find_con(struct bsc_nat *nat, int endpoint) return NULL; }
+static int nat_osmux_only(struct mgcp_config *mgcp_cfg, struct bsc_config *bsc_cfg) +{ + if (mgcp_cfg->osmux == OSMUX_USAGE_ONLY) + return 1; + if (bsc_cfg->osmux == OSMUX_USAGE_ONLY) + return 1; + return 0; +} + static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int state, const char *transaction_id) { struct bsc_nat *nat; @@ -544,9 +553,16 @@ static int bsc_mgcp_policy_cb(struct mgcp_trunk_config *tcfg, int endpoint, int }
/* Allocate a Osmux circuit ID */ - if (state == MGCP_ENDP_CRCX && - nat->mgcp_cfg->osmux && sccp->bsc->cfg->osmux) - osmux_cid = osmux_get_cid(); + 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)) { + LOGP(DMGCP, LOGL_ERROR, + "Rejecting usage of endpoint\n"); + return MGCP_POLICY_REJECT; + } + } + }
/* we need to generate a new and patched message */ bsc_msg = bsc_mgcp_rewrite((char *) nat->mgcp_msg, nat->mgcp_length, @@ -734,6 +750,16 @@ void bsc_mgcp_forward(struct bsc_connection *bsc, struct msgb *msg) if (endp->osmux.state == OSMUX_STATE_ACTIVATING) bsc_mgcp_osmux_confirm(endp, (const char *) msg->l2h);
+ /* If we require osmux and it is not disabled.. fail */ + if (nat_osmux_only(bsc->nat->mgcp_cfg, bsc->cfg) && + endp->osmux.state == OSMUX_STATE_DISABLED) { + LOGP(DMGCP, LOGL_ERROR, + "Failed to activate osmux endpoint 0x%x\n", + ENDPOINT_NUMBER(endp)); + free_chan_downstream(endp, bsc_endp, bsc); + return; + } + /* free some stuff */ talloc_free(bsc_endp->transaction_id); bsc_endp->transaction_id = NULL; diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c index 981168c..cd8293c 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c @@ -162,8 +162,14 @@ static void config_write_bsc_single(struct vty *vty, struct bsc_config *bsc) if (bsc->paging_group != -1) vty_out(vty, " paging group %d%s", bsc->paging_group, VTY_NEWLINE); vty_out(vty, " paging forbidden %d%s", bsc->forbid_paging, VTY_NEWLINE); - if (bsc->osmux) + switch (bsc->osmux) { + case OSMUX_USAGE_ON: vty_out(vty, " osmux on%s", VTY_NEWLINE); + break; + case OSMUX_USAGE_ONLY: + vty_out(vty, " osmux only%s", VTY_NEWLINE); + break; + } }
static int config_write_bsc(struct vty *vty) @@ -1124,18 +1130,20 @@ DEFUN(show_ussd_connection, #define OSMUX_STR "RTP multiplexing\n" DEFUN(cfg_bsc_osmux, cfg_bsc_osmux_cmd, - "osmux (on|off)", - OSMUX_STR "Enable OSMUX\n" "Disable OSMUX\n") + "osmux (on|off|only)", + OSMUX_STR "Enable OSMUX\n" "Disable OSMUX\n" "Only OSMUX\n") { struct bsc_config *conf = vty->index; int old = conf->osmux;
if (strcmp(argv[0], "on") == 0) - conf->osmux = 1; + conf->osmux = OSMUX_USAGE_ON; else if (strcmp(argv[0], "off") == 0) - conf->osmux = 0; + conf->osmux = OSMUX_USAGE_OFF; + else if (strcmp(argv[0], "only") == 0) + conf->osmux = OSMUX_USAGE_ONLY;
- if (old == 0 && conf->osmux == 1 && !conf->nat->mgcp_cfg->osmux_init) { + if (old == 0 && conf->osmux > 0 && !conf->nat->mgcp_cfg->osmux_init) { LOGP(DMGCP, LOGL_NOTICE, "Setting up OSMUX socket\n"); if (osmux_init(OSMUX_ROLE_BSC_NAT, conf->nat->mgcp_cfg) < 0) { LOGP(DMGCP, LOGL_ERROR, "Cannot init OSMUX\n"); @@ -1143,7 +1151,7 @@ DEFUN(cfg_bsc_osmux, VTY_NEWLINE); return CMD_WARNING; } - } else if (old == 1 && conf->osmux == 0) { + } else if (old > 0 && conf->osmux == 0) { LOGP(DMGCP, LOGL_NOTICE, "Disabling OSMUX socket\n"); /* Don't stop the socket, we may already have ongoing voice * flows already using Osmux. This just switch indicates that
On Sun, Oct 04, 2015 at 11:22:53AM +0200, Holger Hans Peter Freyther wrote:
- /* If we require osmux and it is not disabled.. fail */
- if (nat_osmux_only(bsc->nat->mgcp_cfg, bsc->cfg) &&
endp->osmux.state == OSMUX_STATE_DISABLED) {
[...]
Comment: If we require osmux and it IS disabled, fail
That's the only thing I spotted in your patch series.
~Neels
On 05 Oct 2015, at 12:37, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
On Sun, Oct 04, 2015 at 11:22:53AM +0200, Holger Hans Peter Freyther wrote:
- /* If we require osmux and it is not disabled.. fail */
- if (nat_osmux_only(bsc->nat->mgcp_cfg, bsc->cfg) &&
endp->osmux.state == OSMUX_STATE_DISABLED) {[...]
Comment: If we require osmux and it IS disabled, fail
That's the only thing I spotted in your patch series.
thanks!
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(-)
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; }
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
-
Holger Freyther -
Holger Hans Peter Freyther
-
Neels Hofmeyr