Add workaround to prevent out-of-bounds error.
diff mbox

Message ID 1453389308-4532-1-git-send-email-suraev@alumni.ntnu.no
State Changes Requested
Headers show

Commit Message

Suraev Jan. 21, 2016, 3:15 p.m. UTC
From: Max <msuraev@sysmocom.de>

Refactor and simplify gsm 7 bit decoder.
Remove useless tests for legacy functions.
Make tests more human-readable.
Replace assert inside the library with returning error code.

Ticket: OW#1198
Sponsored-by: On-Waves ehf
---
 src/gsm/gsm_utils.c   | 35 +++++++++++++++++-------------
 tests/sms/sms_test.c  | 59 ++++++++++++++++++++++++---------------------------
 tests/sms/sms_test.ok | 28 +++++++++---------------
 3 files changed, 58 insertions(+), 64 deletions(-)

Patch
diff mbox

diff --git a/src/gsm/gsm_utils.c b/src/gsm/gsm_utils.c
index fad59bc..bcc0271 100644
--- a/src/gsm/gsm_utils.c
+++ b/src/gsm/gsm_utils.c
@@ -126,18 +126,17 @@  uint8_t gsm_get_octet_len(const uint8_t sept_len){
 	return octet_len;
 }
 
-/* GSM 03.38 6.2.1 Character unpacking */
+/* GSM 03.38 6.2.1 Character unpacking
+ * returns -1 on size errors, number of characters otherwise
+ */
 int gsm_7bit_decode_n_hdr(char *text, size_t n, const uint8_t *user_data, uint8_t septet_l, uint8_t ud_hdr_ind)
 {
-	int i = 0;
-	int shift = 0;
-	uint8_t c7, c8;
-	uint8_t next_is_ext = 0;
+	unsigned shift = 0;
+	uint8_t c7, c8, next_is_ext = 0, lu, ru, maxlen = gsm_get_octet_len(septet_l);
 	const char *text_buf_begin = text;
 	const char *text_buf_end = text + n;
-	int nchars;
 
-	OSMO_ASSERT (n > 0);
+	if (n < 1) return -1;
 
 	/* skip the user data header */
 	if (ud_hdr_ind) {
@@ -148,12 +147,20 @@  int gsm_7bit_decode_n_hdr(char *text, size_t n, const uint8_t *user_data, uint8_
 		septet_l = septet_l - shift;
 	}
 
+	unsigned i, l, r;
 	for (i = 0; i < septet_l && text != text_buf_end - 1; i++) {
-		c7 =
-			((user_data[((i + shift) * 7 + 7) >> 3] <<
-			  (7 - (((i + shift) * 7 + 7) & 7))) |
-			 (user_data[((i + shift) * 7) >> 3] >>
-			  (((i + shift) * 7) & 7))) & 0x7f;
+
+		l = ((i + shift) * 7 + 7) >> 3;
+		r = ((i + shift) * 7) >> 3;
+
+		if (l >= maxlen)
+			lu = 0; // workaround to prevent getting out of bounds
+		else
+			lu = user_data[l] << (7 - (((i + shift) * 7 + 7) & 7));
+
+		ru = user_data[r] >> (((i + shift) * 7) & 7);
+
+		c7 = (lu | ru) & 0x7f;
 
 		if (next_is_ext) {
 			/* this is an extension character */
@@ -169,11 +176,9 @@  int gsm_7bit_decode_n_hdr(char *text, size_t n, const uint8_t *user_data, uint8_
 		*(text++) = c8;
 	}
 
-	nchars = text - text_buf_begin;
-
 	*text = '\0';
 
-	return nchars;
+	return text - text_buf_begin;
 }
 
 int gsm_7bit_decode_n(char *text, size_t n, const uint8_t *user_data, uint8_t septet_l)
diff --git a/tests/sms/sms_test.c b/tests/sms/sms_test.c
index cdd4158..1927925 100644
--- a/tests/sms/sms_test.c
+++ b/tests/sms/sms_test.c
@@ -44,6 +44,7 @@  struct test_case {
 	const uint16_t expected_octet_length;
 	const uint16_t expected_septet_length;
 	const uint8_t ud_hdr_ind;
+	const char * descr;
 };
 
 static const char simple_text[] = "test text";
@@ -131,6 +132,7 @@  static const struct test_case test_multiple_encode[] =
 		.expected_octet_length = sizeof(concatenated_part1_enc),
 		.expected_septet_length = concatenated_part1_septet_length,
 		.ud_hdr_ind = 1,
+		.descr = "concatenated text p1"
 	},
 	{
 		.input = (const uint8_t *) concatenated_text,
@@ -138,6 +140,7 @@  static const struct test_case test_multiple_encode[] =
 		.expected_octet_length = sizeof(concatenated_part2_enc),
 		.expected_septet_length = concatenated_part2_septet_length,
 		.ud_hdr_ind = 1,
+		.descr = "concatenated text p2"
 	},
 };
 
@@ -149,6 +152,7 @@  static const struct test_case test_encode[] =
 		.expected_octet_length = sizeof(simple_enc),
 		.expected_septet_length = simple_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "simple text"
 	},
 	{
 		.input = (const uint8_t *) escape_text,
@@ -156,6 +160,7 @@  static const struct test_case test_encode[] =
 		.expected_octet_length = sizeof(escape_enc),
 		.expected_septet_length = escape_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "escape text"
 	},
 	{
 		.input = (const uint8_t *) enhanced_text,
@@ -163,6 +168,7 @@  static const struct test_case test_encode[] =
 		.expected_octet_length = sizeof(enhanced_enc),
 		.expected_septet_length = enhanced_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "enhanced text"
 	},
 	{
 		.input = (const uint8_t *) enhancedV2_text,
@@ -170,6 +176,7 @@  static const struct test_case test_encode[] =
 		.expected_octet_length = sizeof(enhancedV2_enc),
 		.expected_septet_length = enhancedV2_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "enhanced text v2"
 	},
 };
 
