From bbbf089137ed0ada7589606b392354d77e40f2df Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 9 Oct 2019 10:53:37 -0700 Subject: [PATCH] liblog: use packed structs instead of raw unaligned reads Per jmgao@, we still need to worry about unaligned integer accesses. In any case, we already have the packed structs for all of the unaligned data that we're reading, so this change favors using those packed structs. Bug: 142256213 Test: x86,arm32,arm64 liblog-unit-tests Change-Id: I21fc629eac49895d03b5b31daa4cc494b0c4c230 --- liblog/include/private/android_logger.h | 12 +++ liblog/log_event_list.cpp | 122 ++++++++++++------------ liblog/logprint.cpp | 77 ++++++++------- liblog/tests/liblog_test.cpp | 93 ++++++++---------- 4 files changed, 155 insertions(+), 149 deletions(-) diff --git a/liblog/include/private/android_logger.h b/liblog/include/private/android_logger.h index 5e04148b7..d3cb571dc 100644 --- a/liblog/include/private/android_logger.h +++ b/liblog/include/private/android_logger.h @@ -57,6 +57,18 @@ typedef struct __attribute__((__packed__)) { int32_t tag; // Little Endian Order } android_event_header_t; +// Event payload EVENT_TYPE_LIST +typedef struct __attribute__((__packed__)) { + int8_t type; // EVENT_TYPE_LIST + int8_t element_count; +} android_event_list_t; + +// Event payload EVENT_TYPE_FLOAT +typedef struct __attribute__((__packed__)) { + int8_t type; // EVENT_TYPE_FLOAT + float data; +} android_event_float_t; + /* Event payload EVENT_TYPE_INT */ typedef struct __attribute__((__packed__)) { int8_t type; // EVENT_TYPE_INT diff --git a/liblog/log_event_list.cpp b/liblog/log_event_list.cpp index 18ea93056..7882c96ce 100644 --- a/liblog/log_event_list.cpp +++ b/liblog/log_event_list.cpp @@ -51,11 +51,9 @@ struct android_log_context_internal { typedef struct android_log_context_internal android_log_context_internal; static void init_context(android_log_context_internal* context, uint32_t tag) { - size_t needed; - context->tag = tag; context->read_write_flag = kAndroidLoggerWrite; - needed = sizeof(uint8_t) + sizeof(uint8_t); + size_t needed = sizeof(android_event_list_t); if ((context->pos + needed) > MAX_EVENT_PAYLOAD) { context->overflow = true; } @@ -143,7 +141,6 @@ int android_log_parser_reset(android_log_context ctx, const char* msg, size_t le } int android_log_write_list_begin(android_log_context ctx) { - size_t needed; android_log_context_internal* context; context = (android_log_context_internal*)ctx; @@ -154,7 +151,7 @@ int android_log_write_list_begin(android_log_context ctx) { context->overflow = true; return -EOVERFLOW; } - needed = sizeof(uint8_t) + sizeof(uint8_t); + size_t needed = sizeof(android_event_list_t); if ((context->pos + needed) > MAX_EVENT_PAYLOAD) { context->overflow = true; return -EIO; @@ -168,8 +165,9 @@ int android_log_write_list_begin(android_log_context ctx) { if (context->overflow) { return -EIO; } - context->storage[context->pos + 0] = EVENT_TYPE_LIST; - context->storage[context->pos + 1] = 0; + auto* event_list = reinterpret_cast(&context->storage[context->pos]); + event_list->type = EVENT_TYPE_LIST; + event_list->element_count = 0; context->list[context->list_nest_depth] = context->pos + 1; context->count[context->list_nest_depth] = 0; context->pos += needed; @@ -177,57 +175,49 @@ int android_log_write_list_begin(android_log_context ctx) { } int android_log_write_int32(android_log_context ctx, int32_t value) { - size_t needed; - android_log_context_internal* context; - - context = (android_log_context_internal*)ctx; + android_log_context_internal* context = (android_log_context_internal*)ctx; if (!context || (kAndroidLoggerWrite != context->read_write_flag)) { return -EBADF; } if (context->overflow) { return -EIO; } - needed = sizeof(uint8_t) + sizeof(value); + size_t needed = sizeof(android_event_int_t); if ((context->pos + needed) > MAX_EVENT_PAYLOAD) { context->overflow = true; return -EIO; } context->count[context->list_nest_depth]++; - context->storage[context->pos + 0] = EVENT_TYPE_INT; - *reinterpret_cast(&context->storage[context->pos + 1]) = value; + auto* event_int = reinterpret_cast(&context->storage[context->pos]); + event_int->type = EVENT_TYPE_INT; + event_int->data = value; context->pos += needed; return 0; } int android_log_write_int64(android_log_context ctx, int64_t value) { - size_t needed; - android_log_context_internal* context; - - context = (android_log_context_internal*)ctx; + android_log_context_internal* context = (android_log_context_internal*)ctx; if (!context || (kAndroidLoggerWrite != context->read_write_flag)) { return -EBADF; } if (context->overflow) { return -EIO; } - needed = sizeof(uint8_t) + sizeof(value); + size_t needed = sizeof(android_event_long_t); if ((context->pos + needed) > MAX_EVENT_PAYLOAD) { context->overflow = true; return -EIO; } context->count[context->list_nest_depth]++; - context->storage[context->pos + 0] = EVENT_TYPE_LONG; - *reinterpret_cast(&context->storage[context->pos + 1]) = value; + auto* event_long = reinterpret_cast(&context->storage[context->pos]); + event_long->type = EVENT_TYPE_LONG; + event_long->data = value; context->pos += needed; return 0; } int android_log_write_string8_len(android_log_context ctx, const char* value, size_t maxlen) { - size_t needed; - ssize_t len; - android_log_context_internal* context; - - context = (android_log_context_internal*)ctx; + android_log_context_internal* context = (android_log_context_internal*)ctx; if (!context || (kAndroidLoggerWrite != context->read_write_flag)) { return -EBADF; } @@ -237,8 +227,8 @@ int android_log_write_string8_len(android_log_context ctx, const char* value, si if (!value) { value = ""; } - len = strnlen(value, maxlen); - needed = sizeof(uint8_t) + sizeof(int32_t) + len; + int32_t len = strnlen(value, maxlen); + size_t needed = sizeof(android_event_string_t) + len; if ((context->pos + needed) > MAX_EVENT_PAYLOAD) { /* Truncate string for delivery */ len = MAX_EVENT_PAYLOAD - context->pos - 1 - sizeof(int32_t); @@ -248,10 +238,11 @@ int android_log_write_string8_len(android_log_context ctx, const char* value, si } } context->count[context->list_nest_depth]++; - context->storage[context->pos + 0] = EVENT_TYPE_STRING; - *reinterpret_cast(&context->storage[context->pos + 1]) = len; + auto* event_string = reinterpret_cast(&context->storage[context->pos]); + event_string->type = EVENT_TYPE_STRING; + event_string->length = len; if (len) { - memcpy(&context->storage[context->pos + 5], value, len); + memcpy(&event_string->data, value, len); } context->pos += needed; return len; @@ -262,26 +253,22 @@ int android_log_write_string8(android_log_context ctx, const char* value) { } int android_log_write_float32(android_log_context ctx, float value) { - size_t needed; - uint32_t ivalue; - android_log_context_internal* context; - - context = (android_log_context_internal*)ctx; + android_log_context_internal* context = (android_log_context_internal*)ctx; if (!context || (kAndroidLoggerWrite != context->read_write_flag)) { return -EBADF; } if (context->overflow) { return -EIO; } - needed = sizeof(uint8_t) + sizeof(ivalue); + size_t needed = sizeof(android_event_float_t); if ((context->pos + needed) > MAX_EVENT_PAYLOAD) { context->overflow = true; return -EIO; } - ivalue = *(uint32_t*)&value; context->count[context->list_nest_depth]++; - context->storage[context->pos + 0] = EVENT_TYPE_FLOAT; - *reinterpret_cast(&context->storage[context->pos + 1]) = ivalue; + auto* event_float = reinterpret_cast(&context->storage[context->pos]); + event_float->type = EVENT_TYPE_FLOAT; + event_float->data = value; context->pos += needed; return 0; } @@ -443,20 +430,22 @@ static android_log_list_element android_log_read_next_internal(android_log_conte return elem; } - elem.type = static_cast(context->storage[pos++]); + elem.type = static_cast(context->storage[pos]); switch ((int)elem.type) { case EVENT_TYPE_FLOAT: /* Rely on union to translate elem.data.int32 into elem.data.float32 */ /* FALLTHRU */ - case EVENT_TYPE_INT: + case EVENT_TYPE_INT: { elem.len = sizeof(int32_t); - if ((pos + elem.len) > context->len) { + if ((pos + sizeof(android_event_int_t)) > context->len) { elem.type = EVENT_TYPE_UNKNOWN; return elem; } - elem.data.int32 = *reinterpret_cast(&context->storage[pos]); + + auto* event_int = reinterpret_cast(&context->storage[pos]); + pos += sizeof(android_event_int_t); + elem.data.int32 = event_int->data; /* common tangeable object suffix */ - pos += elem.len; elem.complete = !context->list_nest_depth && !context->count[0]; if (!peek) { if (!context->count[context->list_nest_depth] || @@ -466,16 +455,19 @@ static android_log_list_element android_log_read_next_internal(android_log_conte context->pos = pos; } return elem; + } - case EVENT_TYPE_LONG: + case EVENT_TYPE_LONG: { elem.len = sizeof(int64_t); - if ((pos + elem.len) > context->len) { + if ((pos + sizeof(android_event_long_t)) > context->len) { elem.type = EVENT_TYPE_UNKNOWN; return elem; } - elem.data.int64 = *reinterpret_cast(&context->storage[pos]); + + auto* event_long = reinterpret_cast(&context->storage[pos]); + pos += sizeof(android_event_long_t); + elem.data.int64 = event_long->data; /* common tangeable object suffix */ - pos += elem.len; elem.complete = !context->list_nest_depth && !context->count[0]; if (!peek) { if (!context->count[context->list_nest_depth] || @@ -485,15 +477,22 @@ static android_log_list_element android_log_read_next_internal(android_log_conte context->pos = pos; } return elem; + } - case EVENT_TYPE_STRING: - if ((pos + sizeof(int32_t)) > context->len) { + case EVENT_TYPE_STRING: { + if ((pos + sizeof(android_event_string_t)) > context->len) { elem.type = EVENT_TYPE_UNKNOWN; elem.complete = true; return elem; } - elem.len = *reinterpret_cast(&context->storage[pos]); - pos += sizeof(int32_t); + auto* event_string = reinterpret_cast(&context->storage[pos]); + pos += sizeof(android_event_string_t); + // Wire format is int32_t, but elem.len is uint16_t... + if (event_string->length >= UINT16_MAX) { + elem.type = EVENT_TYPE_UNKNOWN; + return elem; + } + elem.len = event_string->length; if ((pos + elem.len) > context->len) { elem.len = context->len - pos; /* truncate string */ elem.complete = true; @@ -502,7 +501,7 @@ static android_log_list_element android_log_read_next_internal(android_log_conte return elem; } } - elem.data.string = (char*)&context->storage[pos]; + elem.data.string = event_string->data; /* common tangeable object suffix */ pos += elem.len; elem.complete = !context->list_nest_depth && !context->count[0]; @@ -514,13 +513,16 @@ static android_log_list_element android_log_read_next_internal(android_log_conte context->pos = pos; } return elem; + } - case EVENT_TYPE_LIST: - if ((pos + sizeof(uint8_t)) > context->len) { + case EVENT_TYPE_LIST: { + if ((pos + sizeof(android_event_list_t)) > context->len) { elem.type = EVENT_TYPE_UNKNOWN; elem.complete = true; return elem; } + auto* event_list = reinterpret_cast(&context->storage[pos]); + pos += sizeof(android_event_list_t); elem.complete = context->list_nest_depth >= ANDROID_MAX_LIST_NEST_DEPTH; if (peek) { return elem; @@ -528,15 +530,17 @@ static android_log_list_element android_log_read_next_internal(android_log_conte if (context->count[context->list_nest_depth]) { context->count[context->list_nest_depth]--; } - context->list_stop = !context->storage[pos]; + context->list_stop = event_list->element_count == 0; context->list_nest_depth++; if (context->list_nest_depth <= ANDROID_MAX_LIST_NEST_DEPTH) { - context->count[context->list_nest_depth] = context->storage[pos]; + context->count[context->list_nest_depth] = event_list->element_count; } - context->pos = pos + sizeof(uint8_t); + context->pos = pos; return elem; + } case EVENT_TYPE_LIST_STOP: /* Suprise Newline terminates lists. */ + pos++; if (!peek) { context->pos = pos; } diff --git a/liblog/logprint.cpp b/liblog/logprint.cpp index dd2c797c3..82fbafd70 100644 --- a/liblog/logprint.cpp +++ b/liblog/logprint.cpp @@ -35,8 +35,10 @@ #include #include + #include #include +#include #include "log_portability.h" @@ -635,8 +637,7 @@ static int android_log_printBinaryEvent(const unsigned char** pEventData, size_t if (eventDataLen < 1) return -1; - type = *eventData++; - eventDataLen--; + type = *eventData; cp = NULL; len = 0; @@ -725,22 +726,24 @@ static int android_log_printBinaryEvent(const unsigned char** pEventData, size_t case EVENT_TYPE_INT: /* 32-bit signed int */ { - int32_t ival; - - if (eventDataLen < 4) return -1; - ival = *reinterpret_cast(eventData); - eventData += 4; - eventDataLen -= 4; - - lval = ival; + if (eventDataLen < sizeof(android_event_int_t)) return -1; + auto* event_int = reinterpret_cast(eventData); + lval = event_int->data; + eventData += sizeof(android_event_int_t); + eventDataLen -= sizeof(android_event_int_t); } goto pr_lval; case EVENT_TYPE_LONG: /* 64-bit signed long */ - if (eventDataLen < 8) return -1; - lval = *reinterpret_cast(eventData); - eventData += 8; - eventDataLen -= 8; + if (eventDataLen < sizeof(android_event_long_t)) { + return -1; + } + { + auto* event_long = reinterpret_cast(eventData); + lval = event_long->data; + } + eventData += sizeof(android_event_long_t); + eventDataLen -= sizeof(android_event_long_t); pr_lval: outCount = snprintf(outBuf, outBufLen, "%" PRId64, lval); if (outCount < outBufLen) { @@ -754,14 +757,11 @@ static int android_log_printBinaryEvent(const unsigned char** pEventData, size_t case EVENT_TYPE_FLOAT: /* float */ { - uint32_t ival; - float fval; - - if (eventDataLen < 4) return -1; - ival = *reinterpret_cast(eventData); - fval = *(float*)&ival; - eventData += 4; - eventDataLen -= 4; + if (eventDataLen < sizeof(android_event_float_t)) return -1; + auto* event_float = reinterpret_cast(eventData); + float fval = event_float->data; + eventData += sizeof(android_event_int_t); + eventDataLen -= sizeof(android_event_int_t); outCount = snprintf(outBuf, outBufLen, "%f", fval); if (outCount < outBufLen) { @@ -776,12 +776,11 @@ static int android_log_printBinaryEvent(const unsigned char** pEventData, size_t case EVENT_TYPE_STRING: /* UTF-8 chars, not NULL-terminated */ { - unsigned int strLen; - - if (eventDataLen < 4) return -1; - strLen = *reinterpret_cast(eventData); - eventData += 4; - eventDataLen -= 4; + if (eventDataLen < sizeof(android_event_string_t)) return -1; + auto* event_string = reinterpret_cast(eventData); + unsigned int strLen = event_string->length; + eventData += sizeof(android_event_string_t); + eventDataLen -= sizeof(android_event_string_t); if (eventDataLen < strLen) { result = -1; /* mark truncated */ @@ -814,20 +813,19 @@ static int android_log_printBinaryEvent(const unsigned char** pEventData, size_t case EVENT_TYPE_LIST: /* N items, all different types */ { - unsigned char count; - int i; + if (eventDataLen < sizeof(android_event_list_t)) return -1; + auto* event_list = reinterpret_cast(eventData); - if (eventDataLen < 1) return -1; - - count = *eventData++; - eventDataLen--; + int8_t count = event_list->element_count; + eventData += sizeof(android_event_list_t); + eventDataLen -= sizeof(android_event_list_t); if (outBufLen <= 0) goto no_room; *outBuf++ = '['; outBufLen--; - for (i = 0; i < count; i++) { + for (int i = 0; i < count; i++) { result = android_log_printBinaryEvent(&eventData, &eventDataLen, &outBuf, &outBufLen, fmtStr, fmtLen); if (result != 0) goto bail; @@ -1017,10 +1015,11 @@ int android_log_processBinaryLogBuffer( } } inCount = buf->len; - if (inCount < 4) return -1; - tagIndex = *reinterpret_cast(eventData); - eventData += 4; - inCount -= 4; + if (inCount < sizeof(android_event_header_t)) return -1; + auto* event_header = reinterpret_cast(eventData); + tagIndex = event_header->tag; + eventData += sizeof(android_event_header_t); + inCount -= sizeof(android_event_header_t); entry->tagLen = 0; entry->tag = NULL; diff --git a/liblog/tests/liblog_test.cpp b/liblog/tests/liblog_test.cpp index 9780b2824..cfcfd995b 100644 --- a/liblog/tests/liblog_test.cpp +++ b/liblog/tests/liblog_test.cpp @@ -1924,10 +1924,10 @@ static void android_errorWriteWithInfoLog_helper(int TAG, const char* SUBTAG, char* original = eventData; // Tag - int tag = *reinterpret_cast(eventData); - eventData += 4; + auto* event_header = reinterpret_cast(eventData); + eventData += sizeof(android_event_header_t); - if (tag != TAG) { + if (event_header->tag != TAG) { continue; } @@ -1938,45 +1938,37 @@ static void android_errorWriteWithInfoLog_helper(int TAG, const char* SUBTAG, } // List type - ASSERT_EQ(EVENT_TYPE_LIST, eventData[0]); - eventData++; - - // Number of elements in list - ASSERT_EQ(3, eventData[0]); - eventData++; + auto* event_list = reinterpret_cast(eventData); + ASSERT_EQ(EVENT_TYPE_LIST, event_list->type); + ASSERT_EQ(3, event_list->element_count); + eventData += sizeof(android_event_list_t); // Element #1: string type for subtag - ASSERT_EQ(EVENT_TYPE_STRING, eventData[0]); - eventData++; - + auto* event_string_subtag = reinterpret_cast(eventData); + ASSERT_EQ(EVENT_TYPE_STRING, event_string_subtag->type); unsigned subtag_len = strlen(SUBTAG); if (subtag_len > 32) subtag_len = 32; - ASSERT_EQ(subtag_len, *reinterpret_cast(eventData)); - eventData += 4; - - if (memcmp(SUBTAG, eventData, subtag_len)) { + ASSERT_EQ(static_cast(subtag_len), event_string_subtag->length); + if (memcmp(SUBTAG, &event_string_subtag->data, subtag_len)) { continue; } - eventData += subtag_len; + eventData += sizeof(android_event_string_t) + subtag_len; // Element #2: int type for uid - ASSERT_EQ(EVENT_TYPE_INT, eventData[0]); - eventData++; - - ASSERT_EQ(UID, *reinterpret_cast(eventData)); - eventData += 4; + auto* event_int_uid = reinterpret_cast(eventData); + ASSERT_EQ(EVENT_TYPE_INT, event_int_uid->type); + ASSERT_EQ(UID, event_int_uid->data); + eventData += sizeof(android_event_int_t); // Element #3: string type for data - ASSERT_EQ(EVENT_TYPE_STRING, eventData[0]); - eventData++; - - size_t dataLen = *reinterpret_cast(eventData); - eventData += 4; + auto* event_string_data = reinterpret_cast(eventData); + ASSERT_EQ(EVENT_TYPE_STRING, event_string_data->type); + size_t dataLen = event_string_data->length; if (DATA_LEN < 512) ASSERT_EQ(DATA_LEN, (int)dataLen); - - if (memcmp(payload, eventData, dataLen)) { + if (memcmp(payload, &event_string_data->data, dataLen)) { continue; } + eventData += sizeof(android_event_string_t); if (DATA_LEN >= 512) { eventData += dataLen; @@ -2082,10 +2074,12 @@ static void android_errorWriteLog_helper(int TAG, const char* SUBTAG, if (!eventData) continue; // Tag - int tag = *reinterpret_cast(eventData); - eventData += 4; + auto* event_header = reinterpret_cast(eventData); + eventData += sizeof(android_event_header_t); - if (tag != TAG) continue; + if (event_header->tag != TAG) { + continue; + } if (!SUBTAG) { // This tag should not have been written because the data was null @@ -2135,10 +2129,10 @@ static void android_errorWriteLog_helper(int TAG, const char* SUBTAG, } // Tag - int tag = *reinterpret_cast(eventData); - eventData += 4; + auto* event_header = reinterpret_cast(eventData); + eventData += sizeof(android_event_header_t); - if (tag != TAG) { + if (event_header->tag != TAG) { continue; } @@ -2149,21 +2143,17 @@ static void android_errorWriteLog_helper(int TAG, const char* SUBTAG, } // List type - ASSERT_EQ(EVENT_TYPE_LIST, eventData[0]); - eventData++; - - // Number of elements in list - ASSERT_EQ(3, eventData[0]); - eventData++; + auto* event_list = reinterpret_cast(eventData); + ASSERT_EQ(EVENT_TYPE_LIST, event_list->type); + ASSERT_EQ(3, event_list->element_count); + eventData += sizeof(android_event_list_t); // Element #1: string type for subtag - ASSERT_EQ(EVENT_TYPE_STRING, eventData[0]); - eventData++; + auto* event_string = reinterpret_cast(eventData); + ASSERT_EQ(EVENT_TYPE_STRING, event_string->type); + ASSERT_EQ(static_cast(strlen(SUBTAG)), event_string->length); - ASSERT_EQ(strlen(SUBTAG), *reinterpret_cast(eventData)); - eventData += 4; - - if (memcmp(SUBTAG, eventData, strlen(SUBTAG))) { + if (memcmp(SUBTAG, &event_string->data, strlen(SUBTAG))) { continue; } ++count; @@ -2673,13 +2663,14 @@ static void create_android_logger(const char* (*fn)(uint32_t tag, // test buffer reading API int buffer_to_string = -1; if (eventData) { - snprintf(msgBuf, sizeof(msgBuf), "I/[%" PRIu32 "]", *reinterpret_cast(eventData)); + auto* event_header = reinterpret_cast(eventData); + eventData += sizeof(android_event_header_t); + snprintf(msgBuf, sizeof(msgBuf), "I/[%" PRId32 "]", event_header->tag); print_barrier(); fprintf(stderr, "%-10s(%5u): ", msgBuf, pid); memset(msgBuf, 0, sizeof(msgBuf)); - buffer_to_string = android_log_buffer_to_string( - eventData + sizeof(uint32_t), log_msg.entry.len - sizeof(uint32_t), - msgBuf, sizeof(msgBuf)); + buffer_to_string = + android_log_buffer_to_string(eventData, log_msg.entry.len, msgBuf, sizeof(msgBuf)); fprintf(stderr, "%s\n", msgBuf); print_barrier(); }