From be0f4abfaff2f88645bc709b686619a00977e782 Mon Sep 17 00:00:00 2001
From: Tom Cherry <tomcherry@google.com>
Date: Fri, 23 Aug 2019 09:09:40 -0700
Subject: [PATCH] logd: Allow setDropped() to be called on already dropped
 messages

rprichard@ pointed out a bug where LogBufferElement::setDropped()
reallocates mMsg to the size of mMsgLen in the case where getTag() !=
0.  However, mMsgLen is in a union with mDroppedCount and
mDroppedCount is the value used when a message is already dropped.
Therefore, it's possible that logd uses the wrong value and allocates
much more memory than intended.

We do call setDropped() on elements that have already been dropped in
LogBufferElementLast::coalesce(), so this is not a superfluous issue.

To simplify this even more, this code puts an mTag in a union with
mMsg, such that if mDropped is true, there will never be an allocated
message; mTag will be directly referred to instead.  This also reduces
the number of allocations needed very slightly.

Test: logd/liblog/logcat unit tests.
Change-Id: Ia1bfba076439fe31c745a243283d41902bca45ac
---
 logd/LogBufferElement.cpp | 57 +++++++++++++++++++++------------------
 logd/LogBufferElement.h   |  5 +++-
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/logd/LogBufferElement.cpp b/logd/LogBufferElement.cpp
index 2fd9f95e8..82c4fb978 100644
--- a/logd/LogBufferElement.cpp
+++ b/logd/LogBufferElement.cpp
@@ -56,14 +56,7 @@ LogBufferElement::LogBufferElement(const LogBufferElement& elem)
       mLogId(elem.mLogId),
       mDropped(elem.mDropped) {
     if (mDropped) {
-        if (elem.isBinary() && elem.mMsg != nullptr) {
-            // for the following "len" value, refer to : setDropped(uint16_t value), getTag()
-            const int len = sizeof(android_event_header_t);
-            mMsg = new char[len];
-            memcpy(mMsg, elem.mMsg, len);
-        } else {
-            mMsg = nullptr;
-        }
+        mTag = elem.getTag();
     } else {
         mMsg = new char[mMsgLen];
         memcpy(mMsg, elem.mMsg, mMsgLen);
@@ -71,31 +64,43 @@ LogBufferElement::LogBufferElement(const LogBufferElement& elem)
 }
 
 LogBufferElement::~LogBufferElement() {
-    delete[] mMsg;
+    if (!mDropped) {
+        delete[] mMsg;
+    }
 }
 
 uint32_t LogBufferElement::getTag() const {
-    return (isBinary() &&
-            ((mDropped && mMsg != nullptr) ||
-             (!mDropped && mMsgLen >= sizeof(android_event_header_t))))
-               ? reinterpret_cast<const android_event_header_t*>(mMsg)->tag
-               : 0;
+    // Binary buffers have no tag.
+    if (!isBinary()) {
+        return 0;
+    }
+
+    // Dropped messages store the tag in place of mMsg.
+    if (mDropped) {
+        return mTag;
+    }
+
+    // For non-dropped messages, we get the tag from the message header itself.
+    if (mMsgLen < sizeof(android_event_header_t)) {
+        return 0;
+    }
+
+    return reinterpret_cast<const android_event_header_t*>(mMsg)->tag;
 }
 
 uint16_t LogBufferElement::setDropped(uint16_t value) {
-    // The tag information is saved in mMsg data, if the tag is non-zero
-    // save only the information needed to get the tag.
-    if (getTag() != 0) {
-        if (mMsgLen > sizeof(android_event_header_t)) {
-            char* truncated_msg = new char[sizeof(android_event_header_t)];
-            memcpy(truncated_msg, mMsg, sizeof(android_event_header_t));
-            delete[] mMsg;
-            mMsg = truncated_msg;
-        }  // mMsgLen == sizeof(android_event_header_t), already at minimum.
-    } else {
-        delete[] mMsg;
-        mMsg = nullptr;
+    if (mDropped) {
+        return mDroppedCount = value;
     }
+
+    // The tag information is saved in mMsg data, which is in a union with mTag, used after mDropped
+    // is set to true. Therefore we save the tag value aside, delete mMsg, then set mTag to the tag
+    // value in its place.
+    auto old_tag = getTag();
+    delete[] mMsg;
+    mMsg = nullptr;
+
+    mTag = old_tag;
     mDropped = true;
     return mDroppedCount = value;
 }
diff --git a/logd/LogBufferElement.h b/logd/LogBufferElement.h
index 57b0a95bb..da4991b28 100644
--- a/logd/LogBufferElement.h
+++ b/logd/LogBufferElement.h
@@ -41,7 +41,10 @@ class __attribute__((packed)) LogBufferElement {
     const uint32_t mPid;
     const uint32_t mTid;
     log_time mRealTime;
-    char* mMsg;
+    union {
+        char* mMsg;    // mDropped == false
+        int32_t mTag;  // mDropped == true
+    };
     union {
         const uint16_t mMsgLen;  // mDropped == false
         uint16_t mDroppedCount;  // mDropped == true