@@ -181,6 +188,7 @@  static const struct test_case test_decode[] =
 		.expected = (const uint8_t *) simple_text,
 		.expected_septet_length = simple_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "simple text"
 	},
 	{
 		.input = escape_enc,
@@ -188,6 +196,7 @@  static const struct test_case test_decode[] =
 		.expected = (const uint8_t *) escape_text,
 		.expected_septet_length = escape_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "escape text"
 	},
 	{
 		.input = enhanced_enc,
@@ -195,6 +204,7 @@  static const struct test_case test_decode[] =
 		.expected = (const uint8_t *) enhanced_text,
 		.expected_septet_length = enhanced_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "enhanced text"
 	},
 	{
 		.input = enhancedV2_enc,
@@ -202,6 +212,7 @@  static const struct test_case test_decode[] =
 		.expected = (const uint8_t *) enhancedV2_text,
 		.expected_septet_length = enhancedV2_septet_length,
 		.ud_hdr_ind = 0,
+		.descr = "enhanced text v2"
 	},
 	{
 		.input = concatenated_part1_enc,
@@ -209,6 +220,7 @@  static const struct test_case test_decode[] =
 		.expected = (const uint8_t *) splitted_text_part1,
 		.expected_septet_length = concatenated_part1_septet_length_with_header,
 		.ud_hdr_ind = 1,
+		.descr = "concatenated text p1"
 	},
 	{
 		.input = concatenated_part2_enc,
@@ -216,7 +228,8 @@  static const struct test_case test_decode[] =
 		.expected = (const uint8_t *) splitted_text_part2,
 		.expected_septet_length = concatenated_part2_septet_length_with_header,
 		.ud_hdr_ind = 1,
-	},
+		.descr = "concatenated text p2"
+		},
 };
 
 static void test_octet_return()
