Merge branch 'jerlbeck/mgcp-cleanups'

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);

openbsc/src/libmgcp/mgcp_protocol.c

@@ -36,10 +36,42 @@
 #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))
 
+#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
@@ -229,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);
@@ -246,11 +293,10 @@ 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;
-	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
@@ -512,6 +558,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;
@@ -522,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);
@@ -538,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));
@@ -545,6 +651,7 @@ static struct msgb *handle_create_con(struct mgcp_parse_data *p)
 		}
 	}
 
+mgcp_header_done:
 	tcfg = p->endp->tcfg;
 
 	/* Check required data */
@@ -595,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) {
@@ -679,35 +790,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;
 		}
@@ -1218,7 +1307,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",

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,
+	bsc_msg = bsc_mgcp_rewrite((char *) nat->mgcp_msg, nat->mgcp_length,
-				   nat->mgcp_cfg->source_addr, mgcp_endp->bts_end.local_port);
+				   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;
 }
 

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,
 	},
 };
 

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, &payload_type);
+
+		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);

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"
@@ -48,6 +82,26 @@
 		 "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"
@@ -111,10 +165,16 @@
 #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[] = {
@@ -122,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 },
+	{ "CRCX", CRCX, CRCX_RET, 97, 126 },
-	{ "MDCX3", MDCX3, MDCX3_RET },
+	{ "MDCX3", MDCX3, MDCX3_RET, PTYPE_NONE, 126 },
-	{ "DLCX", DLCX, DLCX_RET },
+	{ "MDCX4", MDCX4, MDCX4_RET, 99, 126 },
-	{ "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET },
+	{ "DLCX", DLCX, DLCX_RET, -1, -1 },
+	{ "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET, 97, 126 },
 	{ "EMPTY", EMPTY, EMPTY_RET },
 	{ "SHORT1", SHORT, SHORT_RET },
 	{ "SHORT2", SHORT2, SHORT2_RET },
@@ -133,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[] = {
@@ -154,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();
@@ -164,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;
@@ -173,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);
@@ -182,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);
@@ -463,6 +569,7 @@ int main(int argc, char **argv)
 {
 	osmo_init_logging(&log_info);
 
+	test_strline();
 	test_messages();
 	test_retransmission();
 	test_packet_loss_calc();

openbsc/tests/mgcp/mgcp_test.ok

@@ -1,9 +1,23 @@
+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
 Testing MDCX2
 Testing CRCX
 Testing MDCX3
+Testing MDCX4
 Testing DLCX
 Testing CRCX_ZYN
 Testing EMPTY