Change in openbsc[master]: nat: Allocate bsc_nat_parsed on the stack instead of heap

Harald Welte gerrit-no-reply at lists.osmocom.org
Tue May 21 10:32:35 UTC 2019


Harald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/13843 )

Change subject: nat: Allocate bsc_nat_parsed on the stack instead of heap
......................................................................

nat: Allocate bsc_nat_parsed on the stack instead of heap

There's no real need to allocate it using talloc. Allocating it on the
stack simplifies the code, avoids mem leaks and makes it faster.

Change-Id: I66c44890952339f15131081e2f629a2824b6d3ba
---
M openbsc/include/openbsc/bsc_nat.h
M openbsc/src/osmo-bsc_nat/bsc_filter.c
M openbsc/src/osmo-bsc_nat/bsc_nat.c
M openbsc/src/osmo-bsc_nat/bsc_nat_rewrite.c
M openbsc/src/osmo-bsc_nat/bsc_ussd.c
M openbsc/tests/bsc-nat/bsc_nat_test.c
6 files changed, 136 insertions(+), 174 deletions(-)

Approvals:
  Daniel Willmann: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h
index 3eba70d..86134be 100644
--- a/openbsc/include/openbsc/bsc_nat.h
+++ b/openbsc/include/openbsc/bsc_nat.h
@@ -356,10 +356,7 @@
 
 const char *bsc_con_type_to_string(int type);
 
-/**
- * parse the given message into the above structure
- */
-struct bsc_nat_parsed *bsc_nat_parse(struct msgb *msg);
+int bsc_nat_parse(struct msgb *msg, struct bsc_nat_parsed *parsed);
 
 /**
  * filter based on IP Access header in both directions
diff --git a/openbsc/src/osmo-bsc_nat/bsc_filter.c b/openbsc/src/osmo-bsc_nat/bsc_filter.c
index 432529e..a878682 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_filter.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_filter.c
@@ -72,19 +72,24 @@
 	{ IPAC_PROTO_MGCP_OLD, ALLOW_ANY, ALLOW_ANY, ALLOW_ANY, FILTER_TO_BOTH },
 };
 
-struct bsc_nat_parsed *bsc_nat_parse(struct msgb *msg)
+ /*! Parse the given message into the parsed structure.
+  *  \param[in] msg the IPA message to parse
+  *  \param[out] parsed the structure to fill with parsed values
+  *  \returns 0 on success, negative on error
+  */
+int bsc_nat_parse(struct msgb *msg, struct bsc_nat_parsed *parsed)
 {
 	struct sccp_parse_result result;
-	struct bsc_nat_parsed *parsed;
 	struct ipaccess_head *hh;
 
 	/* quick fail */
 	if (msg->len < 4)
-		return NULL;
+		return -1;
 
-	parsed = talloc_zero(msg, struct bsc_nat_parsed);
 	if (!parsed)
-		return NULL;
+		return -1;
+
+	memset(parsed, 0, sizeof(*parsed));
 
 	/* more init */
 	parsed->ipa_proto = parsed->called_ssn = parsed->calling_ssn = -1;
@@ -99,22 +104,19 @@
 	/* do a size check on the input */
 	if (ntohs(hh->len) != msgb_l2len(msg)) {
 		LOGP(DLINP, LOGL_ERROR, "Wrong input length?\n");
-		talloc_free(parsed);
-		return NULL;
+		return -1;
 	}
 
 	/* analyze sccp down here */
 	if (parsed->ipa_proto == IPAC_PROTO_SCCP) {
 		memset(&result, 0, sizeof(result));
 		if (sccp_parse_header(msg, &result) != 0) {
-			talloc_free(parsed);
-			return 0;
+			return -1;
 		}
 
 		if (msg->l3h && msgb_l3len(msg) < 3) {
 			LOGP(DNAT, LOGL_ERROR, "Not enough space or GSM payload\n");
-			talloc_free(parsed);
-			return 0;
+			return -1;
 		}
 
 		parsed->sccp_type = sccp_determine_msg_type(msg);
@@ -132,7 +134,7 @@
 		}
 	}
 
-	return parsed;
+	return 0;
 }
 
 /* Returns 0 if message is whitelisted (has to beforwarded by bsc-nat), 1 if
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c
index 30e4b34..2e2eac0 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c
@@ -696,24 +696,23 @@
 {
 	struct nat_sccp_connection *con = NULL;
 	struct bsc_connection *bsc;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	int proto;
 
 	/* filter, drop, patch the message? */
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		LOGP(DNAT, LOGL_ERROR, "Can not parse msg from BSC.\n");
 		return -1;
 	}
 
