From 121e478ed4e5d1d93826c3015e0006055d03b24d Mon Sep 17 00:00:00 2001
From: John Reck <jreck@google.com>
Date: Wed, 12 Jun 2019 16:57:59 -0700
Subject: [PATCH] Fix buffer overflow in Trace's JNI layer

There doesn't appear to be anything better
than blindly make a buffer 4x the length of a
jstring if a maximum buffer size is useful.

So do that.

This does slightly regress Trace's performance.

Before:

android.os.TracePerfTest:INSTRUMENTATION_STATUS: enabled_mean=14
INSTRUMENTATION_STATUS: enabled_median=14
INSTRUMENTATION_STATUS: enabled_min=14
INSTRUMENTATION_STATUS: enabled_standardDeviation=0
INSTRUMENTATION_STATUS_CODE: -1
.INSTRUMENTATION_STATUS: beginEndSection_mean=3087
INSTRUMENTATION_STATUS: beginEndSection_median=3059
INSTRUMENTATION_STATUS: beginEndSection_min=3020
INSTRUMENTATION_STATUS: beginEndSection_standardDeviation=75
INSTRUMENTATION_STATUS_CODE: -1
.INSTRUMENTATION_STATUS: counter_mean=1893
INSTRUMENTATION_STATUS: counter_median=1900
INSTRUMENTATION_STATUS: counter_min=1851
INSTRUMENTATION_STATUS: counter_standardDeviation=26
INSTRUMENTATION_STATUS_CODE: -1
.INSTRUMENTATION_STATUS: asyncBeginEnd_mean=4281
INSTRUMENTATION_STATUS: asyncBeginEnd_median=4306
INSTRUMENTATION_STATUS: asyncBeginEnd_min=4184
INSTRUMENTATION_STATUS: asyncBeginEnd_standardDeviation=65
INSTRUMENTATION_STATUS_CODE: -1

After:

android.os.TracePerfTest:INSTRUMENTATION_STATUS: enabled_mean=16
INSTRUMENTATION_STATUS: enabled_median=16
INSTRUMENTATION_STATUS: enabled_min=16
INSTRUMENTATION_STATUS: enabled_standardDeviation=0
INSTRUMENTATION_STATUS_CODE: -1
.INSTRUMENTATION_STATUS: beginEndSection_mean=3869
INSTRUMENTATION_STATUS: beginEndSection_median=3864
INSTRUMENTATION_STATUS: beginEndSection_min=3840
INSTRUMENTATION_STATUS: beginEndSection_standardDeviation=21
INSTRUMENTATION_STATUS_CODE: -1
.INSTRUMENTATION_STATUS: counter_mean=2511
INSTRUMENTATION_STATUS: counter_median=2503
INSTRUMENTATION_STATUS: counter_min=2480
INSTRUMENTATION_STATUS: counter_standardDeviation=35
INSTRUMENTATION_STATUS_CODE: -1
.INSTRUMENTATION_STATUS: asyncBeginEnd_mean=5348
INSTRUMENTATION_STATUS: asyncBeginEnd_median=5344
INSTRUMENTATION_STATUS: asyncBeginEnd_min=5318
INSTRUMENTATION_STATUS: asyncBeginEnd_standardDeviation=28
INSTRUMENTATION_STATUS_CODE: -1

But it also works correctly and doesn't crash, and that seems worth it.

Fixes: 133104515
Test: systrace still works, AtraceHostTest passes, verified
    Trace.beginSection of 4-byte utf8 octets showed up in systrace
Change-Id: Ie2e31227d9380df4190f9bc09ecd67f8a982827f
---
 core/jni/android_os_Trace.cpp | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/core/jni/android_os_Trace.cpp b/core/jni/android_os_Trace.cpp
index 81428dc02fb47..bd82bd91c55d6 100644
--- a/core/jni/android_os_Trace.cpp
+++ b/core/jni/android_os_Trace.cpp
@@ -24,26 +24,29 @@
 
 namespace android {
 
-inline static void sanitizeString(char* str, size_t size) {
-    for (size_t i = 0; i < size; i++) {
-        char c = str[i];
-        if (c == '\0' || c == '\n' || c == '|') {
-            str[i] = ' ';
+inline static void sanitizeString(char* str) {
+    while (*str) {
+        char c = *str;
+        if (c == '\n' || c == '|') {
+            *str = ' ';
         }
+        str++;
     }
 }
 
-inline static void getString(JNIEnv* env, jstring jstring, char* outBuffer, jsize maxSize) {
-    jsize size = std::min(env->GetStringLength(jstring), maxSize);
-    env->GetStringUTFRegion(jstring, 0, size, outBuffer);
-    sanitizeString(outBuffer, size);
-    outBuffer[size] = '\0';
-}
-
 template<typename F>
 inline static void withString(JNIEnv* env, jstring jstr, F callback) {
-    std::array<char, 1024> buffer;
-    getString(env, jstr, buffer.data(), buffer.size());
+    // We need to handle the worst case of 1 character -> 4 bytes
+    // So make a buffer of size 4097 and let it hold a string with a maximum length
+    // of 1024. The extra last byte for the null terminator.
+    std::array<char, 4097> buffer;
+    // We have no idea of knowing how much data GetStringUTFRegion wrote, so null it out in
+    // advance so we can have a reliable null terminator
+    memset(buffer.data(), 0, buffer.size());
+    jsize size = std::min(env->GetStringLength(jstr), 1024);
+    env->GetStringUTFRegion(jstr, 0, size, buffer.data());
+    sanitizeString(buffer.data());
+
     callback(buffer.data());
 }