From a01bd60851c2d2b613cda6148fb1ca5976887a93 Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Fri, 29 Nov 2013 13:43:43 +0100 Subject: [PATCH 1/7] mgcp: Rename for_each_line to for_each_non_empty_line The implementation of for_each_line is based on strtok() and skips any sequence of CR and LF. Thus empty lines are never detected. There exists code which tests for an empty line to detect the beginning of the SDP part which is dead code currently (the parser works nevertheless due to other reasons). So the semantics of this macro have been misunderstood at least once. This patch renames the macro to reflect the semantics more precisely. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 70964958a..c040ab1b4 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -36,7 +36,7 @@ #include #include -#define for_each_line(line, save) \ +#define for_each_non_empty_line(line, save) \ for (line = strtok_r(NULL, "\r\n", &save); line;\ line = strtok_r(NULL, "\r\n", &save)) @@ -527,7 +527,7 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) return create_err_response(NULL, 510, "CRCX", p->trans); /* parse CallID C: and LocalParameters L: */ - for_each_line(line, p->save) { + for_each_non_empty_line(line, p->save) { switch (line[0]) { case 'L': local_options = (const char *) line + 3; @@ -652,7 +652,7 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) return create_err_response(endp, 400, "MDCX", p->trans); } - for_each_line(line, p->save) { + for_each_non_empty_line(line, p->save) { switch (line[0]) { case 'C': { if (verify_call_id(endp, line + 3) != 0) @@ -771,7 +771,7 @@ static struct msgb *handle_delete_con(struct mgcp_parse_data *p) return create_err_response(endp, 400, "DLCX", p->trans); } - for_each_line(line, p->save) { + for_each_non_empty_line(line, p->save) { switch (line[0]) { case 'C': if (verify_call_id(endp, line + 3) != 0) @@ -873,7 +873,7 @@ static struct msgb *handle_noti_req(struct mgcp_parse_data *p) if (p->found != 0) return create_err_response(NULL, 400, "RQNT", p->trans); - for_each_line(line, p->save) { + for_each_non_empty_line(line, p->save) { switch (line[0]) { case 'S': tone = extract_tone(line); @@ -1218,7 +1218,7 @@ int mgcp_parse_stats(struct msgb *msg, uint32_t *ps, uint32_t *os, return -1; /* this can only parse the message that is created above... */ - for_each_line(line, save) { + for_each_non_empty_line(line, save) { switch (line[0]) { case 'P': rc = sscanf(line, "P: PS=%u, OS=%u, PR=%u, OR=%u, PL=%d, JI=%u", From 2bee7f96ff46f2f7e563d7aca5c4703214b6cd0e Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Fri, 29 Nov 2013 13:43:44 +0100 Subject: [PATCH 2/7] mgcp: Add new for_each_line macro that also returns empty lines This patch add the for_each_line macro based on a strline_r() function (similar to strtok_r()), that is also part of this patch. This strline_r() function is tolerant with respect to line endings, it supports CR-only, CRLF, and LF-only and any combinations thereof (note that a CRLF is always detected as a single line break). Similar to for_each_non_empty_line (the former for_each_line) where the 'save' pointer needed to be initialised by a call to strtok_r(), the new for_each_line macro expects, that the 'save' pointer has been initialised by a call to strline_r(). Also note, that for_each_line/strline_r and for_each_non_empty_line/strtok_r may use the 'save' pointer differently, so calls to them can not be mixed. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 32 ++++++++++++++++++++++++++ openbsc/tests/mgcp/mgcp_test.c | 35 +++++++++++++++++++++++++++++ openbsc/tests/mgcp/mgcp_test.ok | 13 +++++++++++ 3 files changed, 80 insertions(+) diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index c040ab1b4..19a6f5325 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -40,6 +40,38 @@ for (line = strtok_r(NULL, "\r\n", &save); line;\ line = strtok_r(NULL, "\r\n", &save)) +#define for_each_line(line, save) \ + for (line = strline_r(NULL, &save); line;\ + line = strline_r(NULL, &save)) + +char *strline_r(char *str, char **saveptr) +{ + char *result; + + if (str) + *saveptr = str; + + result = *saveptr; + + if (*saveptr != NULL) { + *saveptr = strpbrk(*saveptr, "\r\n"); + + if (*saveptr != NULL) { + char *eos = *saveptr; + + if ((*saveptr)[0] == '\r' && (*saveptr)[1] == '\n') + (*saveptr)++; + (*saveptr)++; + if ((*saveptr)[0] == '\0') + *saveptr = NULL; + + *eos = '\0'; + } + } + + return result; +} + /* Assume audio frame length of 20ms */ #define DEFAULT_RTP_AUDIO_FRAME_DUR_NUM 20 #define DEFAULT_RTP_AUDIO_FRAME_DUR_DEN 1000 diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index d36aaa8c6..362f02986 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -25,6 +25,40 @@ #include #include +char *strline_r(char *str, char **saveptr); + +const char *strline_test_data = + "one CR\r" + "two CR\r" + "\r" + "one CRLF\r\n" + "two CRLF\r\n" + "\r\n" + "one LF\n" + "two LF\n" + "\n" + "mixed (4 lines)\r\r\n\n\r\n"; + +#define EXPECTED_NUMBER_OF_LINES 13 + +static void test_strline(void) +{ + char *save = NULL; + char *line; + char buf[2048]; + int counter = 0; + + strncpy(buf, strline_test_data, sizeof(buf)); + + for (line = strline_r(buf, &save); line; + line = strline_r(NULL, &save)) { + printf("line: '%s'\n", line); + counter++; + } + + OSMO_ASSERT(counter == EXPECTED_NUMBER_OF_LINES); +} + #define AUEP1 "AUEP 158663169 ds/e1-1/2@172.16.6.66 MGCP 1.0\r\n" #define AUEP1_RET "200 158663169 OK\r\n" #define AUEP2 "AUEP 18983213 ds/e1-2/1@172.16.6.66 MGCP 1.0\r\n" @@ -463,6 +497,7 @@ int main(int argc, char **argv) { osmo_init_logging(&log_info); + test_strline(); test_messages(); test_retransmission(); test_packet_loss_calc(); diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 5666424bd..429e0df68 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -1,3 +1,16 @@ +line: 'one CR' +line: 'two CR' +line: '' +line: 'one CRLF' +line: 'two CRLF' +line: '' +line: 'one LF' +line: 'two LF' +line: '' +line: 'mixed (4 lines)' +line: '' +line: '' +line: '' Testing AUEP1 Testing AUEP2 Testing MDCX1 From a52ac66e5227a31a4f8ecec6aa38b124cf6cb82b Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Fri, 29 Nov 2013 13:43:45 +0100 Subject: [PATCH 3/7] mgcp: Add tests for payload types in MGCP messages These tests mainly check whether the SDP parsing works properly by looking at the payload type detected. Sponsored-by: On-Waves ehf --- openbsc/tests/bsc-nat/bsc_data.c | 5 ++ openbsc/tests/bsc-nat/bsc_nat_test.c | 16 +++++- openbsc/tests/mgcp/mgcp_test.c | 82 ++++++++++++++++++++++++++-- openbsc/tests/mgcp/mgcp_test.ok | 1 + 4 files changed, 98 insertions(+), 6 deletions(-) diff --git a/openbsc/tests/bsc-nat/bsc_data.c b/openbsc/tests/bsc-nat/bsc_data.c index 5a76689e9..d1f8ebc0d 100644 --- a/openbsc/tests/bsc-nat/bsc_data.c +++ b/openbsc/tests/bsc-nat/bsc_data.c @@ -177,6 +177,7 @@ struct mgcp_patch_test { const char *patch; const char *ip; const int port; + const int payload_type; }; static const struct mgcp_patch_test mgcp_messages[] = { @@ -191,24 +192,28 @@ static const struct mgcp_patch_test mgcp_messages[] = { .patch = crcx_resp_patched, .ip = "10.0.0.1", .port = 999, + .payload_type = 98, }, { .orig = mdcx, .patch = mdcx_patched, .ip = "10.0.0.23", .port = 6666, + .payload_type = 126, }, { .orig = mdcx_resp, .patch = mdcx_resp_patched, .ip = "10.0.0.23", .port = 5555, + .payload_type = 98, }, { .orig = mdcx_resp2, .patch = mdcx_resp_patched2, .ip = "10.0.0.23", .port = 5555, + .payload_type = 98, }, }; diff --git a/openbsc/tests/bsc-nat/bsc_nat_test.c b/openbsc/tests/bsc-nat/bsc_nat_test.c index 5158f46cf..3320e0694 100644 --- a/openbsc/tests/bsc-nat/bsc_nat_test.c +++ b/openbsc/tests/bsc-nat/bsc_nat_test.c @@ -624,10 +624,24 @@ static void test_mgcp_rewrite(void) const char *patc = mgcp_messages[i].patch; const char *ip = mgcp_messages[i].ip; const int port = mgcp_messages[i].port; + const int expected_payload_type = mgcp_messages[i].payload_type; + int payload_type = -1; char *input = strdup(orig); - output = bsc_mgcp_rewrite(input, strlen(input), 0x1e, ip, port); + output = bsc_mgcp_rewrite(input, strlen(input), 0x1e, + ip, port); + + if (payload_type != -1) { + fprintf(stderr, "Found media payload type %d in SDP data\n", + payload_type); + if (payload_type != expected_payload_type) { + printf("Wrong payload type %d (expected %d)\n", + payload_type, expected_payload_type); + abort(); + } + } + if (msgb_l2len(output) != strlen(patc)) { printf("Wrong sizes for test: %d %d != %d != %d\n", i, msgb_l2len(output), strlen(patc), strlen(orig)); printf("String '%s' vs '%s'\n", (const char *) output->l2h, patc); diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 362f02986..0aebb4c55 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -82,6 +82,26 @@ static void test_strline(void) "t=0 0\r\n" \ "m=audio 0 RTP/AVP 126\r\n" \ "a=rtpmap:126 AMR/8000\r\n" +#define MDCX4 "MDCX 18983216 1@mgw MGCP 1.0\r\n" \ + "C: 2\r\n" \ + "I: 1\r\n" \ + "L: p:20, a:AMR, nt:IN\r\n" \ + "\n" \ + "v=0\r\n" \ + "o=- 1 23 IN IP4 0.0.0.0\r\n" \ + "c=IN IP4 0.0.0.0\r\n" \ + "t=0 0\r\n" \ + "m=audio 4441 RTP/AVP 99\r\n" \ + "a=rtpmap:99 AMR/8000\r\n" +#define MDCX4_RET "200 18983216 OK\r\n" \ + "I: 1\n" \ + "\n" \ + "v=0\r\n" \ + "o=- 1 23 IN IP4 0.0.0.0\r\n" \ + "c=IN IP4 0.0.0.0\r\n" \ + "t=0 0\r\n" \ + "m=audio 0 RTP/AVP 126\r\n" \ + "a=rtpmap:126 AMR/8000\r\n" #define SHORT2 "CRCX 1" #define SHORT2_RET "510 000000 FAIL\r\n" @@ -145,10 +165,16 @@ static void test_strline(void) #define RQNT1_RET "200 186908780 OK\r\n" #define RQNT2_RET "200 186908781 OK\r\n" +#define PTYPE_IGNORE 0 /* == default initializer */ +#define PTYPE_NONE 128 +#define PTYPE_NYI PTYPE_NONE + struct mgcp_test { const char *name; const char *req; const char *exp_resp; + int exp_net_ptype; + int exp_bts_ptype; }; static const struct mgcp_test tests[] = { @@ -156,10 +182,11 @@ static const struct mgcp_test tests[] = { { "AUEP2", AUEP2, AUEP2_RET }, { "MDCX1", MDCX_WRONG_EP, MDCX_ERR_RET }, { "MDCX2", MDCX_UNALLOCATED, MDCX_RET }, - { "CRCX", CRCX, CRCX_RET }, - { "MDCX3", MDCX3, MDCX3_RET }, - { "DLCX", DLCX, DLCX_RET }, - { "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET }, + { "CRCX", CRCX, CRCX_RET, PTYPE_NYI, 126 }, + { "MDCX3", MDCX3, MDCX3_RET, PTYPE_NONE, 126 }, + { "MDCX4", MDCX4, MDCX4_RET, 99, 126 }, + { "DLCX", DLCX, DLCX_RET, -1, -1 }, + { "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET, PTYPE_NYI, 126 }, { "EMPTY", EMPTY, EMPTY_RET }, { "SHORT1", SHORT, SHORT_RET }, { "SHORT2", SHORT2, SHORT2_RET }, @@ -167,7 +194,7 @@ static const struct mgcp_test tests[] = { { "SHORT4", SHORT4, SHORT2_RET }, { "RQNT1", RQNT, RQNT1_RET }, { "RQNT2", RQNT2, RQNT2_RET }, - { "DLCX", DLCX, DLCX_RET }, + { "DLCX", DLCX, DLCX_RET, -1, -1 }, }; static const struct mgcp_test retransmit[] = { @@ -188,9 +215,21 @@ static struct msgb *create_msg(const char *str) return msg; } +static int last_endpoint = -1; + +static int mgcp_test_policy_cb(struct mgcp_trunk_config *cfg, int endpoint, + int state, const char *transactio_id) +{ + fprintf(stderr, "Policy CB got state %d on endpoint %d\n", + state, endpoint); + last_endpoint = endpoint; + return MGCP_POLICY_CONT; +} + static void test_messages(void) { struct mgcp_config *cfg; + struct mgcp_endpoint *endp; int i; cfg = mgcp_config_alloc(); @@ -198,8 +237,16 @@ static void test_messages(void) cfg->trunk.number_endpoints = 64; mgcp_endpoints_allocate(&cfg->trunk); + cfg->policy_cb = mgcp_test_policy_cb; + mgcp_endpoints_allocate(mgcp_trunk_alloc(cfg, 1)); + /* reset endpoints */ + for (i = 0; i < cfg->trunk.number_endpoints; i++) { + endp = &cfg->trunk.endpoints[i]; + endp->net_end.payload_type = PTYPE_NONE; + } + for (i = 0; i < ARRAY_SIZE(tests); i++) { const struct mgcp_test *t = &tests[i]; struct msgb *inp; @@ -207,6 +254,8 @@ static void test_messages(void) printf("Testing %s\n", t->name); + last_endpoint = -1; + inp = create_msg(t->req); msg = mgcp_handle_message(cfg, inp); msgb_free(inp); @@ -216,6 +265,29 @@ static void test_messages(void) } else if (strcmp((char *) msg->data, t->exp_resp) != 0) printf("%s failed '%s'\n", t->name, (char *) msg->data); msgb_free(msg); + + /* Check detected payload type */ + if (t->exp_net_ptype != PTYPE_IGNORE || + t->exp_bts_ptype != PTYPE_IGNORE) { + OSMO_ASSERT(last_endpoint != -1); + endp = &cfg->trunk.endpoints[last_endpoint]; + + fprintf(stderr, "endpoint %d: " + "payload type BTS %d (exp %d), NET %d (exp %d)\n", + last_endpoint, + endp->bts_end.payload_type, t->exp_bts_ptype, + endp->net_end.payload_type, t->exp_net_ptype); + + if (t->exp_bts_ptype != PTYPE_IGNORE) + OSMO_ASSERT(endp->bts_end.payload_type == + t->exp_bts_ptype); + if (t->exp_net_ptype != PTYPE_IGNORE) + OSMO_ASSERT(endp->net_end.payload_type == + t->exp_net_ptype); + + /* Reset them again for next test */ + endp->net_end.payload_type = PTYPE_NONE; + } } talloc_free(cfg); diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 429e0df68..3bfd78b3d 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -17,6 +17,7 @@ Testing MDCX1 Testing MDCX2 Testing CRCX Testing MDCX3 +Testing MDCX4 Testing DLCX Testing CRCX_ZYN Testing EMPTY From 1771171e056a167c559c7f479512647642f518f9 Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Fri, 29 Nov 2013 13:43:46 +0100 Subject: [PATCH 4/7] mgcp: Refactor MGCP/SDP parsing This patch separates the SDP parsing from the (message specific) MGCP parsing. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 98 +++++++++++++++++++---------- 1 file changed, 66 insertions(+), 32 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 19a6f5325..d4a23a7d7 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -282,7 +282,7 @@ struct msgb *mgcp_handle_message(struct mgcp_config *cfg, struct msgb *msg) */ memset(&pdata, 0, sizeof(pdata)); pdata.cfg = cfg; - data = strtok_r((char *) msg->l3h, "\r\n", &pdata.save); + data = strline_r((char *) msg->l3h, &pdata.save); pdata.found = mgcp_analyze_header(&pdata, data); if (pdata.endp && pdata.trans && pdata.endp->last_trans @@ -544,6 +544,62 @@ static int allocate_ports(struct mgcp_endpoint *endp) return 0; } +static int parse_sdp_data(struct mgcp_rtp_end *rtp, struct mgcp_parse_data *p) +{ + char *line; + int found_media = 0; + + for_each_line(line, p->save) { + switch (line[0]) { + case 'a': + case 'o': + case 's': + case 't': + case 'v': + /* skip these SDP attributes */ + break; + case 'm': { + int port; + int payload; + + if (sscanf(line, "m=audio %d RTP/AVP %d", + &port, &payload) == 2) { + rtp->rtp_port = htons(port); + rtp->rtcp_port = htons(port + 1); + rtp->payload_type = payload; + found_media = 1; + } + break; + } + case 'c': { + char ipv4[16]; + + if (sscanf(line, "c=IN IP4 %15s", ipv4) == 1) { + inet_aton(ipv4, &rtp->addr); + } + break; + } + default: + if (p->endp) + LOGP(DMGCP, LOGL_NOTICE, + "Unhandled SDP option: '%c'/%d on 0x%x\n", + line[0], line[0], ENDPOINT_NUMBER(p->endp)); + else + LOGP(DMGCP, LOGL_NOTICE, + "Unhandled SDP option: '%c'/%d\n", + line[0], line[0]); + break; + } + } + + if (found_media) + LOGP(DMGCP, LOGL_NOTICE, + "Got media info via SDP: port %d, payload %d, addr %s\n", + ntohs(rtp->rtp_port), rtp->payload_type, inet_ntoa(rtp->addr)); + + return found_media; +} + static struct msgb *handle_create_con(struct mgcp_parse_data *p) { struct mgcp_trunk_config *tcfg; @@ -559,7 +615,7 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) return create_err_response(NULL, 510, "CRCX", p->trans); /* parse CallID C: and LocalParameters L: */ - for_each_non_empty_line(line, p->save) { + for_each_line(line, p->save) { switch (line[0]) { case 'L': local_options = (const char *) line + 3; @@ -684,7 +740,7 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) return create_err_response(endp, 400, "MDCX", p->trans); } - for_each_non_empty_line(line, p->save) { + for_each_line(line, p->save) { switch (line[0]) { case 'C': { if (verify_call_id(endp, line + 3) != 0) @@ -711,35 +767,13 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) break; case '\0': /* SDP file begins */ + parse_sdp_data(&endp->net_end, p); + /* This will exhaust p->save, so the loop will + * terminate next time. + */ break; - case 'a': - case 'o': - case 's': - case 't': - case 'v': - /* skip these SDP attributes */ - break; - case 'm': { - int port; - int payload; - - if (sscanf(line, "m=audio %d RTP/AVP %d", &port, &payload) == 2) { - endp->net_end.rtp_port = htons(port); - endp->net_end.rtcp_port = htons(port + 1); - endp->net_end.payload_type = payload; - } - break; - } - case 'c': { - char ipv4[16]; - - if (sscanf(line, "c=IN IP4 %15s", ipv4) == 1) { - inet_aton(ipv4, &endp->net_end.addr); - } - break; - } default: - LOGP(DMGCP, LOGL_NOTICE, "Unhandled option: '%c'/%d on 0x%x\n", + LOGP(DMGCP, LOGL_NOTICE, "Unhandled MGCP option: '%c'/%d on 0x%x\n", line[0], line[0], ENDPOINT_NUMBER(endp)); break; } @@ -803,7 +837,7 @@ static struct msgb *handle_delete_con(struct mgcp_parse_data *p) return create_err_response(endp, 400, "DLCX", p->trans); } - for_each_non_empty_line(line, p->save) { + for_each_line(line, p->save) { switch (line[0]) { case 'C': if (verify_call_id(endp, line + 3) != 0) @@ -905,7 +939,7 @@ static struct msgb *handle_noti_req(struct mgcp_parse_data *p) if (p->found != 0) return create_err_response(NULL, 400, "RQNT", p->trans); - for_each_non_empty_line(line, p->save) { + for_each_line(line, p->save) { switch (line[0]) { case 'S': tone = extract_tone(line); From 9107e2da13ef83ad8a5ae40aabe2fe6bfb816dde Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Tue, 3 Dec 2013 17:14:44 +0100 Subject: [PATCH 5/7] mgcp: NUL-terminate MGCP message The MGCP message isn't always NUL-terminated when arriving at mgcp_handle_message(). This may lead to undefined results. This patch ensures that the message text is NUL-terminated by setting *msg->tail to '\0' in mgcp_handle_message(). Addresses: <000b> mgcp_protocol.c:642 Unhandled option: 'r'/114 on 0x3 <000b> mgcp_protocol.c:593 Unhandled SDP option: '='/61 on 0x3 <000b> mgcp_protocol.c:871 Unhandled option: '.'/46 on 0x2 Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index d4a23a7d7..645b8a75e 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -261,12 +261,27 @@ struct msgb *mgcp_handle_message(struct mgcp_config *cfg, struct msgb *msg) int i, code, handled = 0; struct msgb *resp = NULL; char *data; + unsigned char *tail = msg->l2h + msgb_l2len(msg); /* char after l2 data */ if (msgb_l2len(msg) < 4) { LOGP(DMGCP, LOGL_ERROR, "msg too short: %d\n", msg->len); return NULL; } + /* Ensure that the msg->l2h is NUL terminated. */ + if (tail[-1] == '\0') + /* nothing to do */; + else if (msgb_tailroom(msg) > 0) + tail[0] = '\0'; + else if (tail[-1] == '\r' || tail[-1] == '\n') + tail[-1] = '\0'; + else { + LOGP(DMGCP, LOGL_ERROR, "Cannot NUL terminate MGCP message: " + "Length: %d, Buffer size: %d\n", + msgb_l2len(msg), msg->data_len); + return NULL; + } + /* attempt to treat it as a response */ if (sscanf((const char *)&msg->l2h[0], "%3d %*s", &code) == 1) { LOGP(DMGCP, LOGL_DEBUG, "Response: Code: %d\n", code); @@ -278,7 +293,6 @@ struct msgb *mgcp_handle_message(struct mgcp_config *cfg, struct msgb *msg) /* * Check for a duplicate message and respond. - * FIXME: Verify that the msg->l3h is NULL terminated. */ memset(&pdata, 0, sizeof(pdata)); pdata.cfg = cfg; From 3dff27d38ded0e902c93dbc025f5664c3ac1ccdf Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Fri, 29 Nov 2013 13:43:48 +0100 Subject: [PATCH 6/7] mgcp/nat: Take payload type from SDP data So far the payload type used in RTP streams has been taken from the trunk configuration in NAT mode. This patch changes the implementation to use the payload type announced in the SDP part of MGCP messages and responses. SDP descriptions more than one m=audio line are not yet supported properly (always the last one is taken). Ticket: OW#466 Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/bsc_nat.h | 3 ++- openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c | 17 +++++++++++++---- openbsc/tests/bsc-nat/bsc_nat_test.c | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h index 635b12541..150979b10 100644 --- a/openbsc/include/openbsc/bsc_nat.h +++ b/openbsc/include/openbsc/bsc_nat.h @@ -408,7 +408,8 @@ void bsc_mgcp_free_endpoints(struct bsc_nat *nat); int bsc_mgcp_nat_init(struct bsc_nat *nat); struct nat_sccp_connection *bsc_mgcp_find_con(struct bsc_nat *, int endpoint_number); -struct msgb *bsc_mgcp_rewrite(char *input, int length, int endp, const char *ip, int port); +struct msgb *bsc_mgcp_rewrite(char *input, int length, int endp, const char *ip, + int port, int *payload_type); void bsc_mgcp_forward(struct bsc_connection *bsc, struct msgb *msg); void bsc_mgcp_clear_endpoints_for(struct bsc_connection *bsc); diff --git a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c index 8bb6075d1..3dad39628 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c +++ b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c @@ -542,8 +542,10 @@ 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, - nat->mgcp_cfg->source_addr, mgcp_endp->bts_end.local_port); + bsc_msg = bsc_mgcp_rewrite((char *) nat->mgcp_msg, nat->mgcp_length, + sccp->bsc_endp, nat->mgcp_cfg->source_addr, + mgcp_endp->bts_end.local_port, + &mgcp_endp->net_end.payload_type); if (!bsc_msg) { LOGP(DMGCP, LOGL_ERROR, "Failed to patch the msg.\n"); return MGCP_POLICY_CONT; @@ -683,7 +685,9 @@ void bsc_mgcp_forward(struct bsc_connection *bsc, struct msgb *msg) * with the value of 0 should be no problem. */ output = bsc_mgcp_rewrite((char * ) msg->l2h, msgb_l2len(msg), -1, - bsc->nat->mgcp_cfg->source_addr, endp->net_end.local_port); + bsc->nat->mgcp_cfg->source_addr, + endp->net_end.local_port, + &endp->bts_end.payload_type); if (!output) { LOGP(DMGCP, LOGL_ERROR, "Failed to rewrite MGCP msg.\n"); @@ -743,7 +747,9 @@ static void patch_mgcp(struct msgb *output, const char *op, const char *tok, } /* we need to replace some strings... */ -struct msgb *bsc_mgcp_rewrite(char *input, int length, int endpoint, const char *ip, int port) +struct msgb *bsc_mgcp_rewrite(char *input, int length, int endpoint, + const char *ip, int port, + int *payload_type) { static const char crcx_str[] = "CRCX "; static const char dlcx_str[] = "DLCX "; @@ -836,6 +842,9 @@ copy: memcpy(output->l3h, buf, strlen(buf)); } + if (payload != -1 && payload_type) + *payload_type = payload; + return output; } diff --git a/openbsc/tests/bsc-nat/bsc_nat_test.c b/openbsc/tests/bsc-nat/bsc_nat_test.c index 3320e0694..a121c8ab3 100644 --- a/openbsc/tests/bsc-nat/bsc_nat_test.c +++ b/openbsc/tests/bsc-nat/bsc_nat_test.c @@ -630,7 +630,7 @@ static void test_mgcp_rewrite(void) char *input = strdup(orig); output = bsc_mgcp_rewrite(input, strlen(input), 0x1e, - ip, port); + ip, port, &payload_type); if (payload_type != -1) { fprintf(stderr, "Found media payload type %d in SDP data\n", From 78a9501cfda39c8413c6698a063a08405521160c Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Fri, 29 Nov 2013 13:43:49 +0100 Subject: [PATCH 7/7] mgcp: Handle SDP in CRCX received by the MGW So far the SDP part of the CRCX message has been ignored by the MGW. This patch adds SDP parsing for this case, eventually updating the net end's payload type and connection parameters. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 9 +++++++++ openbsc/tests/mgcp/mgcp_test.c | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 645b8a75e..a0b905de0 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -624,6 +624,7 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) const char *callid = NULL; const char *mode = NULL; char *line; + int have_sdp = 0; if (p->found != 0) return create_err_response(NULL, 510, "CRCX", p->trans); @@ -640,6 +641,9 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) case 'M': mode = (const char *) line + 3; break; + case '\0': + have_sdp = 1; + goto mgcp_header_done; default: LOGP(DMGCP, LOGL_NOTICE, "Unhandled option: '%c'/%d on 0x%x\n", *line, *line, ENDPOINT_NUMBER(endp)); @@ -647,6 +651,7 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) } } +mgcp_header_done: tcfg = p->endp->tcfg; /* Check required data */ @@ -697,9 +702,13 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) goto error2; endp->allocated = 1; + + /* set up RTP media parameters */ endp->bts_end.payload_type = tcfg->audio_payload; endp->bts_end.fmtp_extra = talloc_strdup(tcfg->endpoints, tcfg->audio_fmtp_extra); + if (have_sdp) + parse_sdp_data(&endp->net_end, p); /* policy CB */ if (p->cfg->policy_cb) { diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 0aebb4c55..c58f52d9e 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -182,11 +182,11 @@ static const struct mgcp_test tests[] = { { "AUEP2", AUEP2, AUEP2_RET }, { "MDCX1", MDCX_WRONG_EP, MDCX_ERR_RET }, { "MDCX2", MDCX_UNALLOCATED, MDCX_RET }, - { "CRCX", CRCX, CRCX_RET, PTYPE_NYI, 126 }, + { "CRCX", CRCX, CRCX_RET, 97, 126 }, { "MDCX3", MDCX3, MDCX3_RET, PTYPE_NONE, 126 }, { "MDCX4", MDCX4, MDCX4_RET, 99, 126 }, { "DLCX", DLCX, DLCX_RET, -1, -1 }, - { "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET, PTYPE_NYI, 126 }, + { "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET, 97, 126 }, { "EMPTY", EMPTY, EMPTY_RET }, { "SHORT1", SHORT, SHORT_RET }, { "SHORT2", SHORT2, SHORT2_RET },