-	if (bsc_nat_filter_ipa(DIR_BSC, msg, parsed))
+	if (bsc_nat_filter_ipa(DIR_BSC, msg, &parsed))
 		goto exit;
 
-	proto = parsed->ipa_proto;
+	proto = parsed.ipa_proto;
 
 	/* Route and modify the SCCP packet */
 	if (proto == IPAC_PROTO_SCCP) {
-		switch (parsed->sccp_type) {
+		switch (parsed.sccp_type) {
 		case SCCP_MSG_TYPE_UDT:
 			/* forward UDT messages to every BSC */
 			goto send_to_all;
@@ -722,8 +721,8 @@
 		case SCCP_MSG_TYPE_CREF:
 		case SCCP_MSG_TYPE_DT1:
 		case SCCP_MSG_TYPE_IT:
-			con = patch_sccp_src_ref_to_bsc(msg, parsed, nat);
-			if (parsed->gsm_type == BSS_MAP_MSG_ASSIGMENT_RQST) {
+			con = patch_sccp_src_ref_to_bsc(msg, &parsed, nat);
+			if (parsed.gsm_type == BSS_MAP_MSG_ASSIGMENT_RQST) {
 				osmo_counter_inc(nat->stats.sccp.calls);
 
 				if (con) {
@@ -735,14 +734,14 @@
 				} else
 					LOGP(DNAT, LOGL_ERROR, "Assignment command but no BSC.\n");
 			} else if (con && con->con_local == NAT_CON_END_USSD &&
-				   parsed->gsm_type == BSS_MAP_MSG_CLEAR_CMD) {
+				   parsed.gsm_type == BSS_MAP_MSG_CLEAR_CMD) {
 				LOGP(DNAT, LOGL_NOTICE, "Clear Command for USSD Connection. Ignoring.\n");
 				con = NULL;
 			}
 			break;
 		case SCCP_MSG_TYPE_CC:
-			con = patch_sccp_src_ref_to_bsc(msg, parsed, nat);
-			if (!con || update_sccp_src_ref(con, parsed) != 0)
+			con = patch_sccp_src_ref_to_bsc(msg, &parsed, nat);
+			if (!con || update_sccp_src_ref(con, &parsed) != 0)
 				goto exit;
 			break;
 		case SCCP_MSG_TYPE_RLC:
@@ -755,27 +754,24 @@
 			goto exit;
 		}
 
-		if (!con && parsed->sccp_type == SCCP_MSG_TYPE_RLSD) {
+		if (!con && parsed.sccp_type == SCCP_MSG_TYPE_RLSD) {
 			LOGP(DNAT, LOGL_NOTICE, "Sending fake RLC on RLSD message to network.\n");
 			/* Exchange src/dest for the reply */
-			nat_send_rlc(msc_con, &parsed->original_dest_ref,
-					parsed->src_local_ref);
+			nat_send_rlc(msc_con, &parsed.original_dest_ref,
+					parsed.src_local_ref);
 		} else if (!con)
-			LOGP(DNAT, LOGL_ERROR, "Unknown connection for msg type: 0x%x from the MSC.\n", parsed->sccp_type);
+			LOGP(DNAT, LOGL_ERROR, "Unknown connection for msg type: 0x%x from the MSC.\n", parsed.sccp_type);
 	}
 
-	if (!con) {
-		talloc_free(parsed);
+	if (!con)
 		return -1;
-	}
+
 	if (!con->bsc->authenticated) {
-		talloc_free(parsed);
 		LOGP(DNAT, LOGL_ERROR, "Selected BSC not authenticated.\n");
 		return -1;
 	}
 
-	update_con_authorize(con, parsed, msg);
-	talloc_free(parsed);
+	update_con_authorize(con, &parsed, msg);
 
 	bsc_send_data(con->bsc, msg->l2h, msgb_l2len(msg), proto);
 	return 0;
@@ -786,7 +782,7 @@
 	 * Command to every BSC in our network. We will analys the PAGING
 	 * message and then send it to the authenticated messages...
 	 */
-	if (parsed->ipa_proto == IPAC_PROTO_SCCP && parsed->gsm_type == BSS_MAP_MSG_PAGING) {
+	if (parsed.ipa_proto == IPAC_PROTO_SCCP && parsed.gsm_type == BSS_MAP_MSG_PAGING) {
 		bsc_nat_handle_paging(nat, msg);
 		goto exit;
 	}
@@ -795,11 +791,10 @@
 		if (!bsc->authenticated)
 			continue;
 
-		bsc_send_data(bsc, msg->l2h, msgb_l2len(msg), parsed->ipa_proto);
+		bsc_send_data(bsc, msg->l2h, msgb_l2len(msg), parsed.ipa_proto);
 	}
 
 exit:
-	talloc_free(parsed);
 	return 0;
 }
 
@@ -1130,19 +1125,19 @@
 	struct bsc_msc_connection *con_msc = NULL;
 	struct bsc_connection *con_bsc = NULL;
 	int con_type;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	struct bsc_filter_reject_cause cause;
 	*bsc_conn_closed = false;
 
 	/* Parse and filter messages */
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	bool parsed_ok = bsc_nat_parse(msg, &parsed) == 0;
+	if (!parsed_ok) {
 		LOGP(DNAT, LOGL_ERROR, "Can not parse msg from BSC.\n");
 		msgb_free(msg);
 		return -1;
 	}
 
-	if (bsc_nat_filter_ipa(DIR_MSC, msg, parsed))
+	if (bsc_nat_filter_ipa(DIR_MSC, msg, &parsed))
 		goto exit;
 
 	/*
@@ -1157,32 +1152,32 @@
 
 
 	/* modify the SCCP entries */
-	if (parsed->ipa_proto == IPAC_PROTO_SCCP) {
+	if (parsed.ipa_proto == IPAC_PROTO_SCCP) {
 		int filter;
 		struct nat_sccp_connection *con;
-		switch (parsed->sccp_type) {
+		switch (parsed.sccp_type) {
 		case SCCP_MSG_TYPE_CR:
 			memset(&cause, 0, sizeof(cause));
-			filter = bsc_nat_filter_sccp_cr(bsc, msg, parsed,
+			filter = bsc_nat_filter_sccp_cr(bsc, msg, &parsed,
 						&con_type, &imsi, &cause);
 			if (filter < 0) {
 				if (imsi)
 					bsc_nat_inform_reject(bsc, imsi);
 				bsc_stat_reject(filter, bsc, 0);
 				/* send a SCCP Connection Refused */
-				bsc_send_con_refuse(bsc, parsed, con_type, &cause);
+				bsc_send_con_refuse(bsc, &parsed, con_type, &cause);
 				goto exit2;
 			}
 
-			if (!create_sccp_src_ref(bsc, parsed))
+			if (!create_sccp_src_ref(bsc, &parsed))
 				goto exit2;
-			con = patch_sccp_src_ref_to_msc(msg, parsed, bsc);
+			con = patch_sccp_src_ref_to_msc(msg, &parsed, bsc);
 			OSMO_ASSERT(con);
 			con->msc_con = bsc->nat->msc_con;
 			con_msc = con->msc_con;
 			con->filter_state.con_type = con_type;
 			con->filter_state.imsi_checked = filter;
-			bsc_nat_extract_lac(bsc, con, parsed, msg);
+			bsc_nat_extract_lac(bsc, con, &parsed, msg);
 			if (imsi)
 				con->filter_state.imsi = talloc_steal(con, imsi);
 			imsi = NULL;
@@ -1194,13 +1189,13 @@
 		case SCCP_MSG_TYPE_DT1:
 		case SCCP_MSG_TYPE_CC:
 		case SCCP_MSG_TYPE_IT:
-			con = patch_sccp_src_ref_to_msc(msg, parsed, bsc);
+			con = patch_sccp_src_ref_to_msc(msg, &parsed, bsc);
 			if (con) {
 				/* only filter non local connections */
 				if (!con->con_local) {
 					memset(&cause, 0, sizeof(cause));
 					filter = bsc_nat_filter_dt(bsc, msg,
-							con, parsed, &cause);
+							con, &parsed, &cause);
 					if (filter < 0) {
 						if (con->filter_state.imsi)
 							bsc_nat_inform_reject(bsc,
@@ -1212,20 +1207,19 @@
 					}
 
 					/* hand data to a side channel */
-					if (bsc_ussd_check(con, parsed, msg) == 1)
+					if (bsc_ussd_check(con, &parsed, msg) == 1)
 						con->con_local = NAT_CON_END_USSD;
 
 					/*
 					 * Optionally rewrite setup message. This can
-					 * replace the msg and the parsed structure becomes
-					 * invalid.
+					 * replace the msg and hence data in struct parsed
+					 * becomes invalid.
 					 */
-					msg = bsc_nat_rewrite_msg(bsc->nat, msg, parsed,
+					msg = bsc_nat_rewrite_msg(bsc->nat, msg, &parsed,
 									con->filter_state.imsi);
-					talloc_free(parsed);
-					parsed = NULL;
+					parsed_ok = false;
 				} else if (con->con_local == NAT_CON_END_USSD) {
-					bsc_ussd_check(con, parsed, msg);
+					bsc_ussd_check(con, &parsed, msg);
 				}
 
 				con_bsc = con->bsc;
@@ -1235,13 +1229,13 @@
 
 			break;
 		case SCCP_MSG_TYPE_RLC:
-			con = patch_sccp_src_ref_to_msc(msg, parsed, bsc);
+			con = patch_sccp_src_ref_to_msc(msg, &parsed, bsc);
 			if (con) {
 				con_bsc = con->bsc;
 				con_msc = con->msc_con;
 				con_filter = con->con_local;
 			}
-			remove_sccp_src_ref(bsc, msg, parsed);
+			remove_sccp_src_ref(bsc, msg, &parsed);
 			*bsc_conn_closed = bsc_maybe_close(bsc);
 			break;
 		case SCCP_MSG_TYPE_UDT:
@@ -1249,16 +1243,16 @@
 			con = NULL;
 			break;
 		default:
-			LOGP(DNAT, LOGL_ERROR, "Not forwarding to msc sccp type: 0x%x\n", parsed->sccp_type);
+			LOGP(DNAT, LOGL_ERROR, "Not forwarding to msc sccp type: 0x%x\n", parsed.sccp_type);
 			con = NULL;
 			goto exit2;
 			break;
 		}
-        } else if (parsed->ipa_proto == IPAC_PROTO_MGCP_OLD) {
+        } else if (parsed.ipa_proto == IPAC_PROTO_MGCP_OLD) {
                 bsc_mgcp_forward(bsc, msg);
                 goto exit2;
 	} else {
-		LOGP(DNAT, LOGL_ERROR, "Not forwarding unknown stream id: 0x%x\n", parsed->ipa_proto);
+		LOGP(DNAT, LOGL_ERROR, "Not forwarding unknown stream id: 0x%x\n", parsed.ipa_proto);
 		goto exit2;
 	}
 
@@ -1275,22 +1269,21 @@
 	if (!con_msc) {
 		LOGP(DNAT, LOGL_ERROR, "Not forwarding data bsc_nr: %d ipa: %d type: 0x%x\n",
 			bsc->cfg->nr,
-			parsed ? parsed->ipa_proto : -1,
-			parsed ? parsed->sccp_type : -1);
+			parsed_ok ? parsed.ipa_proto : -1,
+			parsed_ok ? parsed.sccp_type : -1);
 		goto exit2;
 	}
 
 	/* send the non-filtered but maybe modified msg */
-	talloc_free(parsed);
 	queue_for_msc(con_msc, msg);
 
 	return 0;
 
 exit:
 	/* if we filter out the reset send an ack to the BSC */
-	if (parsed->bssap == 0 && parsed->gsm_type == BSS_MAP_MSG_RESET) {
+	if (parsed.bssap == 0 && parsed.gsm_type == BSS_MAP_MSG_RESET) {
 		send_reset_ack(bsc);
-	} else if (parsed->ipa_proto == IPAC_PROTO_IPACCESS) {
+	} else if (parsed.ipa_proto == IPAC_PROTO_IPACCESS) {
 		/* do we know who is handling this? */
 		if (msg->l2h[0] == IPAC_MSGT_ID_RESP && msgb_l2len(msg) > 2) {
 			struct tlv_parsed tvp;
@@ -1315,7 +1308,6 @@
 exit2:
 	if (imsi)
 		talloc_free(imsi);
-	talloc_free(parsed);
 	msgb_free(msg);
 	return -1;
 }
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_rewrite.c b/openbsc/src/osmo-bsc_nat/bsc_nat_rewrite.c
index e7c387c..1dd2ada 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat_rewrite.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat_rewrite.c
@@ -618,8 +618,6 @@
 
 	ipa_prepend_header(sccp, IPAC_PROTO_SCCP);
 
-	/* the parsed hangs off from msg but it needs to survive */
-	talloc_steal(sccp, parsed);
 	msgb_free(msg);
 	return sccp;
 }
diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c
index dea1807..d44b1b2 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c
@@ -90,30 +90,27 @@
 static int forward_sccp(struct bsc_nat *nat, struct msgb *msg)
 {
 	struct nat_sccp_connection *con;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 
-
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		LOGP(DNAT, LOGL_ERROR, "Can not parse msg from USSD.\n");
 		msgb_free(msg);
 		return -1;
 	}
 
-	if (!parsed->dest_local_ref) {
+	if (!parsed.dest_local_ref) {
 		LOGP(DNAT, LOGL_ERROR, "No destination local reference.\n");
 		msgb_free(msg);
 		return -1;
 	}
 
-	con = bsc_nat_find_con_by_bsc(nat, parsed->dest_local_ref);
+	con = bsc_nat_find_con_by_bsc(nat, parsed.dest_local_ref);
 	if (!con || !con->bsc) {
 		LOGP(DNAT, LOGL_ERROR, "No active connection found.\n");
 		msgb_free(msg);
 		return -1;
 	}
 
-	talloc_free(parsed);
 	bsc_write_msg(&con->bsc->write_queue, msg);
 	return 0;
 }
diff --git a/openbsc/tests/bsc-nat/bsc_nat_test.c b/openbsc/tests/bsc-nat/bsc_nat_test.c
index e0d0051..4bee60d 100644
--- a/openbsc/tests/bsc-nat/bsc_nat_test.c
+++ b/openbsc/tests/bsc-nat/bsc_nat_test.c
@@ -235,20 +235,19 @@
 	printf("Testing BSS Filtering.\n");
 	for (i = 0; i < ARRAY_SIZE(results); ++i) {
 		int result;
-		struct bsc_nat_parsed *parsed;
+		struct bsc_nat_parsed parsed;
 		struct msgb *msg = msgb_alloc(4096, "test-message");
 
 		printf("Going to test item: %d\n", i);
 		memcpy(msg->data, results[i].data, results[i].length);
 		msg->l2h = msgb_put(msg, results[i].length);
 
-		parsed = bsc_nat_parse(msg);
-		if (!parsed) {
+		if (bsc_nat_parse(msg, &parsed) < 0) {
 			printf("FAIL: Failed to parse the message\n");
 			continue;
 		}
 
-		result = bsc_nat_filter_ipa(results[i].dir, msg, parsed);
+		result = bsc_nat_filter_ipa(results[i].dir, msg, &parsed);
 		if (result != results[i].result) {
 			printf("FAIL: Not the expected result got: %d wanted: %d\n",
 				result, results[i].result);
@@ -310,7 +309,7 @@
 	struct bsc_connection *con;
 	struct nat_sccp_connection *con_found;
 	struct nat_sccp_connection *rc_con;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	struct msgb *msg;
 
 	printf("Testing connection tracking.\n");
@@ -326,19 +325,19 @@
 
 	/* 1.) create a connection */
 	copy_to_msg(msg, bsc_cr, sizeof(bsc_cr));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_msc(msg, parsed, con);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_msc(msg, &parsed, con);
 	if (con_found != NULL) {
 		printf("Con should not exist realref(%u)\n",
 		       sccp_src_ref_to_int(&con_found->real_ref));
 		abort();
 	}
-	rc_con = create_sccp_src_ref(con, parsed);
+	rc_con = create_sccp_src_ref(con, &parsed);
 	if (!rc_con) {
 		printf("Failed to create a ref\n");
 		abort();
 	}
-	con_found = patch_sccp_src_ref_to_msc(msg, parsed, con);
+	con_found = patch_sccp_src_ref_to_msc(msg, &parsed, con);
 	if (!con_found) {
 		printf("Failed to find connection.\n");
 		abort();
@@ -356,40 +355,39 @@
 		printf("Failed to patch the BSC CR msg.\n");
 		abort();
 	}
-	talloc_free(parsed);
 
 	/* 2.) get the cc */
 	copy_to_msg(msg, msc_cc, sizeof(msc_cc));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_bsc(msg, parsed, nat);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_bsc(msg, &parsed, nat);
 	VERIFY(con_found, con, msg, msc_cc_patched, "MSC CC");
-	if (update_sccp_src_ref(con_found, parsed) != 0) {
+	if (update_sccp_src_ref(con_found, &parsed) != 0) {
 		printf("Failed to update the SCCP con.\n");
 		abort();
 	}
 
 	/* 3.) send some data */
 	copy_to_msg(msg, bsc_dtap, sizeof(bsc_dtap));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_msc(msg, parsed, con);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_msc(msg, &parsed, con);
 	VERIFY(con_found, con, msg, bsc_dtap_patched, "BSC DTAP");
 
 	/* 4.) receive some data */
 	copy_to_msg(msg, msc_dtap, sizeof(msc_dtap));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_bsc(msg, parsed, nat);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_bsc(msg, &parsed, nat);
 	VERIFY(con_found, con, msg, msc_dtap_patched, "MSC DTAP");
 
 	/* 5.) close the connection */
 	copy_to_msg(msg, msc_rlsd, sizeof(msc_rlsd));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_bsc(msg, parsed, nat);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_bsc(msg, &parsed, nat);
 	VERIFY(con_found, con, msg, msc_rlsd_patched, "MSC RLSD");
 
 	/* 6.) confirm the connection close */
 	copy_to_msg(msg, bsc_rlc, sizeof(bsc_rlc));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_msc(msg, parsed, con);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_msc(msg, &parsed, con);
 	if (!con_found) {
 		printf("Failed to find connection.\n");
 		abort();
@@ -403,12 +401,11 @@
 		printf("Failed to patch the BSC CR msg.\n");
 		abort();
 	}
-	remove_sccp_src_ref(con, msg, parsed);
-	talloc_free(parsed);
+	remove_sccp_src_ref(con, msg, &parsed);
 
 	copy_to_msg(msg, bsc_rlc, sizeof(bsc_rlc));
-	parsed = bsc_nat_parse(msg);
-	con_found = patch_sccp_src_ref_to_msc(msg, parsed, con);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	con_found = patch_sccp_src_ref_to_msc(msg, &parsed, con);
 
 	/* verify that it is gone */
 	if (con_found != NULL) {
@@ -416,8 +413,6 @@
 		       sccp_src_ref_to_int(&con_found->real_ref));
 		abort();
 	}
-	talloc_free(parsed);
-
 
 	bsc_config_free(con->cfg);
 	bsc_nat_free(nat);
@@ -507,7 +502,7 @@
 	struct bsc_connection *bsc;
 	struct bsc_nat *nat;
 	struct nat_sccp_connection con;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	struct msgb *msg;
 
 	printf("Testing MGCP.\n");
@@ -529,7 +524,7 @@
 
 	msg = msgb_alloc(4096, "foo");
 	copy_to_msg(msg, ass_cmd, sizeof(ass_cmd));
-	parsed = bsc_nat_parse(msg);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
 
 	if (msg->l2h[16] != 0 ||
 	    msg->l2h[17] != 0x1) {
@@ -568,8 +563,6 @@
 		abort();
 	}
 
-	talloc_free(parsed);
-
 	bsc_mgcp_dlcx(&con);
 	if (con.bsc_endp != -1 || con.msc_endp != -1 ||
 	    con.bsc->_endpoint_status[1] != 0 || con.bsc->last_endpoint != 0x1) {
@@ -867,7 +860,7 @@
 {
 	int i, res, contype;
 	struct msgb *msg = msgb_alloc(4096, "test_cr_filter");
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	struct bsc_msg_acc_lst *nat_lst, *bsc_lst;
 	struct bsc_msg_acc_lst_entry *nat_entry, *bsc_entry;
 	struct bsc_filter_reject_cause cause;
@@ -912,14 +905,13 @@
 			      &cr_filter[i].bsc_imsi_deny) != 0)
 			abort();
 
-		parsed = bsc_nat_parse(msg);
-		if (!parsed) {
+		if (bsc_nat_parse(msg, &parsed) < 0) {
 			printf("FAIL: Failed to parse the message\n");
 			abort();
 		}
 
 		memset(&cause, 0, sizeof(cause));
-		res = bsc_nat_filter_sccp_cr(bsc, msg, parsed, &contype, &imsi, &cause);
+		res = bsc_nat_filter_sccp_cr(bsc, msg, &parsed, &contype, &imsi, &cause);
 		if (res != cr_filter[i].result) {
 			printf("FAIL: Wrong result %d for test %d.\n", res, i);
 			abort();
@@ -934,8 +926,7 @@
 			abort();
 		}
 
-		talloc_steal(parsed, imsi);
-		talloc_free(parsed);
+		talloc_free(imsi);
 	}
 
 	msgb_free(msg);
@@ -946,7 +937,7 @@
 {
 	int i;
 	struct msgb *msg = msgb_alloc(4096, "test_dt_filter");
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	struct bsc_filter_reject_cause cause;
 
 	struct bsc_nat *nat = bsc_nat_alloc();
@@ -960,26 +951,25 @@
 	msgb_reset(msg);
 	copy_to_msg(msg, id_resp, ARRAY_SIZE(id_resp));
 
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	if (parsed->bssap != BSSAP_MSG_DTAP) {
+	if (parsed.bssap != BSSAP_MSG_DTAP) {
 		printf("FAIL: It should be dtap\n");
 		abort();
 	}
 
 	/* gsm_type is actually the size of the dtap */
-	if (parsed->gsm_type < msgb_l3len(msg) - 3) {
+	if (parsed.gsm_type < msgb_l3len(msg) - 3) {
 		printf("FAIL: Not enough space for the content\n");
 		abort();
 	}
 
 	memset(&cause, 0, sizeof(cause));
 	OSMO_ASSERT(!con->filter_state.imsi);
-	if (bsc_nat_filter_dt(bsc, msg, con, parsed, &cause) != 1) {
+	if (bsc_nat_filter_dt(bsc, msg, con, &parsed, &cause) != 1) {
 		printf("FAIL: Should have passed..\n");
 		abort();
 	}
@@ -991,13 +981,13 @@
 		msgb_reset(msg);
 		copy_to_msg(msg, id_resp, ARRAY_SIZE(id_resp));
 
-		parsed = bsc_nat_parse(msg);
-		if (!parsed)
+		if (bsc_nat_parse(msg, &parsed) < 0)
 			continue;
 
+
 		con->filter_state.imsi_checked = 0;
 		memset(&cause, 0, sizeof(cause));
-		bsc_nat_filter_dt(bsc, msg, con, parsed, &cause);
+		bsc_nat_filter_dt(bsc, msg, con, &parsed, &cause);
 	}
 
 	msgb_free(msg);
@@ -1008,7 +998,7 @@
 {
 	struct msgb *msg = msgb_alloc(4096, "test_dt_filter");
 	struct msgb *out;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	const char *imsi = "27408000001234";
 
 	struct bsc_nat *nat = bsc_nat_alloc();
@@ -1028,31 +1018,28 @@
 	/* verify that nothing changed */
 	msgb_reset(msg);
 	copy_to_msg(msg, cc_setup_international, ARRAY_SIZE(cc_setup_international));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (msg != out) {
 		printf("FAIL: The message should not have been changed\n");
 		abort();
 	}
 
 	verify_msg(out, cc_setup_international, ARRAY_SIZE(cc_setup_international));
-	talloc_free(parsed);
 
 	/* verify that something in the message changes */
 	msgb_reset(msg);
 	copy_to_msg(msg, cc_setup_national, ARRAY_SIZE(cc_setup_national));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (!out) {
 		printf("FAIL: A new message should be created.\n");
 		abort();
@@ -1071,13 +1058,12 @@
 	bsc_nat_num_rewr_entry_adapt(nat, &nat->num_rewr, &entries);
 	msg = msgb_alloc(4096, "test_dt_filter");
 	copy_to_msg(msg, cc_setup_national, ARRAY_SIZE(cc_setup_national));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (!out) {
 		printf("FAIL: A new message should be created.\n");
 		abort();
@@ -1096,13 +1082,12 @@
 	bsc_nat_num_rewr_entry_adapt(nat, &nat->num_rewr, &entries);
 	msg = msgb_alloc(4096, "test_dt_filter");
 	copy_to_msg(msg, cc_setup_national, ARRAY_SIZE(cc_setup_national));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (out != msg) {
 		printf("FAIL: The message should be unchanged.\n");
 		abort();
@@ -1118,13 +1103,12 @@
 	bsc_nat_num_rewr_entry_adapt(nat, &nat->num_rewr, &entries);
 	msg = msgb_alloc(4096, "test_dt_filter");
 	copy_to_msg(msg, cc_setup_national_patched, ARRAY_SIZE(cc_setup_national_patched));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp %d\n", __LINE__);
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (!out) {
 		printf("FAIL: A new message should be created %d.\n", __LINE__);
 		abort();
@@ -1146,13 +1130,12 @@
 	bsc_nat_num_rewr_entry_adapt(nat, &nat->num_rewr, &entries);
 	msg = msgb_alloc(4096, "test_dt_filter");
 	copy_to_msg(msg, cc_setup_national_patched, ARRAY_SIZE(cc_setup_national_patched));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp %d\n", __LINE__);
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (!out) {
 		printf("FAIL: A new message should be created %d.\n", __LINE__);
 		abort();
@@ -1174,7 +1157,7 @@
 {
 	struct msgb *msg = msgb_alloc(4096, "test_dt_filter");
 	struct msgb *out;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	const char *imsi = "27408000001234";
 
 	struct bsc_nat *nat = bsc_nat_alloc();
@@ -1195,13 +1178,12 @@
 
 	msgb_reset(msg);
 	copy_to_msg(msg, cc_setup_national, ARRAY_SIZE(cc_setup_national));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (!out) {
 		printf("FAIL: A new message should be created.\n");
 		abort();
@@ -1223,7 +1205,7 @@
 {
 	struct msgb *msg = msgb_alloc(4096, "test_dt_filter");
 	struct msgb *out;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	const char *imsi = "27408000001234";
 
 	struct bsc_nat *nat = bsc_nat_alloc();
@@ -1255,13 +1237,12 @@
 
 	msgb_reset(msg);
 	copy_to_msg(msg, cc_setup_national, ARRAY_SIZE(cc_setup_national));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse ID resp\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (!out) {
 		printf("FAIL: A new message should be created.\n");
 		abort();
@@ -1281,7 +1262,7 @@
 static void test_sms_smsc_rewrite()
 {
 	struct msgb *msg = msgb_alloc(4096, "SMSC rewrite"), *out;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	const char *imsi = "515039900406700";
 
 	struct bsc_nat *nat = bsc_nat_alloc();
@@ -1317,13 +1298,12 @@
 	 * Check if the SMSC address is changed
 	 */
 	copy_to_msg(msg, smsc_rewrite, ARRAY_SIZE(smsc_rewrite));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse SMS\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (out == msg) {
 		printf("FAIL: This should have changed.\n");
 		abort();
@@ -1337,13 +1317,12 @@
 	bsc_nat_num_rewr_entry_adapt(nat, &nat->smsc_rewr, NULL);
 	msg = msgb_alloc(4096, "SMSC rewrite");
 	copy_to_msg(msg, smsc_rewrite, ARRAY_SIZE(smsc_rewrite));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse SMS\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (out == msg) {
 		printf("FAIL: This should have changed.\n");
 		abort();
@@ -1357,13 +1336,12 @@
 	bsc_nat_num_rewr_entry_adapt(nat, &nat->sms_clear_tp_srr, NULL);
 	msg = msgb_alloc(4096, "SMSC rewrite");
 	copy_to_msg(msg, smsc_rewrite, ARRAY_SIZE(smsc_rewrite));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse SMS\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (out != msg) {
 		printf("FAIL: This should not have changed.\n");
 		abort();
@@ -1377,7 +1355,7 @@
 static void test_sms_number_rewrite(void)
 {
 	struct msgb *msg, *out;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	const char *imsi = "515039900406700";
 
 	struct bsc_nat *nat = bsc_nat_alloc();
@@ -1401,13 +1379,12 @@
 	 */
  	msg = msgb_alloc(4096, "SMSC rewrite");
 	copy_to_msg(msg, smsc_rewrite, ARRAY_SIZE(smsc_rewrite));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse SMS\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (out == msg) {
 		printf("FAIL: This should have changed.\n");
 		abort();
@@ -1429,13 +1406,12 @@
 
  	msg = msgb_alloc(4096, "SMSC rewrite");
 	copy_to_msg(msg, smsc_rewrite, ARRAY_SIZE(smsc_rewrite));
-	parsed = bsc_nat_parse(msg);
-	if (!parsed) {
+	if (bsc_nat_parse(msg, &parsed) < 0) {
 		printf("FAIL: Could not parse SMS\n");
 		abort();
 	}
 
-	out = bsc_nat_rewrite_msg(nat, msg, parsed, imsi);
+	out = bsc_nat_rewrite_msg(nat, msg, &parsed, imsi);
 	if (out == msg) {
 		printf("FAIL: This should have changed.\n");
 		abort();
@@ -1520,7 +1496,7 @@
 	struct bsc_connection *bsc;
 	struct bsc_nat *nat;
 	struct nat_sccp_connection con;
-	struct bsc_nat_parsed *parsed;
+	struct bsc_nat_parsed parsed;
 	struct msgb *msg = msgb_alloc(4096, "test-message");
 
 	printf("Testing LAC extraction from SCCP CR\n");
@@ -1538,8 +1514,8 @@
 	memcpy(msg->l2h, bssmap_cr, ARRAY_SIZE(bssmap_cr));
 
 	/* parse it and pass it on */
-	parsed = bsc_nat_parse(msg);
-	res = bsc_nat_extract_lac(bsc, &con, parsed, msg);
+	OSMO_ASSERT(bsc_nat_parse(msg, &parsed) == 0);
+	res = bsc_nat_extract_lac(bsc, &con, &parsed, msg);
 	OSMO_ASSERT(res == 0);
 
 	/* verify the LAC */

-- 
To view, visit https://gerrit.osmocom.org/13843
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I66c44890952339f15131081e2f629a2824b6d3ba
Gerrit-Change-Number: 13843
Gerrit-PatchSet: 5
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Daniel Willmann <dwillmann at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190521/4f2cd620/attachment.html>


More information about the gerrit-log mailing list