From 154d225b934de70387533f3dfaf5d449a580c28b Mon Sep 17 00:00:00 2001 From: michael-methner Date: Tue, 11 Oct 2022 04:53:12 +0200 Subject: Avoid memory corruption behind buffer wp in function dlt_getloginfo_conv_ascii_to_id (#411) * Avoid memory corruption behind buffer wp in function dlt_getloginfo_conv_ascii_to_id - Introduced new function dlt_getloginfo_conv_ascii_to_string for '\0' terminated strings - Avoid printing garbage characters in dlt-control after APID and CTID (which are not null terminated anymore) - Added unit test for dlt_client_parse_get_log_info_resp_text and dlt_getloginfo_conv_ascii_to_string - Use dlt_getloginfo_conv_ascii_to_string to get '\0' terminated for app_description and context_description Signed-off-by: Michael Methner --- include/dlt/dlt_common.h | 17 ++++++++- src/console/dlt-control.c | 8 ++-- src/lib/dlt_client.c | 4 +- src/shared/dlt_common.c | 23 +++++++++--- tests/gtest_dlt_common.cpp | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 13 deletions(-) diff --git a/include/dlt/dlt_common.h b/include/dlt/dlt_common.h index 100a481..ead124f 100644 --- a/include/dlt/dlt_common.h +++ b/include/dlt/dlt_common.h @@ -1631,15 +1631,28 @@ int16_t dlt_getloginfo_conv_ascii_to_uint16_t(char *rp, int *rp_count); */ int16_t dlt_getloginfo_conv_ascii_to_int16_t(char *rp, int *rp_count); + +/** + * Convert get log info from ASCII to string (with '\0' termination) + * + * @param rp char + * @param rp_count int + * @param wp char Array needs to be 1 byte larger than len to store '\0' + * @param len int + */ +void dlt_getloginfo_conv_ascii_to_string(char *rp, int *rp_count, char *wp, int len); + + /** - * Convert get log info from ASCII to ID + * Convert get log info from ASCII to ID (without '\0' termination) * * @param rp char * @param rp_count int * @param wp char * @param len int + * @return position of last read character in wp */ -void dlt_getloginfo_conv_ascii_to_id(char *rp, int *rp_count, char *wp, int len); +int dlt_getloginfo_conv_ascii_to_id(char *rp, int *rp_count, char *wp, int len); /** * Convert from hex ASCII to binary diff --git a/src/console/dlt-control.c b/src/console/dlt-control.c index 7bf091a..8fbf9f0 100644 --- a/src/console/dlt-control.c +++ b/src/console/dlt-control.c @@ -212,9 +212,9 @@ void dlt_process_get_log_info(void) dlt_print_id(apid, app.app_id); if (app.app_description != 0) - printf("APID:%4s %s\n", apid, app.app_description); + printf("APID:%4.4s %s\n", apid, app.app_description); else - printf("APID:%4s \n", apid); + printf("APID:%4.4s \n", apid); for (j = 0; j < app.count_context_ids; j++) { con = app.context_id_info[j]; @@ -222,13 +222,13 @@ void dlt_process_get_log_info(void) dlt_print_id(ctid, con.context_id); if (con.context_description != 0) - printf("CTID:%4s %2d %2d %s\n", + printf("CTID:%4.4s %2d %2d %s\n", ctid, con.log_level, con.trace_status, con.context_description); else - printf("CTID:%4s %2d %2d\n", + printf("CTID:%4.4s %2d %2d\n", ctid, con.log_level, con.trace_status); diff --git a/src/lib/dlt_client.c b/src/lib/dlt_client.c index 245ebef..c032fd4 100644 --- a/src/lib/dlt_client.c +++ b/src/lib/dlt_client.c @@ -1332,7 +1332,7 @@ DltReturnValue dlt_client_parse_get_log_info_resp_text(DltServiceGetLogInfoRespo return DLT_RETURN_ERROR; } - dlt_getloginfo_conv_ascii_to_id(rp, + dlt_getloginfo_conv_ascii_to_string(rp, &rp_count, con->context_description, con->len_context_description); @@ -1352,7 +1352,7 @@ DltReturnValue dlt_client_parse_get_log_info_resp_text(DltServiceGetLogInfoRespo return DLT_RETURN_ERROR; } - dlt_getloginfo_conv_ascii_to_id(rp, + dlt_getloginfo_conv_ascii_to_string(rp, &rp_count, app->app_description, app->len_app_description); diff --git a/src/shared/dlt_common.c b/src/shared/dlt_common.c index 2ab6ed9..4dfbc20 100644 --- a/src/shared/dlt_common.c +++ b/src/shared/dlt_common.c @@ -4084,17 +4084,31 @@ int16_t dlt_getloginfo_conv_ascii_to_int16_t(char *rp, int *rp_count) return (signed char)strtol(num_work, &endptr, 16); } -void dlt_getloginfo_conv_ascii_to_id(char *rp, int *rp_count, char *wp, int len) +void dlt_getloginfo_conv_ascii_to_string(char *rp, int *rp_count, char *wp, int len) +{ + if ((rp == NULL ) || (rp_count == NULL ) || (wp == NULL )) + return; + /* ------------------------------------------------------ + * from: [72 65 6d 6f ] -> to: [0x72,0x65,0x6d,0x6f,0x00] + * ------------------------------------------------------ */ + + int count = dlt_getloginfo_conv_ascii_to_id(rp, rp_count, wp, len); + *(wp + count) = '\0'; + + return; +} + +int dlt_getloginfo_conv_ascii_to_id(char *rp, int *rp_count, char *wp, int len) { char number16[3] = { 0 }; char *endptr; int count; if ((rp == NULL) || (rp_count == NULL) || (wp == NULL)) - return; + return 0; /* ------------------------------------------------------ - * from: [72 65 6d 6f ] -> to: [0x72,0x65,0x6d,0x6f,0x00] + * from: [72 65 6d 6f ] -> to: [0x72,0x65,0x6d,0x6f] * ------------------------------------------------------ */ for (count = 0; count < len; count++) { number16[0] = *(rp + *rp_count + 0); @@ -4103,8 +4117,7 @@ void dlt_getloginfo_conv_ascii_to_id(char *rp, int *rp_count, char *wp, int len) *rp_count += 3; } - *(wp + count) = 0; - return; + return count; } void dlt_hex_ascii_to_binary(const char *ptr, uint8_t *binary, int *size) diff --git a/tests/gtest_dlt_common.cpp b/tests/gtest_dlt_common.cpp index 9a375ce..18830be 100644 --- a/tests/gtest_dlt_common.cpp +++ b/tests/gtest_dlt_common.cpp @@ -34,6 +34,8 @@ extern "C" #include "dlt-daemon_cfg.h" #include "dlt_user_cfg.h" #include "dlt_version.h" +#include "dlt_client.h" +#include "dlt_protocol.h" int dlt_buffer_increase_size(DltBuffer *); int dlt_buffer_minimize_size(DltBuffer *); @@ -4260,7 +4262,97 @@ TEST(dlt_get_minor_version, nullpointer) /* End Method:dlt_common::dlt_get_minor_version */ +TEST(dlt_client_parse_get_log_info_resp_text, normal) +{ + char input[] = "get_log_info, 07, 02 00 4c 4f 47 00 03 00 54 45 53 54 ff ff 18 00 54 65 73 74 20 43 6f 6e 74 65 78 74 20 66 6f 72 20 4c 6f 67 67 69 6e 67 54 53 31 00 ff ff 1b 00 54 65 73 74 20 43 6f 6e 74 65 78 74 31 20 66 6f 72 20 69 6e 6a 65 63 74 69 6f 6e 54 53 32 00 ff ff 1b 00 54 65 73 74 20 43 6f 6e 74 65 78 74 32 20 66 6f 72 20 69 6e 6a 65 63 74 69 6f 6e 1c 00 54 65 73 74 20 41 70 70 6c 69 63 61 74 69 6f 6e 20 66 6f 72 20 4c 6f 67 67 69 6e 67 53 59 53 00 02 00 4a 4f 55 52 ff ff 0f 00 4a 6f 75 72 6e 61 6c 20 41 64 61 70 74 65 72 4d 47 52 00 ff ff 22 00 43 6f 6e 74 65 78 74 20 6f 66 20 6d 61 69 6e 20 64 6c 74 20 73 79 73 74 65 6d 20 6d 61 6e 61 67 65 72 12 00 44 4c 54 20 53 79 73 74 65 6d 20 4d 61 6e 61 67 65 72 72 65 6d 6f"; + /* expected output: + * APID:LOG- Test Application for Logging + * CTID:TEST -1 -1 Test Context for Logging + * CTID:TS1- -1 -1 Test Context1 for injection + * CTID:TS2- -1 -1 Test Context2 for injection + * APID:SYS- DLT System Manager + * CTID:JOUR -1 -1 Journal Adapter + * CTID:MGR- -1 -1 Context of main dlt system manager + */ + + DltServiceGetLogInfoResponse * resp = (DltServiceGetLogInfoResponse *)malloc(sizeof(DltServiceGetLogInfoResponse )); + DltReturnValue ret = (DltReturnValue) dlt_set_loginfo_parse_service_id( + input, &resp->service_id, &resp->status); + EXPECT_EQ(DLT_RETURN_OK, ret); + EXPECT_EQ(DLT_SERVICE_ID_GET_LOG_INFO, resp->service_id); + EXPECT_EQ(GET_LOG_INFO_STATUS_MAX, resp->status); + + ret = dlt_client_parse_get_log_info_resp_text(resp, input); + EXPECT_EQ(DLT_RETURN_OK, ret); + + EXPECT_EQ(2,resp->log_info_type.count_app_ids); + + EXPECT_EQ(0, memcmp( "LOG", resp->log_info_type.app_ids[0].app_id,4)); + EXPECT_EQ(0, strcmp("Test Application for Logging", resp->log_info_type.app_ids[0].app_description)); + EXPECT_EQ(28, resp->log_info_type.app_ids[0].len_app_description); + + EXPECT_EQ(3, resp->log_info_type.app_ids[0].count_context_ids); + + EXPECT_EQ(0, memcmp( "TEST", resp->log_info_type.app_ids[0].context_id_info[0].context_id,4)); + EXPECT_EQ(0, strcmp( "Test Context for Logging",resp->log_info_type.app_ids[0].context_id_info[0].context_description )); + EXPECT_EQ(24,resp->log_info_type.app_ids[0].context_id_info[0].len_context_description); + EXPECT_EQ(-1,resp->log_info_type.app_ids[0].context_id_info[0].log_level); + EXPECT_EQ(-1,resp->log_info_type.app_ids[0].context_id_info[0].trace_status); + + EXPECT_EQ(0, memcmp( "TS1", resp->log_info_type.app_ids[0].context_id_info[1].context_id,4)); + EXPECT_EQ(0, strcmp( "Test Context1 for injection",resp->log_info_type.app_ids[0].context_id_info[1].context_description )); + EXPECT_EQ(27,resp->log_info_type.app_ids[0].context_id_info[1].len_context_description); + EXPECT_EQ(-1,resp->log_info_type.app_ids[0].context_id_info[1].log_level); + EXPECT_EQ(-1,resp->log_info_type.app_ids[0].context_id_info[1].trace_status); + + EXPECT_EQ(0, memcmp( "TS2", resp->log_info_type.app_ids[0].context_id_info[2].context_id,4)); + EXPECT_EQ(0, strcmp( "Test Context2 for injection",resp->log_info_type.app_ids[0].context_id_info[2].context_description )); + EXPECT_EQ(27,resp->log_info_type.app_ids[0].context_id_info[2].len_context_description); + EXPECT_EQ(-1,resp->log_info_type.app_ids[0].context_id_info[2].log_level); + EXPECT_EQ(-1,resp->log_info_type.app_ids[0].context_id_info[2].trace_status); + + EXPECT_EQ(0, memcmp( "SYS", resp->log_info_type.app_ids[1].app_id,4)); + EXPECT_EQ(0, strcmp("DLT System Manager", resp->log_info_type.app_ids[1].app_description)); + EXPECT_EQ(18, resp->log_info_type.app_ids[1].len_app_description); + + EXPECT_EQ(2, resp->log_info_type.app_ids[1].count_context_ids); + EXPECT_EQ(0, memcmp( "JOUR", resp->log_info_type.app_ids[1].context_id_info[0].context_id,4)); + EXPECT_EQ(0, strcmp( "Journal Adapter",resp->log_info_type.app_ids[1].context_id_info[0].context_description )); + EXPECT_EQ(15,resp->log_info_type.app_ids[1].context_id_info[0].len_context_description); + EXPECT_EQ(-1,resp->log_info_type.app_ids[1].context_id_info[0].log_level); + EXPECT_EQ(-1,resp->log_info_type.app_ids[1].context_id_info[0].trace_status); + + EXPECT_EQ(0, memcmp( "MGR", resp->log_info_type.app_ids[1].context_id_info[1].context_id,4)); + EXPECT_EQ(0, strcmp( "Context of main dlt system manager",resp->log_info_type.app_ids[1].context_id_info[1].context_description )); + EXPECT_EQ(34,resp->log_info_type.app_ids[1].context_id_info[1].len_context_description); + EXPECT_EQ(-1,resp->log_info_type.app_ids[1].context_id_info[1].log_level); + EXPECT_EQ(-1,resp->log_info_type.app_ids[1].context_id_info[1].trace_status); + + ret = (DltReturnValue)dlt_client_cleanup_get_log_info(resp); + EXPECT_EQ(DLT_RETURN_OK,ret); +} + +TEST(dlt_getloginfo_conv_ascii_to_string, normal) +{ + /* NULL-Pointer, expect exeption */ + char rp_1[] = "72 65 6d 6f"; + char rp_2[] = "123456789 72 65 6d 6f"; + int rp_count = 0; + char rp_str[] = + { 0x72, 0x65, 0x6d, 0x6f, 0x00 }; + char wp[5]; + + dlt_getloginfo_conv_ascii_to_string(rp_1, &rp_count, wp, 4); + EXPECT_EQ(0, strcmp(rp_str, wp)); + EXPECT_EQ(12, rp_count); + + rp_count = 10; + dlt_getloginfo_conv_ascii_to_string(rp_2, &rp_count, wp, 4); + EXPECT_EQ(0, strcmp(rp_str, wp)); + EXPECT_EQ(22, rp_count); + +} /*##############################################################################################################################*/ /*##############################################################################################################################*/ -- cgit v1.2.1