[PATCH] Add workaround to prevent out-of-bounds error.

From: Max &lt;msuraev(a)sysmocom.de&gt; 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(-) 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 -- 2.5.0