@@ -288,29 +301,17 @@  int main(int argc, char** argv)
 
 	/* test 7-bit encoding */
 	for (i = 0; i < ARRAY_SIZE(test_encode); ++i) {
-		/* Test legacy function (return value only) */
-		septet_length = gsm_7bit_encode(coded,
-						(const char *) test_encode[i].input);
-		printf("Legacy encode case %d: "
-		       "septet length %d (expected %d)\n"
-		       , i
-		       , septet_length, test_encode[i].expected_septet_length
-		      );
-		OSMO_ASSERT (septet_length == test_encode[i].expected_septet_length);
-
-		/* Test new function */
 		memset(coded, 0x42, sizeof(coded));
 		septet_length = gsm_7bit_encode_n(coded, sizeof(coded),
 			       			  (const char *) test_encode[i].input,
 						  &octets_written);
 		computed_octet_length = gsm_get_octet_len(septet_length);
-		printf("Encode case %d: "
-		       "Octet length %d (expected %d, computed %d), "
-		       "septet length %d (expected %d)\n"
-		       , i
-		       , octets_written, test_encode[i].expected_octet_length, computed_octet_length
-		       , septet_length, test_encode[i].expected_septet_length
-		      );
+		printf("Encode case %d (%s): "
+		       "Octet length %d (expected %d, computed %d, test %s), "
+		       "septet length %d (expected %d, test %s)\n",
+		       i, test_encode[i].descr,
+		       octets_written, test_encode[i].expected_octet_length, computed_octet_length, (test_encode[i].expected_octet_length == computed_octet_length) ? "OK" : "FAIL",
+		       septet_length, test_encode[i].expected_septet_length, (septet_length == test_encode[i].expected_septet_length) ? "OK" : "FAIL");
 
 		OSMO_ASSERT (octets_written == test_encode[i].expected_octet_length);
 		OSMO_ASSERT (octets_written == computed_octet_length);
@@ -377,22 +378,18 @@  int main(int argc, char** argv)
 
 	/* test 7-bit decoding */
 	for (i = 0; i < ARRAY_SIZE(test_decode); ++i) {
-		/* Test legacy function (return value only) */
-		if (!test_decode[i].ud_hdr_ind) {
-			nchars = gsm_7bit_decode(result, test_decode[i].input,
-						 test_decode[i].expected_septet_length);
-			printf("Legacy decode case %d: "
-			       "return value %d (expected %d)\n",
-			       i, nchars, test_decode[i].expected_septet_length);
-		}
-
-		/* Test new function */
 		memset(result, 0x42, sizeof(result));
+//		printf("Attempting to 7 bit decode %d to %d with exp %d and ind %d\n", test_decode[i].input_length, sizeof(result), test_decode[i].expected_septet_length, test_decode[i].ud_hdr_ind);
 		nchars = gsm_7bit_decode_n_hdr(result, sizeof(result), test_decode[i].input,
 				test_decode[i].expected_septet_length, test_decode[i].ud_hdr_ind);
-		printf("Decode case %d: return value %d (expected %d)\n", i, nchars, strlen(result));
+		printf("Decode case %d (%s): return value %d, expected %d, test %s\n", i, test_decode[i].descr, nchars, strlen(result), (strlen(result) == nchars) ? "OK" : "FAIL");
+
+		int res = strcmp(result, (const char *) test_decode[i].expected);
+		if(0 != res) {
+		  printf("%d:\n%s\n%s\n", res, osmo_hexdump(result, strlen(result)),
+			 osmo_hexdump(test_decode[i].expected, strlen(result)));
+		}
 
-		OSMO_ASSERT(strcmp(result, (const char *) test_decode[i].expected) == 0);
 		OSMO_ASSERT(nchars == strlen(result));
 
 		/* check buffer limiting */
diff --git a/tests/sms/sms_test.ok b/tests/sms/sms_test.ok
index fa536ea..144e602 100644
--- a/tests/sms/sms_test.ok
+++ b/tests/sms/sms_test.ok
@@ -1,22 +1,14 @@ 
 SMS testing
-Legacy encode case 0: septet length 9 (expected 9)
-Encode case 0: Octet length 8 (expected 8, computed 8), septet length 9 (expected 9)
-Legacy encode case 1: septet length 41 (expected 41)
-Encode case 1: Octet length 36 (expected 36, computed 36), septet length 41 (expected 41)
-Legacy encode case 2: septet length 39 (expected 39)
-Encode case 2: Octet length 35 (expected 35, computed 35), septet length 39 (expected 39)
-Legacy encode case 3: septet length 40 (expected 40)
-Encode case 3: Octet length 35 (expected 35, computed 35), septet length 40 (expected 40)
-Legacy decode case 0: return value 9 (expected 9)
-Decode case 0: return value 9 (expected 9)
-Legacy decode case 1: return value 41 (expected 41)
-Decode case 1: return value 40 (expected 40)
-Legacy decode case 2: return value 39 (expected 39)
-Decode case 2: return value 31 (expected 31)
-Legacy decode case 3: return value 40 (expected 40)
-Decode case 3: return value 32 (expected 32)
-Decode case 4: return value 153 (expected 153)
-Decode case 5: return value 40 (expected 40)
+Encode case 0 (simple text): Octet length 8 (expected 8, computed 8, test OK), septet length 9 (expected 9, test OK)
+Encode case 1 (escape text): Octet length 36 (expected 36, computed 36, test OK), septet length 41 (expected 41, test OK)
+Encode case 2 (enhanced text): Octet length 35 (expected 35, computed 35, test OK), septet length 39 (expected 39, test OK)
+Encode case 3 (enhanced text v2): Octet length 35 (expected 35, computed 35, test OK), septet length 40 (expected 40, test OK)
+Decode case 0 (simple text): return value 9, expected 9, test OK
+Decode case 1 (escape text): return value 40, expected 40, test OK
+Decode case 2 (enhanced text): return value 31, expected 31, test OK
+Decode case 3 (enhanced text v2): return value 32, expected 32, test OK
+Decode case 4 (concatenated text p1): return value 153, expected 153, test OK
+Decode case 5 (concatenated text p2): return value 40, expected 40, test OK
 Encoding some tests and printing number of septets/octets
 SEPTETS: 8 OCTETS: 7
 Done