From 441054aa1e0deec7de29234ce1dd67aae8cc3e6e Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 15 Oct 2019 16:53:11 -0700 Subject: [PATCH] Remove old logger_entry_v* formats logger_entry and logger_entry_v2 were used for the kernel logger, which we have long since deprecated. logger_entry_v3 is the same as logger_entry_v4 without a uid field, so it is trivially removable, especially since we're now always providing uids in log messages. liblog and logd already get updated in sync with each other, so we have no reason for backwards compatibility with their format. Test: build, unit tests Change-Id: I27c90609f28c8d826e5614fdb3fe59bde22b5042 --- debuggerd/libdebuggerd/tombstone.cpp | 2 +- liblog/include/log/log_read.h | 63 ++-------------------------- liblog/logger_read.cpp | 30 +++++-------- liblog/logprint.cpp | 45 ++++++-------------- liblog/pmsg_reader.cpp | 28 ++++++------- liblog/tests/liblog_test.cpp | 12 +++--- logcat/logcat.cpp | 7 ++-- logd/LogBufferElement.cpp | 4 +- logd/LogTags.cpp | 4 +- logd/tests/logd_test.cpp | 55 ++++++------------------ 10 files changed, 66 insertions(+), 184 deletions(-) diff --git a/debuggerd/libdebuggerd/tombstone.cpp b/debuggerd/libdebuggerd/tombstone.cpp index da2ba5877..1993840ab 100644 --- a/debuggerd/libdebuggerd/tombstone.cpp +++ b/debuggerd/libdebuggerd/tombstone.cpp @@ -595,7 +595,7 @@ static void dump_log_file(log_t* log, pid_t pid, const char* filename, unsigned } AndroidLogEntry e; char buf[512]; - if (android_log_processBinaryLogBuffer(&log_entry.entry_v1, &e, g_eventTagMap, buf, + if (android_log_processBinaryLogBuffer(&log_entry.entry, &e, g_eventTagMap, buf, sizeof(buf)) == 0) { _LOG(log, logtype::LOGS, "%s.%03d %5d %5d %c %-8.*s: %s\n", timeBuf, log_entry.entry.nsec / 1000000, log_entry.entry.pid, log_entry.entry.tid, 'I', diff --git a/liblog/include/log/log_read.h b/liblog/include/log/log_read.h index 2079e7a82..ee3b250a1 100644 --- a/liblog/include/log/log_read.h +++ b/liblog/include/log/log_read.h @@ -50,53 +50,9 @@ extern "C" { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wzero-length-array" -/* - * The userspace structure for version 1 of the logger_entry ABI. - */ struct logger_entry { - uint16_t len; /* length of the payload */ - uint16_t __pad; /* no matter what, we get 2 bytes of padding */ - int32_t pid; /* generating process's pid */ - int32_t tid; /* generating process's tid */ - int32_t sec; /* seconds since Epoch */ - int32_t nsec; /* nanoseconds */ - char msg[0]; /* the entry's payload */ -}; - -/* - * The userspace structure for version 2 of the logger_entry ABI. - */ -struct logger_entry_v2 { uint16_t len; /* length of the payload */ - uint16_t hdr_size; /* sizeof(struct logger_entry_v2) */ - int32_t pid; /* generating process's pid */ - int32_t tid; /* generating process's tid */ - int32_t sec; /* seconds since Epoch */ - int32_t nsec; /* nanoseconds */ - uint32_t euid; /* effective UID of logger */ - char msg[0]; /* the entry's payload */ -} __attribute__((__packed__)); - -/* - * The userspace structure for version 3 of the logger_entry ABI. - */ -struct logger_entry_v3 { - uint16_t len; /* length of the payload */ - uint16_t hdr_size; /* sizeof(struct logger_entry_v3) */ - int32_t pid; /* generating process's pid */ - int32_t tid; /* generating process's tid */ - int32_t sec; /* seconds since Epoch */ - int32_t nsec; /* nanoseconds */ - uint32_t lid; /* log id of the payload */ - char msg[0]; /* the entry's payload */ -} __attribute__((__packed__)); - -/* - * The userspace structure for version 4 of the logger_entry ABI. - */ -struct logger_entry_v4 { - uint16_t len; /* length of the payload */ - uint16_t hdr_size; /* sizeof(struct logger_entry_v4) */ + uint16_t hdr_size; /* sizeof(struct logger_entry) */ int32_t pid; /* generating process's pid */ uint32_t tid; /* generating process's tid */ uint32_t sec; /* seconds since Epoch */ @@ -124,11 +80,7 @@ struct logger_entry_v4 { struct log_msg { union { unsigned char buf[LOGGER_ENTRY_MAX_LEN + 1]; - struct logger_entry_v4 entry; - struct logger_entry_v4 entry_v4; - struct logger_entry_v3 entry_v3; - struct logger_entry_v2 entry_v2; - struct logger_entry entry_v1; + struct logger_entry entry; } __attribute__((aligned(4))); #ifdef __cplusplus /* Matching log_time operators */ @@ -162,19 +114,12 @@ struct log_msg { } char* msg() { unsigned short hdr_size = entry.hdr_size; - if (!hdr_size) { - hdr_size = sizeof(entry_v1); - } - if ((hdr_size < sizeof(entry_v1)) || (hdr_size > sizeof(entry))) { + if (hdr_size != sizeof(entry)) { return nullptr; } return reinterpret_cast(buf) + hdr_size; } - unsigned int len() { - return (entry.hdr_size ? entry.hdr_size - : static_cast(sizeof(entry_v1))) + - entry.len; - } + unsigned int len() { return entry.hdr_size + entry.len; } #endif }; diff --git a/liblog/logger_read.cpp b/liblog/logger_read.cpp index ff816b768..939d3af69 100644 --- a/liblog/logger_read.cpp +++ b/liblog/logger_read.cpp @@ -273,34 +273,26 @@ static int android_transport_read(struct android_log_logger_list* logger_list, struct log_msg* log_msg) { int ret = (*transp->transport->read)(logger_list, transp, log_msg); + if (ret < 0) { + return ret; + } + if (ret > (int)sizeof(*log_msg)) { ret = sizeof(*log_msg); } transp->ret = ret; - /* propagate errors, or make sure len & hdr_size members visible */ - if (ret < (int)(sizeof(log_msg->entry.len) + sizeof(log_msg->entry.hdr_size))) { - if (ret >= (int)sizeof(log_msg->entry.len)) { - log_msg->entry.len = 0; - } - return ret; - } - - /* hdr_size correction (logger_entry -> logger_entry_v2+ conversion) */ - if (log_msg->entry_v2.hdr_size == 0) { - log_msg->entry_v2.hdr_size = sizeof(struct logger_entry); - } - if ((log_msg->entry_v2.hdr_size < sizeof(log_msg->entry_v1)) || - (log_msg->entry_v2.hdr_size > sizeof(log_msg->entry))) { + if (ret < static_cast(sizeof(log_msg->entry))) { return -EINVAL; } - /* len validation */ - if (ret <= log_msg->entry_v2.hdr_size) { - log_msg->entry.len = 0; - } else { - log_msg->entry.len = ret - log_msg->entry_v2.hdr_size; + if (log_msg->entry.hdr_size != sizeof(log_msg->entry)) { + return -EINVAL; + } + + if (log_msg->entry.len > ret - log_msg->entry.hdr_size) { + return -EINVAL; } return ret; diff --git a/liblog/logprint.cpp b/liblog/logprint.cpp index 82fbafd70..4b61828b2 100644 --- a/liblog/logprint.cpp +++ b/liblog/logprint.cpp @@ -532,18 +532,12 @@ int android_log_processLogBuffer(struct logger_entry* buf, AndroidLogEntry* entr int i; char* msg = buf->msg; - struct logger_entry_v2* buf2 = (struct logger_entry_v2*)buf; - if (buf2->hdr_size) { - if ((buf2->hdr_size < sizeof(((struct log_msg*)NULL)->entry_v1)) || - (buf2->hdr_size > sizeof(((struct log_msg*)NULL)->entry))) { - fprintf(stderr, "+++ LOG: entry illegal hdr_size\n"); - return -1; - } - msg = ((char*)buf2) + buf2->hdr_size; - if (buf2->hdr_size >= sizeof(struct logger_entry_v4)) { - entry->uid = ((struct logger_entry_v4*)buf)->uid; - } + if (buf->hdr_size != sizeof(struct logger_entry)) { + fprintf(stderr, "+++ LOG: entry illegal hdr_size\n"); + return -1; } + entry->uid = buf->uid; + for (i = 1; i < buf->len; i++) { if (msg[i] == '\0') { if (msgStart == -1) { @@ -993,27 +987,15 @@ int android_log_processBinaryLogBuffer( entry->pid = buf->pid; entry->tid = buf->tid; - /* - * Pull the tag out, fill in some additional details based on incoming - * buffer version (v3 adds lid, v4 adds uid). - */ eventData = (const unsigned char*)buf->msg; - struct logger_entry_v2* buf2 = (struct logger_entry_v2*)buf; - if (buf2->hdr_size) { - if ((buf2->hdr_size < sizeof(((struct log_msg*)NULL)->entry_v1)) || - (buf2->hdr_size > sizeof(((struct log_msg*)NULL)->entry))) { - fprintf(stderr, "+++ LOG: entry illegal hdr_size\n"); - return -1; - } - eventData = ((unsigned char*)buf2) + buf2->hdr_size; - if ((buf2->hdr_size >= sizeof(struct logger_entry_v3)) && - (((struct logger_entry_v3*)buf)->lid == LOG_ID_SECURITY)) { - entry->priority = ANDROID_LOG_WARN; - } - if (buf2->hdr_size >= sizeof(struct logger_entry_v4)) { - entry->uid = ((struct logger_entry_v4*)buf)->uid; - } + if (buf->hdr_size != sizeof(struct logger_entry)) { + fprintf(stderr, "+++ LOG: entry illegal hdr_size\n"); + return -1; } + if (buf->lid == LOG_ID_SECURITY) { + entry->priority = ANDROID_LOG_WARN; + } + entry->uid = buf->uid; inCount = buf->len; if (inCount < sizeof(android_event_header_t)) return -1; auto* event_header = reinterpret_cast(eventData); @@ -1069,9 +1051,6 @@ int android_log_processBinaryLogBuffer( if ((result == 1) && fmtStr) { /* We overflowed :-(, let's repaint the line w/o format dressings */ eventData = (const unsigned char*)buf->msg; - if (buf2->hdr_size) { - eventData = ((unsigned char*)buf2) + buf2->hdr_size; - } eventData += 4; outBuf = messageBuf; outRemaining = messageBufLen - 1; diff --git a/liblog/pmsg_reader.cpp b/liblog/pmsg_reader.cpp index ce923f332..0877c6875 100644 --- a/liblog/pmsg_reader.cpp +++ b/liblog/pmsg_reader.cpp @@ -144,7 +144,7 @@ static int pmsgRead(struct android_log_logger_list* logger_list, ((logger_list->start.tv_sec != buf.l.realtime.tv_sec) || (logger_list->start.tv_nsec <= buf.l.realtime.tv_nsec)))) && (!logger_list->pid || (logger_list->pid == buf.p.pid))) { - char* msg = log_msg->entry_v4.msg; + char* msg = log_msg->entry.msg; *msg = buf.prio; fd = atomic_load(&transp->context.fd); if (fd <= 0) { @@ -158,16 +158,16 @@ static int pmsgRead(struct android_log_logger_list* logger_list, return -EIO; } - log_msg->entry_v4.len = buf.p.len - sizeof(buf) + sizeof(buf.prio); - log_msg->entry_v4.hdr_size = sizeof(log_msg->entry_v4); - log_msg->entry_v4.pid = buf.p.pid; - log_msg->entry_v4.tid = buf.l.tid; - log_msg->entry_v4.sec = buf.l.realtime.tv_sec; - log_msg->entry_v4.nsec = buf.l.realtime.tv_nsec; - log_msg->entry_v4.lid = buf.l.id; - log_msg->entry_v4.uid = buf.p.uid; + log_msg->entry.len = buf.p.len - sizeof(buf) + sizeof(buf.prio); + log_msg->entry.hdr_size = sizeof(log_msg->entry); + log_msg->entry.pid = buf.p.pid; + log_msg->entry.tid = buf.l.tid; + log_msg->entry.sec = buf.l.realtime.tv_sec; + log_msg->entry.nsec = buf.l.realtime.tv_nsec; + log_msg->entry.lid = buf.l.id; + log_msg->entry.uid = buf.p.uid; - return ret + sizeof(buf.prio) + log_msg->entry_v4.hdr_size; + return ret + sizeof(buf.prio) + log_msg->entry.hdr_size; } fd = atomic_load(&transp->context.fd); @@ -215,7 +215,7 @@ ssize_t __android_log_pmsg_file_read(log_id_t logId, char prio, const char* pref struct android_log_transport_context transp; struct content { struct listnode node; - struct logger_entry_v4 entry; + struct logger_entry entry; } * content; struct names { struct listnode node; @@ -269,12 +269,12 @@ ssize_t __android_log_pmsg_file_read(log_id_t logId, char prio, const char* pref /* Read the file content */ while (pmsgRead(&logger_list, &transp, &transp.logMsg) > 0) { const char* cp; - size_t hdr_size = transp.logMsg.entry.hdr_size ? transp.logMsg.entry.hdr_size - : sizeof(transp.logMsg.entry_v1); + size_t hdr_size = transp.logMsg.entry.hdr_size; + char* msg = (char*)&transp.logMsg + hdr_size; const char* split = NULL; - if ((hdr_size < sizeof(transp.logMsg.entry_v1)) || (hdr_size > sizeof(transp.logMsg.entry))) { + if (hdr_size != sizeof(transp.logMsg.entry)) { continue; } /* Check for invalid sequence number */ diff --git a/liblog/tests/liblog_test.cpp b/liblog/tests/liblog_test.cpp index 94c4fbb05..c402e20fa 100644 --- a/liblog/tests/liblog_test.cpp +++ b/liblog/tests/liblog_test.cpp @@ -385,7 +385,7 @@ static void bswrite_test(const char* message) { fprintf(stderr, "Expect \"Binary log entry conversion failed\"\n"); } int processBinaryLogBuffer = android_log_processBinaryLogBuffer( - &log_msg.entry_v1, &entry, NULL, msgBuf, sizeof(msgBuf)); + &log_msg.entry, &entry, nullptr, msgBuf, sizeof(msgBuf)); EXPECT_EQ((length == total) ? 0 : -1, processBinaryLogBuffer); if ((processBinaryLogBuffer == 0) || entry.message) { size_t line_overhead = 20; @@ -469,8 +469,7 @@ static void buf_write_test(const char* message) { AndroidLogFormat* logformat = android_log_format_new(); EXPECT_TRUE(NULL != logformat); AndroidLogEntry entry; - int processLogBuffer = - android_log_processLogBuffer(&log_msg.entry_v1, &entry); + int processLogBuffer = android_log_processLogBuffer(&log_msg.entry, &entry); EXPECT_EQ(0, processLogBuffer); if (processLogBuffer == 0) { size_t line_overhead = 11; @@ -1013,8 +1012,7 @@ TEST(liblog, __android_log_buf_print__maxtag) { AndroidLogFormat* logformat = android_log_format_new(); EXPECT_TRUE(NULL != logformat); AndroidLogEntry entry; - int processLogBuffer = - android_log_processLogBuffer(&log_msg.entry_v1, &entry); + int processLogBuffer = android_log_processLogBuffer(&log_msg.entry, &entry); EXPECT_EQ(0, processLogBuffer); if (processLogBuffer == 0) { fflush(stderr); @@ -2507,8 +2505,8 @@ static void create_android_logger(const char* (*fn)(uint32_t tag, EXPECT_TRUE(NULL != logformat); AndroidLogEntry entry; char msgBuf[1024]; - int processBinaryLogBuffer = android_log_processBinaryLogBuffer( - &log_msg.entry_v1, &entry, NULL, msgBuf, sizeof(msgBuf)); + int processBinaryLogBuffer = + android_log_processBinaryLogBuffer(&log_msg.entry, &entry, nullptr, msgBuf, sizeof(msgBuf)); EXPECT_EQ(0, processBinaryLogBuffer); if (processBinaryLogBuffer == 0) { int line_overhead = 20; diff --git a/logcat/logcat.cpp b/logcat/logcat.cpp index 4dcb3383e..f164a1224 100644 --- a/logcat/logcat.cpp +++ b/logcat/logcat.cpp @@ -337,13 +337,12 @@ static void processBuffer(android_logcat_context_internal* context, context->eventTagMap = android_openEventTagMap(nullptr); context->hasOpenedEventTagMap = true; } - err = android_log_processBinaryLogBuffer( - &buf->entry_v1, &entry, context->eventTagMap, binaryMsgBuf, - sizeof(binaryMsgBuf)); + err = android_log_processBinaryLogBuffer(&buf->entry, &entry, context->eventTagMap, + binaryMsgBuf, sizeof(binaryMsgBuf)); // printf(">>> pri=%d len=%d msg='%s'\n", // entry.priority, entry.messageLen, entry.message); } else { - err = android_log_processLogBuffer(&buf->entry_v1, &entry); + err = android_log_processLogBuffer(&buf->entry, &entry); } if ((err < 0) && !context->debug) return; diff --git a/logd/LogBufferElement.cpp b/logd/LogBufferElement.cpp index 5c43e18c7..ec8193355 100644 --- a/logd/LogBufferElement.cpp +++ b/logd/LogBufferElement.cpp @@ -245,9 +245,9 @@ size_t LogBufferElement::populateDroppedMessage(char*& buffer, LogBuffer* parent } log_time LogBufferElement::flushTo(SocketClient* reader, LogBuffer* parent, bool lastSame) { - struct logger_entry_v4 entry = {}; + struct logger_entry entry = {}; - entry.hdr_size = sizeof(struct logger_entry_v4); + entry.hdr_size = sizeof(struct logger_entry); entry.lid = mLogId; entry.pid = mPid; entry.tid = mTid; diff --git a/logd/LogTags.cpp b/logd/LogTags.cpp index f19e7b09d..0cc7886ea 100644 --- a/logd/LogTags.cpp +++ b/logd/LogTags.cpp @@ -311,9 +311,7 @@ void LogTags::ReadPersistEventLogTags() { if (log_msg.entry.len <= sizeof(uint32_t)) continue; uint32_t Tag = get4LE(msg); if (Tag != TAG_DEF_LOG_TAG) continue; - uid_t uid = (log_msg.entry.hdr_size >= sizeof(logger_entry_v4)) - ? log_msg.entry.uid - : AID_ROOT; + uid_t uid = log_msg.entry.uid; std::string Name; std::string Format; diff --git a/logd/tests/logd_test.cpp b/logd/tests/logd_test.cpp index 80625a704..f47bee1c0 100644 --- a/logd/tests/logd_test.cpp +++ b/logd/tests/logd_test.cpp @@ -241,47 +241,18 @@ TEST(logd, statistics) { static void caught_signal(int /* signum */) { } -static void dump_log_msg(const char* prefix, log_msg* msg, unsigned int version, - int lid) { +static void dump_log_msg(const char* prefix, log_msg* msg, int lid) { std::cout << std::flush; std::cerr << std::flush; fflush(stdout); fflush(stderr); - switch (msg->entry.hdr_size) { - case 0: - version = 1; - break; + EXPECT_EQ(sizeof(logger_entry), msg->entry.hdr_size); - case sizeof(msg->entry_v2): /* PLUS case sizeof(msg->entry_v3): */ - if (version == 0) { - version = (msg->entry_v3.lid < LOG_ID_MAX) ? 3 : 2; - } - break; - - case sizeof(msg->entry_v4): - if (version == 0) { - version = 4; - } - break; - } - - fprintf(stderr, "%s: v%u[%u] ", prefix, version, msg->len()); - if (version != 1) { - fprintf(stderr, "hdr_size=%u ", msg->entry.hdr_size); - } - fprintf(stderr, "pid=%u tid=%u %u.%09u ", msg->entry.pid, msg->entry.tid, - msg->entry.sec, msg->entry.nsec); - switch (version) { - case 1: - break; - case 2: - fprintf(stderr, "euid=%u ", msg->entry_v2.euid); - break; - case 3: - default: - lid = msg->entry.lid; - break; - } + fprintf(stderr, "%s: [%u] ", prefix, msg->len()); + fprintf(stderr, "hdr_size=%u ", msg->entry.hdr_size); + fprintf(stderr, "pid=%u tid=%u %u.%09u ", msg->entry.pid, msg->entry.tid, msg->entry.sec, + msg->entry.nsec); + lid = msg->entry.lid; switch (lid) { case 0: @@ -584,11 +555,11 @@ void timeout_negative(const char* command) { } if (content_wrap) { - dump_log_msg("wrap", &msg_wrap, 3, -1); + dump_log_msg("wrap", &msg_wrap, -1); } if (content_timeout) { - dump_log_msg("timeout", &msg_timeout, 3, -1); + dump_log_msg("timeout", &msg_timeout, -1); } EXPECT_TRUE(written); @@ -721,11 +692,11 @@ TEST(logd, timeout) { } if (content_wrap) { - dump_log_msg("wrap", &msg_wrap, 3, -1); + dump_log_msg("wrap", &msg_wrap, -1); } if (content_timeout) { - dump_log_msg("timeout", &msg_timeout, 3, -1); + dump_log_msg("timeout", &msg_timeout, -1); } if (content_wrap || !content_timeout) { @@ -776,7 +747,7 @@ TEST(logd, SNDTIMEO) { EXPECT_TRUE(read_one); if (read_one) { - dump_log_msg("user", &msg, 3, -1); + dump_log_msg("user", &msg, -1); } fprintf(stderr, "Sleep for >%d seconds logd SO_SNDTIMEO ...\n", sndtimeo); @@ -794,7 +765,7 @@ TEST(logd, SNDTIMEO) { EXPECT_EQ(0, recv_ret); if (recv_ret > 0) { - dump_log_msg("user", &msg, 3, -1); + dump_log_msg("user", &msg, -1); } EXPECT_EQ(0, save_errno);