fix cell identifier decoding in libosmocore

The cell ID list decoder merged in 11a4d9dd91216fe353e94bfdbbab53bc4f891c0d
has a bug which was introduced part-way through the review process in
gerrit at https://gerrit.osmocom.org/#/c/6509/

When Neels suggested "why not just {...}id_list[MAXLEN] once?" I changed
the cell identifier list from a union of arrays to an array of unions.

After this change, elements smaller than the largest type in the union
were not laid out consecutively in memory anymore. E.g. uint16_t lac
values now occur at offsets of sizeof(id_list[0]) instead of offsets
of sizeof(uint16_t).

The problem is that I forgot to adjust the decoder accordingly, so the
decoder writes to the wrong offsets and returns cell identifier lists
which appear to contain uninitialized values when read back by API
consumers.

I found this problem while adding new regression tests to libosmocore to
test encoding and decoding. This commit adds one such tests for LAC list
decoding, which failed due to the above bug. I plan to write more tests,
however because this first test already uncovered a severe issue I chose
to submit a fix now and work on additional tests in later commits.

Change-Id: Ie1a5a9d858226be578cf11a03cf996d509bd51fb
Related: OS#2847

2 files changed, 53 insertions, 15 deletions

diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c
index e12a9689..2d95ec68 100644
--- a/src/gsm/gsm0808_utils.c
+++ b/src/gsm/gsm0808_utils.c
@@ -689,7 +689,7 @@ static int decode_lai(const uint8_t *data, uint16_t *mcc, uint16_t *mnc, uint16_
 	return gsm48_decode_lai(&lai, mcc, mnc, lac) ? -1 : 0;
 }
 
