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 7096495..c040ab1 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -36,7 +36,7 @@ #include <openbsc/mgcp.h> #include <openbsc/mgcp_internal.h>
-#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",
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 c040ab1..19a6f53 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 d36aaa8..362f029 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -25,6 +25,40 @@ #include <string.h> #include <limits.h>
+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 5666424..429e0df 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
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 5a76689..d1f8ebc 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 5158f46..3320e06 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 362f029..0aebb4c 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 429e0df..3bfd78b 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
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 19a6f53..d4a23a7 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);
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 | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index d4a23a7..44c93f7 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -262,6 +262,18 @@ struct msgb *mgcp_handle_message(struct mgcp_config *cfg, struct msgb *msg) struct msgb *resp = NULL; char *data;
+ /* Ensure that the msg->l2h is NULL terminated. */ + if (msgb_tailroom(msg) > 0) + *msg->tail = '\0'; + else if (*(msg->tail-1) == '\r' || *(msg->tail-1) == '\n') + *(msg->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; + } + if (msgb_l2len(msg) < 4) { LOGP(DMGCP, LOGL_ERROR, "msg too short: %d\n", msg->len); return NULL; @@ -278,7 +290,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;
On Fri, Nov 29, 2013 at 01:43:47PM +0100, Jacob Erlbeck wrote:
The MGCP message isn't always NUL-terminated when arriving at mgcp_handle_message(). This may lead to undefined results.
oh!
- /* Ensure that the msg->l2h is NULL terminated. */
- if (msgb_tailroom(msg) > 0)
*msg->tail = '\0';- else if (*(msg->tail-1) == '\r' || *(msg->tail-1) == '\n')
*(msg->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;- }
The check misses if "tail - 1" is already \0 and if tail - 1 is not NULL. I would just add an OSMO_ASSERT and fix the caller that didn't null terminate?! What do you think?
On 11/30/2013 07:16 PM, Holger Hans Peter Freyther wrote:
On Fri, Nov 29, 2013 at 01:43:47PM +0100, Jacob Erlbeck wrote:
The MGCP message isn't always NUL-terminated when arriving at mgcp_handle_message(). This may lead to undefined results.
oh!
- /* Ensure that the msg->l2h is NULL terminated. */
- if (msgb_tailroom(msg) > 0)
*msg->tail = '\0';- else if (*(msg->tail-1) == '\r' || *(msg->tail-1) == '\n')
*(msg->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;- }
The check misses if "tail - 1" is already \0
Yes, but does this hurt? I wouldn't expect l2 to end with a \0 since the protocol is defined without one (see below). And even if there was one, a second one wouldn't be a problem.
and if tail - 1 is not NULL.
You mean that tail == (uint8_t *)1 ??? This mustn't happen anyway.
I would just add an OSMO_ASSERT and fix the caller that didn't
null terminate?! What do you think?
I don't agree that the caller has to NUL-terminate. It is an implementation detail of mgcp_handle_message() that it uses string parsing functions that depend on a terminating \0 character. The caller provides the length of the data area and that just fine. Even more it reserves a byte at the end of the buffer to have space for a \0, which is more than the protocol requires. And it guarantees this way, that the first condition (tailroom > 0) always succeeds, which is much better IMO than inserting protocol violating garbage characters (the \0) at the end of the buffer within the l2 data area.
What's about changing msgb to always write a \0 after the data area every time tail gets modified? Like with QByteArray you could always safely use string functions without handling this explicitely. But I'm afraid there is some code already that relies of changing tail freely without expecting this to cause this kind of side effect even when used with put. Perhaps one can make this optional?
Jacob
On Mon, Dec 02, 2013 at 10:21:58AM +0100, Jacob Erlbeck wrote:
- /* Ensure that the msg->l2h is NULL terminated. */
- if (msgb_tailroom(msg) > 0)
*msg->tail = '\0';- else if (*(msg->tail-1) == '\r' || *(msg->tail-1) == '\n')
*(msg->tail - 1) = '\0';- else {
return NULL;- }
The check misses if "tail - 1" is already \0
Yes, but does this hurt? I wouldn't expect l2 to end with a \0 since the protocol is defined without one (see below). And even if there was one, a second one wouldn't be a problem.
tailroom == 0 AND msg->l2h[msgb_length(msg) - 1] == '\0' is rejected by your above code.
You mean that tail == (uint8_t *)1 ??? This mustn't happen anyway.
true. I had tail not being initialized in mind. But that can not happen.
I don't agree that the caller has to NUL-terminate. It is an implementation detail of mgcp_handle_message() that it uses string parsing functions that depend on a terminating \0 character. The
Fair enough. I just noticed that the check above is a bit of code and I think it missed one case, so a OSMO_ASSERT is easier to get right.
What's about changing msgb to always write a \0 after the data area every time tail gets modified? Like with QByteArray you could always safely use string functions without handling this explicitely. But I'm afraid there is some code already that relies of changing tail freely without expecting this to cause this kind of side effect even when used with put. Perhaps one can make this optional?
In theory we always over allocate the msgb and due the usage of talloc_zero should make sure that we have many NULLs at the end. If we want to implement the extra reservation we have a bit of a problem. We generally use a power of two for the allocation (in the hope we get a full page). So if we do:
+1: We certainly need to allocate more than a page -1: We might not have enough data.
For now. Please change the check to use l2h, msgb_length and handle a NULL terminated input with no tailroom.
kind regards holger
On 12/02/2013 11:14 AM, Holger Hans Peter Freyther wrote:
On Mon, Dec 02, 2013 at 10:21:58AM +0100, Jacob Erlbeck wrote:
- /* Ensure that the msg->l2h is NULL terminated. */
- if (msgb_tailroom(msg) > 0)
*msg->tail = '\0';- else if (*(msg->tail-1) == '\r' || *(msg->tail-1) == '\n')
*(msg->tail - 1) = '\0';- else {
return NULL;- }
The check misses if "tail - 1" is already \0
Yes, but does this hurt? I wouldn't expect l2 to end with a \0 since the protocol is defined without one (see below). And even if there was one, a second one wouldn't be a problem.
tailroom == 0 AND msg->l2h[msgb_length(msg) - 1] == '\0' is rejected by your above code.
Ok, agreed. And to be complete, the test of the length of the data arae being > 0 is missing, too.
So I'll add sth like:
else if (msgb_l2len(msg) > 0 && *(msg->tail-1) == '\0') /* do nothing */ else ... '\r' ...
Or do you rather recommend to use msg->l2h + msgb_l2len(msg) instead of msg->tail directly?
And why do you use msgb_length() and not msgb_l2len()?
Jacob
On Mon, Dec 02, 2013 at 11:38:27AM +0100, Jacob Erlbeck wrote:
Or do you rather recommend to use msg->l2h + msgb_l2len(msg) instead of msg->tail directly?
I think using msg->l2h + msgb_l2len is the correct thing to use. There could be other information in the message (yes, I strtok and other things I did to the message would break things)
And why do you use msgb_length() and not msgb_l2len()?
Please us l2len. My example was wrong. Thanks for noticing that!
On 12/02/2013 11:14 AM, Holger Hans Peter Freyther wrote:
On Mon, Dec 02, 2013 at 10:21:58AM +0100, Jacob Erlbeck wrote:
What's about changing msgb to always write a \0 after the data area every time tail gets modified? Like with QByteArray you could always safely use string functions without handling this explicitely. But I'm afraid there is some code already that relies of changing tail freely without expecting this to cause this kind of side effect even when used with put. Perhaps one can make this optional?
In theory we always over allocate the msgb and due the usage of talloc_zero should make sure that we have many NULLs at the end.
The other problem here is, that the buffer gets reused for each MGCP message, so we always have NUL-terminated strings (which made valgrind at al happy all the time) but occasionally with garbage at the end.
Jacob
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 | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index d4a23a7..3487410 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -261,6 +261,19 @@ 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 */ + + /* Ensure that the msg->l2h is NULL terminated. */ + if (msgb_tailroom(msg) > 0) + *tail = '\0'; + else if (msgb_l2len(msg) > 0 && (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; + }
if (msgb_l2len(msg) < 4) { LOGP(DMGCP, LOGL_ERROR, "msg too short: %d\n", msg->len); @@ -278,7 +291,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;
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 635b125..150979b 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 8bb6075..3dad396 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 3320e06..a121c8a 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",
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 44c93f7..270f4bc 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -621,6 +621,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); @@ -637,6 +638,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)); @@ -644,6 +648,7 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p) } }
+mgcp_header_done: tcfg = p->endp->tcfg;
/* Check required data */ @@ -694,9 +699,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 0aebb4c..c58f52d 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 },
The tsdelta computation and error detection didn't handle the intialisation phase properly.
This patches fixes this by skipping the output timing validation for the first packet.
Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_network.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 75d39c1..1ce9cc2 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -219,6 +219,7 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta uint32_t timestamp; struct rtp_hdr *rtp_hdr; int payload = rtp_end->payload_type; + int initializing = 0;
if (len < sizeof(*rtp_hdr)) return; @@ -229,6 +230,7 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta arrival_time = get_current_ts();
if (!state->initialized) { + initializing = 1; state->in_stream.last_seq = seq - 1; state->in_stream.ssrc = state->orig_ssrc = rtp_hdr->ssrc; state->in_stream.last_tsdelta = 0; @@ -237,6 +239,14 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta state->jitter = 0; state->transit = arrival_time - timestamp; state->out_stream = state->in_stream; + state->out_stream.last_timestamp = timestamp; + LOGP(DMGCP, LOGL_INFO, + "Initializing stream on 0x%x SSRC: %u timestamp: %u " + "from %s:%d in %d\n", + ENDPOINT_NUMBER(endp), state->in_stream.ssrc, + state->seq_offset, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); } else if (state->in_stream.ssrc != rtp_hdr->ssrc) { int32_t tsdelta = state->out_stream.last_tsdelta; if (tsdelta == 0) { @@ -286,9 +296,10 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta }
/* Check again, whether the timestamps are still valid */ - check_rtp_timestamp(endp, &state->out_stream, rtp_end, addr, - seq, timestamp, "output", - &state->out_stream.last_tsdelta); + if (!initializing) + check_rtp_timestamp(endp, &state->out_stream, rtp_end, addr, + seq, timestamp, "output", + &state->out_stream.last_tsdelta);
/* * The below takes the shape of the validation from Appendix A. Check
Hi Holger,
please do not apply this one. It will most likely be solved differently soon.
Jacob
On 11/29/2013 01:43 PM, Jacob Erlbeck wrote:
The tsdelta computation and error detection didn't handle the intialisation phase properly.
This patches fixes this by skipping the output timing validation for the first packet.
Sponsored-by: On-Waves ehf
On Fri, Nov 29, 2013 at 01:43:43PM +0100, Jacob Erlbeck wrote:
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.
Do you want to remove the "case '\0':" lines (just to add them in later commits again)?
-
Holger Hans Peter Freyther -
Jacob Erlbeck