[PATCH] libosmocore[master]: Add function to properly encode RAI

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Max gerrit-no-reply at lists.osmocom.org
Fri Jan 5 13:31:14 UTC 2018


Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/5652

to look at the new patch set (#3).

Add function to properly encode RAI

Add gsm48_encode_ra() which takes appropriate struct as [out] parameter
instead of generic buffer. Using uint8_t buffer instead of proper struct
type prooved to be error-prone - see Coverity CID57877, CID57876.

Old gsm48_construct_ra() is made into tiny wrapper around new
function. The test output is adjusted because of the change in function
return value which was constant and hence ignored anyway.

Change-Id: I31f9605277f4945f207c2c44ff82e62399f8db74
---
M include/osmocom/gprs/gprs_bssgp_bss.h
M include/osmocom/gsm/gsm48.h
M src/gb/gprs_bssgp.c
M src/gb/gprs_bssgp_bss.c
M src/gb/libosmogb.map
M src/gsm/gsm48.c
M src/gsm/libosmogsm.map
M tests/gsm0408/gsm0408_test.c
M tests/gsm0408/gsm0408_test.ok
9 files changed, 53 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/52/5652/3

diff --git a/include/osmocom/gprs/gprs_bssgp_bss.h b/include/osmocom/gprs/gprs_bssgp_bss.h
index 74211fd..f07ab52 100644
--- a/include/osmocom/gprs/gprs_bssgp_bss.h
+++ b/include/osmocom/gprs/gprs_bssgp_bss.h
@@ -26,6 +26,7 @@
 #include <osmocom/gprs/gprs_bssgp.h>
 
 uint8_t *bssgp_msgb_tlli_put(struct msgb *msg, uint32_t tlli);
+uint8_t *bssgp_msgb_ra_put(struct msgb *msg, const struct gprs_ra_id *ra_id);
 int bssgp_tx_bvc_ptp_reset(uint16_t nsei, enum gprs_bssgp_cause cause);
 int bssgp_tx_suspend(uint16_t nsei, uint32_t tlli,
 		     const struct gprs_ra_id *ra_id);
diff --git a/include/osmocom/gsm/gsm48.h b/include/osmocom/gsm/gsm48.h
index 424748e..3ebca18 100644
--- a/include/osmocom/gsm/gsm48.h
+++ b/include/osmocom/gsm/gsm48.h
@@ -39,6 +39,7 @@
 
 /* Parse Routeing Area Identifier */
 void gsm48_parse_ra(struct gprs_ra_id *raid, const uint8_t *buf);
+void gsm48_encode_ra(struct gsm48_ra_id *out, const struct gprs_ra_id *raid);
 int gsm48_construct_ra(uint8_t *buf, const struct gprs_ra_id *raid);
 
 int gsm48_number_of_paging_subchannels(struct gsm48_control_channel_descr *chan_desc);
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c
index d27a94f..7228263 100644
--- a/src/gb/gprs_bssgp.c
+++ b/src/gb/gprs_bssgp.c
@@ -161,7 +161,6 @@
 	struct bssgp_normal_hdr *bgph =
 		(struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
 	uint32_t _tlli;
-	uint8_t ra[6];
 
 	msgb_nsei(msg) = nsei;
 	msgb_bvci(msg) = 0; /* Signalling */
@@ -169,8 +168,7 @@
 
 	_tlli = osmo_htonl(tlli);
 	msgb_tvlv_put(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &_tlli);
-	gsm48_construct_ra(ra, ra_id);
-	msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+	bssgp_msgb_ra_put(msg, ra_id);
 	msgb_tvlv_put(msg, BSSGP_IE_SUSPEND_REF_NR, 1, &suspend_ref);
 
 	return gprs_ns_sendmsg(bssgp_nsi, msg);
@@ -185,7 +183,6 @@
 	struct bssgp_normal_hdr *bgph =
 		(struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
 	uint32_t _tlli;
-	uint8_t ra[6];
 
 	msgb_nsei(msg) = nsei;
 	msgb_bvci(msg) = 0; /* Signalling */
@@ -193,8 +190,8 @@
 
 	_tlli = osmo_htonl(tlli);
 	msgb_tvlv_put(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &_tlli);
-	gsm48_construct_ra(ra, ra_id);
-	msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+	bssgp_msgb_ra_put(msg, ra_id);
+
 	if (cause)
 		msgb_tvlv_put(msg, BSSGP_IE_CAUSE, 1, cause);
 
@@ -209,7 +206,6 @@
 	struct bssgp_normal_hdr *bgph =
 		(struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
 	uint32_t _tlli;
-	uint8_t ra[6];
 
 	msgb_nsei(msg) = nsei;
 	msgb_bvci(msg) = 0; /* Signalling */
@@ -217,8 +213,7 @@
 
 	_tlli = osmo_htonl(tlli);
 	msgb_tvlv_put(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &_tlli);
-	gsm48_construct_ra(ra, ra_id);
-	msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+	bssgp_msgb_ra_put(msg, ra_id);
 
 	return gprs_ns_sendmsg(bssgp_nsi, msg);
 }
@@ -231,7 +226,6 @@
 	struct bssgp_normal_hdr *bgph =
 		(struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
 	uint32_t _tlli;
-	uint8_t ra[6];
 
 	msgb_nsei(msg) = nsei;
 	msgb_bvci(msg) = 0; /* Signalling */
@@ -239,8 +233,8 @@
 
 	_tlli = osmo_htonl(tlli);
 	msgb_tvlv_put(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &_tlli);
-	gsm48_construct_ra(ra, ra_id);
-	msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+	bssgp_msgb_ra_put(msg, ra_id);
+
 	if (cause)
 		msgb_tvlv_put(msg, BSSGP_IE_CAUSE, 1, cause);
 
@@ -259,7 +253,7 @@
 			 uint16_t cid)
 {
 	/* 6 octets RAC */
-	gsm48_construct_ra(buf, raid);
+	gsm48_encode_ra((struct gsm48_ra_id *)buf, raid);
 	/* 2 octets CID */
 	osmo_store16be(cid, buf+6);
 
@@ -1215,7 +1209,7 @@
 	uint16_t drx_params = osmo_htons(pinfo->drx_params);
 	uint8_t mi[10];
 	int imsi_len = gsm48_generate_mid_from_imsi(mi, pinfo->imsi);
-	uint8_t ra[6];
+	struct gsm48_ra_id ra;
 
 	if (imsi_len < 2)
 		return -EINVAL;
@@ -1241,12 +1235,11 @@
 		}
 		break;
 	case BSSGP_PAGING_LOCATION_AREA:
-		gsm48_construct_ra(ra, &pinfo->raid);
-		msgb_tvlv_put(msg, BSSGP_IE_LOCATION_AREA, 4, ra);
+		gsm48_encode_ra(&ra, &pinfo->raid);
+		msgb_tvlv_put(msg, BSSGP_IE_LOCATION_AREA, 4, (const uint8_t *)&ra);
 		break;
 	case BSSGP_PAGING_ROUTEING_AREA:
-		gsm48_construct_ra(ra, &pinfo->raid);
-		msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+		bssgp_msgb_ra_put(msg, &pinfo->raid);
 		break;
 	case BSSGP_PAGING_BVCI:
 		{
diff --git a/src/gb/gprs_bssgp_bss.c b/src/gb/gprs_bssgp_bss.c
index 3939e25..c7e5e4d 100644
--- a/src/gb/gprs_bssgp_bss.c
+++ b/src/gb/gprs_bssgp_bss.c
@@ -44,6 +44,14 @@
 	return msgb_tvlv_put(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &_tlli);
 }
 
+uint8_t *bssgp_msgb_ra_put(struct msgb *msg, const struct gprs_ra_id *ra_id)
+{
+	struct gsm48_ra_id ra;
+
+	gsm48_encode_ra(&ra, ra_id);
+	return msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, sizeof(ra), (const uint8_t *)&ra);
+}
+
 /*! GMM-SUSPEND.req (Chapter 10.3.6) */
 int bssgp_tx_suspend(uint16_t nsei, uint32_t tlli,
 		     const struct gprs_ra_id *ra_id)
@@ -51,7 +59,6 @@
 	struct msgb *msg = bssgp_msgb_alloc();
 	struct bssgp_normal_hdr *bgph =
 			(struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
-	uint8_t ra[6];
 
 	LOGP(DBSSGP, LOGL_NOTICE, "BSSGP (BVCI=0) Tx SUSPEND (TLLI=0x%04x)\n",
 		tlli);
@@ -60,9 +67,7 @@
 	bgph->pdu_type = BSSGP_PDUT_SUSPEND;
 
 	bssgp_msgb_tlli_put(msg, tlli);
-
-	gsm48_construct_ra(ra, ra_id);
-	msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+	bssgp_msgb_ra_put(msg, ra_id);
 
 	return gprs_ns_sendmsg(bssgp_nsi, msg);
 }
@@ -74,7 +79,6 @@
 	struct msgb *msg = bssgp_msgb_alloc();
 	struct bssgp_normal_hdr *bgph =
 			(struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
-	uint8_t ra[6];
 
 	LOGP(DBSSGP, LOGL_NOTICE, "BSSGP (BVCI=0) Tx RESUME (TLLI=0x%04x)\n",
 		tlli);
@@ -83,9 +87,7 @@
 	bgph->pdu_type = BSSGP_PDUT_RESUME;
 
 	bssgp_msgb_tlli_put(msg, tlli);
-
-	gsm48_construct_ra(ra, ra_id);
-	msgb_tvlv_put(msg, BSSGP_IE_ROUTEING_AREA, 6, ra);
+	bssgp_msgb_ra_put(msg, ra_id);
 
 	msgb_tvlv_put(msg, BSSGP_IE_SUSPEND_REF_NR, 1, &suspend_ref);
 
diff --git a/src/gb/libosmogb.map b/src/gb/libosmogb.map
index 9a0dba5..83a3621 100644
--- a/src/gb/libosmogb.map
+++ b/src/gb/libosmogb.map
@@ -9,6 +9,7 @@
 bssgp_msgb_alloc;
 bssgp_msgb_copy;
 bssgp_msgb_tlli_put;
+bssgp_msgb_ra_put;
 bssgp_parse_cell_id;
 bssgp_tx_bvc_block;
 bssgp_tx_bvc_reset;
diff --git a/src/gsm/gsm48.c b/src/gsm/gsm48.c
index a7daea4..8876059 100644
--- a/src/gsm/gsm48.c
+++ b/src/gsm/gsm48.c
@@ -689,33 +689,34 @@
 	raid->rac = buf[5];
 }
 
+/*! Encode a 3GPP TS 24.008 § 10.5.5.15 Routing area identification
+ *  \param[out] out Caller-provided packed struct
+ *  \param[in] raid Routing Area ID to be encoded
+ */
+void gsm48_encode_ra(struct gsm48_ra_id *out, const struct gprs_ra_id *raid)
+{
+	out->lac = osmo_htons(raid->lac);
+	out->rac = raid->rac;
+
+	out->digits[0] = ((raid->mcc / 100) % 10) | (((raid->mcc / 10) % 10) << 4);
+	out->digits[1] = raid->mcc % 10;
+
+	if (raid->mnc < 100) {
+		out->digits[1] |= 0xf0;
+		out->digits[2] = ((raid->mnc / 10) % 10) | ((raid->mnc % 10) << 4);
+	} else {
+		out->digits[1] |= (raid->mnc % 10) << 4;
+		out->digits[2] = ((raid->mnc / 100) % 10) | (((raid->mnc / 10) % 10) << 4);
+	}
+}
+
 /*! Encode a TS 04.08 Routing Area Identifier
  *  \param[out] buf Caller-provided output buffer of 6 bytes
  *  \param[in] raid Routing Area ID to be encoded
  *  \returns number of bytes used in \a buf */
 int gsm48_construct_ra(uint8_t *buf, const struct gprs_ra_id *raid)
 {
-	uint16_t mcc = raid->mcc;
-	uint16_t mnc = raid->mnc;
-	uint16_t _lac;
-
-	buf[0] = ((mcc / 100) % 10) | (((mcc / 10) % 10) << 4);
-	buf[1] = (mcc % 10);
-
-	/* I wonder who came up with the stupidity of encoding the MNC
-	 * differently depending on how many digits its decimal number has! */
-	if (mnc < 100) {
-		buf[1] |= 0xf0;
-		buf[2] = ((mnc / 10) % 10) | ((mnc % 10) << 4);
-	} else {
-		buf[1] |= (mnc % 10) << 4;
-		buf[2] = ((mnc / 100) % 10) | (((mnc / 10) % 10) << 4);
-	}
-
-	_lac = osmo_htons(raid->lac);
-	memcpy(buf + 3, &_lac, 2);
-
-	buf[5] = raid->rac;
+	gsm48_encode_ra((struct gsm48_ra_id *)buf, raid);
 
 	return 6;
 }
diff --git a/src/gsm/libosmogsm.map b/src/gsm/libosmogsm.map
index d915234..5611ba3 100644
--- a/src/gsm/libosmogsm.map
+++ b/src/gsm/libosmogsm.map
@@ -205,6 +205,7 @@
 gsm48_rr_msg_name;
 gsm48_cc_state_name;
 gsm48_construct_ra;
+gsm48_encode_ra;
 gsm48_hdr_gmm_cipherable;
 gsm48_decode_bcd_number;
 gsm48_decode_bearer_cap;
diff --git a/tests/gsm0408/gsm0408_test.c b/tests/gsm0408/gsm0408_test.c
index 935ec21..77a8822 100644
--- a/tests/gsm0408/gsm0408_test.c
+++ b/tests/gsm0408/gsm0408_test.c
@@ -137,8 +137,7 @@
 
 static inline void check_ra(const struct gprs_ra_id *raid)
 {
-	uint8_t buf[6];
-	int res;
+	struct gsm48_ra_id ra;
 	struct gprs_ra_id raid0 = {
 		.mnc = 0,
 		.mcc = 0,
@@ -146,10 +145,10 @@
 		.rac = 0,
 	};
 
-	res = gsm48_construct_ra(buf, raid);
-	printf("Constructed RA: %d - %s\n", res, res != sizeof(buf) ? "FAIL" : "OK");
+	gsm48_encode_ra(&ra, raid);
+	printf("Constructed RA:\n");
 
-	gsm48_parse_ra(&raid0, buf);
+	gsm48_parse_ra(&raid0, (const uint8_t *)&ra);
 	dump_ra(raid);
 	dump_ra(&raid0);
 	printf("RA test...");
diff --git a/tests/gsm0408/gsm0408_test.ok b/tests/gsm0408/gsm0408_test.ok
index f0abfd5..83165fa 100644
--- a/tests/gsm0408/gsm0408_test.ok
+++ b/tests/gsm0408/gsm0408_test.ok
@@ -1,11 +1,11 @@
 Test `CSD 9600/V.110/transparent' passed
 Test `Speech, all codecs' passed
 Simple TMSI encoding test....passed
-Constructed RA: 6 - OK
+Constructed RA:
 RA: MNC=121, MCC=77, LAC=666, RAC=5
 RA: MNC=121, MCC=77, LAC=666, RAC=5
 RA test...passed
-Constructed RA: 6 - OK
+Constructed RA:
 RA: MNC=98, MCC=84, LAC=11, RAC=89
 RA: MNC=98, MCC=84, LAC=11, RAC=89
 RA test...passed

-- 
To view, visit https://gerrit.osmocom.org/5652
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31f9605277f4945f207c2c44ff82e62399f8db74
Gerrit-PatchSet: 3
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder



More information about the gerrit-log mailing list