-static int parse_cell_id_global_list(struct osmo_cell_global_id *id_list, const uint8_t *data, size_t remain,
+static int parse_cell_id_global_list(struct gsm0808_cell_id_list2 *cil, const uint8_t *data, size_t remain,
 				     size_t *consumed)
 {
 	struct osmo_cell_global_id *id;
@@ -702,7 +702,7 @@ static int parse_cell_id_global_list(struct osmo_cell_global_id *id_list, const
 	while (remain >= elemlen) {
 		if (i >= GSM0808_CELL_ID_LIST2_MAXLEN)
 			return -ENOSPC;
-		id = &id_list[i];
+		id = &cil->id_list[i].global;
 		lai_offset = i * elemlen;
 		if (decode_lai(&data[lai_offset], &id->lai.plmn.mcc, &id->lai.plmn.mnc, &id->lai.lac) != 0)
 			return -EINVAL;
@@ -716,7 +716,7 @@ static int parse_cell_id_global_list(struct osmo_cell_global_id *id_list, const
 	return i;
 }
 
-static int parse_cell_id_lac_and_ci_list(struct osmo_lac_and_ci_id *id_list, const uint8_t *data, size_t remain,
+static int parse_cell_id_lac_and_ci_list(struct gsm0808_cell_id_list2 *cil, const uint8_t *data, size_t remain,
 					 size_t *consumed)
 {
 	uint16_t *lacp_be, *ci_be;
@@ -734,7 +734,7 @@ static int parse_cell_id_lac_and_ci_list(struct osmo_lac_and_ci_id *id_list, con
 	while (remain >= elemlen) {
 		if (i >= GSM0808_CELL_ID_LIST2_MAXLEN)
 			return -ENOSPC;
-		id = &id_list[i];
+		id = &cil->id_list[i].lac_and_ci;
 		id->lac = osmo_load16be(lacp_be);
 		id->ci = osmo_load16be(ci_be);
 		*consumed += elemlen;
@@ -746,7 +746,8 @@ static int parse_cell_id_lac_and_ci_list(struct osmo_lac_and_ci_id *id_list, con
 	return i;
 }
 
-static int parse_cell_id_ci_list(uint16_t *id_list, const uint8_t *data, size_t remain, size_t *consumed)
+static int parse_cell_id_ci_list(struct gsm0808_cell_id_list2 *cil, const uint8_t *data, size_t remain,
+    size_t *consumed)
 {
 	const uint16_t *ci_be = (const uint16_t *)data;
 	int i = 0;
@@ -756,14 +757,14 @@ static int parse_cell_id_ci_list(uint16_t *id_list, const uint8_t *data, size_t
 	while (remain >= elemlen) {
 		if (i >= GSM0808_CELL_ID_LIST2_MAXLEN)
 			return -ENOSPC;
-		id_list[i++] = osmo_load16be(ci_be++);
+		cil->id_list[i++].ci = osmo_load16be(ci_be++);
 		consumed += elemlen;
 		remain -= elemlen;
 	}
 	return i;
 }
 
-static int parse_cell_id_lai_and_lac(struct osmo_location_area_id *id_list, const uint8_t *data, size_t remain,
+static int parse_cell_id_lai_and_lac(struct gsm0808_cell_id_list2 *cil, const uint8_t *data, size_t remain,
 				     size_t *consumed)
 {
 	struct osmo_location_area_id *id;
@@ -774,7 +775,7 @@ static int parse_cell_id_lai_and_lac(struct osmo_location_area_id *id_list, cons
 	while (remain >= elemlen) {
 		if (i >= GSM0808_CELL_ID_LIST2_MAXLEN)
 			return -ENOSPC;
-		id = &id_list[i];
+		id = &cil->id_list[i].lai_and_lac;
 		if (decode_lai(&data[i * elemlen], &id->plmn.mcc, &id->plmn.mnc, &id->lac) != 0)
 			return -EINVAL;
 		*consumed += elemlen;
@@ -785,7 +786,7 @@ static int parse_cell_id_lai_and_lac(struct osmo_location_area_id *id_list, cons
 	return i;
 }
 
-static int parse_cell_id_lac_list(uint16_t *id_list, const uint8_t *data, size_t remain, size_t *consumed)
+static int parse_cell_id_lac_list(struct gsm0808_cell_id_list2 *cil, const uint8_t *data, size_t remain, size_t *consumed)
 {
 	const uint16_t *lac_be = (const uint16_t *)data;
 	int i = 0;
@@ -795,7 +796,7 @@ static int parse_cell_id_lac_list(uint16_t *id_list, const uint8_t *data, size_t
 	while (remain >= elemlen) {
 		if (i >= GSM0808_CELL_ID_LIST2_MAXLEN)
 			return -ENOSPC;
-		id_list[i++] = osmo_load16be(lac_be++);
+		cil->id_list[i++].lac = osmo_load16be(lac_be++);
 		*consumed += elemlen;
 		remain -= elemlen;
 	}
@@ -828,19 +829,19 @@ int gsm0808_dec_cell_id_list2(struct gsm0808_cell_id_list2 *cil,
 
 	switch (id_discr) {
 	case CELL_IDENT_WHOLE_GLOBAL:
-		list_len = parse_cell_id_global_list(&cil->id_list[0].global, elem, len, &bytes_elem);
+		list_len = parse_cell_id_global_list(cil, elem, len, &bytes_elem);
 		break;
 	case CELL_IDENT_LAC_AND_CI:
-		list_len = parse_cell_id_lac_and_ci_list(&cil->id_list[0].lac_and_ci, elem, len, &bytes_elem);
+		list_len = parse_cell_id_lac_and_ci_list(cil, elem, len, &bytes_elem);
 		break;
 	case CELL_IDENT_CI:
-		list_len = parse_cell_id_ci_list(&cil->id_list[0].ci, elem, len, &bytes_elem);
+		list_len = parse_cell_id_ci_list(cil, elem, len, &bytes_elem);
 		break;
 	case CELL_IDENT_LAI_AND_LAC:
-		list_len = parse_cell_id_lai_and_lac(&cil->id_list[0].lai_and_lac, elem, len, &bytes_elem);
+		list_len = parse_cell_id_lai_and_lac(cil, elem, len, &bytes_elem);
 		break;
 	case CELL_IDENT_LAC:
-		list_len = parse_cell_id_lac_list(&cil->id_list[0].lac, elem, len, &bytes_elem);
+		list_len = parse_cell_id_lac_list(cil, elem, len, &bytes_elem);
 		break;
 	case CELL_IDENT_BSS:
 	case CELL_IDENT_NO_CELL:
diff --git a/tests/gsm0808/gsm0808_test.c b/tests/gsm0808/gsm0808_test.c
index 21a0c992..8e21b0cb 100644
--- a/tests/gsm0808/gsm0808_test.c
+++ b/tests/gsm0808/gsm0808_test.c
@@ -805,6 +805,42 @@ static void test_gsm0808_enc_dec_cell_id_list_single_lac()
 	msgb_free(msg);
 }
 
+static void test_gsm0808_enc_dec_cell_id_list_multi_lac()
+{
+	struct gsm0808_cell_id_list2 enc_cil;
+	struct gsm0808_cell_id_list2 dec_cil;
+	struct msgb *msg;
+	uint8_t cil_enc_expected[] = { GSM0808_IE_CELL_IDENTIFIER_LIST, 0x0b, 0x05,
+		0x23, 0x42,
+		0x24, 0x43,
+		0x25, 0x44,
+		0x26, 0x45,
+		0x27, 0x46
+	};
+	uint8_t rc_enc;
+	int rc_dec;
+
+	memset(&enc_cil, 0, sizeof(enc_cil));
+	enc_cil.id_discr = CELL_IDENT_LAC;
+	enc_cil.id_list[0].lac = 0x2342;
+	enc_cil.id_list[1].lac = 0x2443;
+	enc_cil.id_list[2].lac = 0x2544;
+	enc_cil.id_list[3].lac = 0x2645;
+	enc_cil.id_list[4].lac = 0x2746;
+	enc_cil.id_list_len = 5;
+
+	msg = msgb_alloc(1024, "output buffer");
+	rc_enc = gsm0808_enc_cell_id_list2(msg, &enc_cil);
+	OSMO_ASSERT(rc_enc == sizeof(cil_enc_expected));
+	OSMO_ASSERT(memcmp(cil_enc_expected, msg->data, msg->len) == 0);
+
+	rc_dec = gsm0808_dec_cell_id_list2(&dec_cil, msg->data + 2, msg->len - 2);
+	OSMO_ASSERT(rc_dec == msg->len - 2);
+	OSMO_ASSERT(memcmp(&enc_cil, &dec_cil, sizeof(enc_cil)) == 0);
+
+	msgb_free(msg);
+}
+
 static void test_gsm0808_enc_dec_cell_id_list_bss()
 {
 	struct gsm0808_cell_id_list2 enc_cil;
@@ -861,6 +897,7 @@ int main(int argc, char **argv)
 	test_gsm0808_enc_dec_encrypt_info();
 	test_gsm0808_enc_dec_cell_id_list_lac();
 	test_gsm0808_enc_dec_cell_id_list_single_lac();
+	test_gsm0808_enc_dec_cell_id_list_multi_lac();
 	test_gsm0808_enc_dec_cell_id_list_bss();
 
 	printf("Done\n");