commit 35d7149ef649649ec10d9797ef85a267bad0300e
author Anthony Stange <stange@google.com> Fri Oct 23 09:43:43 2020 -0400
committer TreeHugger Robot <treehugger-gerrit@google.com> Fri Oct 23 19:22:39 2020 +0000
tree 2a69f8cf33041ae4e1ac3ca22592d18e422e28ef
parent 25dd3bbdbd9af932d0c0154af291a3886218ba79

Make logging logic safer

1) Handle case where vsnprintf indicates the log message was truncated
to avoid a buffer overflow
2) Add a mutex to ensure that two log invocations from separate threads
don't overwrite each other's logs

Fixes: 171515363
Test: Load CHRE w/ sensor world and observe no duplicate or out of
order logs
Change-Id: I736f63ce41032d4382f1c59ae004fee005940ba3

platform/aoc/log.cc

diff --git a/platform/aoc/log.cc b/platform/aoc/log.cc
index 689cef1..186029e 100644
--- a/platform/aoc/log.cc
+++ b/platform/aoc/log.cc
@@ -17,6 +17,7 @@
 #include "chre/target_platform/log.h"
 
 #include "chre/core/event_loop_manager.h"
+#include "chre/platform/mutex.h"
 #include "chre/platform/system_time.h"
 
 #include <endian.h>
@@ -25,6 +26,7 @@
 namespace {
 constexpr size_t kChreLogBufferSize = CHRE_MESSAGE_TO_HOST_MAX_SIZE;
 char logBuffer[kChreLogBufferSize];
+Mutex logMutex;
 }  // namespace
 
 void log(enum chreLogLevel level, const char *formatStr, ...) {
@@ -37,6 +39,8 @@
 // TODO: b/146164384 - We will need to batch logs rather than send them
 // one at a time to avoid waking the AP.
 void vaLog(enum chreLogLevel level, const char *format, va_list args) {
+  LockGuard<Mutex> lockGuard(logMutex);
+
   auto &hostCommsMgr =
       chre::EventLoopManagerSingleton::get()->getHostCommsManager();
 
@@ -49,11 +53,18 @@
   memcpy(&logBuffer[logBufIndex], &currentTimeNs, sizeof(uint64_t));
   logBufIndex += sizeof(uint64_t);
 
-  int msgLen = vsnprintf(&logBuffer[logBufIndex],
-                         kChreLogBufferSize - logBufIndex, format, args);
+  int remainingBytes = kChreLogBufferSize - logBufIndex;
+  int msgLen = vsnprintf(&logBuffer[logBufIndex], remainingBytes, format, args);
   if (msgLen >= 0) {
-    // msgLen doesn't include the terminating null char.
-    logBufIndex += msgLen + 1;
+    // vsnprintf will truncate the log and add a terminating null if it's over
+    // the buffer size which is preferred.
+    if (msgLen >= remainingBytes) {
+      msgLen = remainingBytes;
+    } else {
+      // msgLen doesn't include the terminating null char.
+      msgLen++;
+    }
+    logBufIndex += msgLen;
 
     hostCommsMgr.sendLogMessage(reinterpret_cast<uint8_t *>(logBuffer),
                                 logBufIndex);