From 8ccb8be42fc9a99d00ba0ae737edc7de97f00969 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 27 Sep 2022 12:56:58 -0600 Subject: [PATCH] Handle accidental null arguments to Trace APIs. We have @NonNull annotations, but not everyone is watching the warning messages related to them. Make the implementation a bit more robust by swapping in the "(null)" literal when someone accidentally leaks one through. Bug: 249325721 Test: atest FrameworksCoreTests:TraceTest Change-Id: Ic0f5a0236fbce88e086f3d68b2e5c6ed6b0aa33c --- core/java/android/os/Trace.java | 9 ------- core/jni/android_os_Trace.cpp | 27 ++++++++++++------- .../coretests/src/android/os/TraceTest.java | 16 ++++++++++- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/core/java/android/os/Trace.java b/core/java/android/os/Trace.java index 4965057a7fdbc..8bfa0e99e3b43 100644 --- a/core/java/android/os/Trace.java +++ b/core/java/android/os/Trace.java @@ -340,9 +340,6 @@ public final class Trace { * @hide */ public static void instant(long traceTag, String methodName) { - if (methodName == null) { - throw new IllegalArgumentException("methodName cannot be null"); - } if (isTagEnabled(traceTag)) { nativeInstant(traceTag, methodName); } @@ -357,12 +354,6 @@ public final class Trace { * @hide */ public static void instantForTrack(long traceTag, String trackName, String methodName) { - if (trackName == null) { - throw new IllegalArgumentException("trackName cannot be null"); - } - if (methodName == null) { - throw new IllegalArgumentException("methodName cannot be null"); - } if (isTagEnabled(traceTag)) { nativeInstantForTrack(traceTag, trackName, methodName); } diff --git a/core/jni/android_os_Trace.cpp b/core/jni/android_os_Trace.cpp index 1c61c7be4414c..ffacd9c8a95ba 100644 --- a/core/jni/android_os_Trace.cpp +++ b/core/jni/android_os_Trace.cpp @@ -22,6 +22,8 @@ #include +static constexpr const char* kNullReplacement = "(null)"; + namespace android { inline static void sanitizeString(char* str) { @@ -36,6 +38,11 @@ inline static void sanitizeString(char* str) { template inline static void withString(JNIEnv* env, jstring jstr, F callback) { + if (CC_UNLIKELY(jstr == nullptr)) { + callback(kNullReplacement); + return; + } + // 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. @@ -52,14 +59,14 @@ inline static void withString(JNIEnv* env, jstring jstr, F callback) { static void android_os_Trace_nativeTraceCounter(JNIEnv* env, jclass, jlong tag, jstring nameStr, jlong value) { - withString(env, nameStr, [tag, value](char* str) { + withString(env, nameStr, [tag, value](const char* str) { atrace_int64(tag, str, value); }); } static void android_os_Trace_nativeTraceBegin(JNIEnv* env, jclass, jlong tag, jstring nameStr) { - withString(env, nameStr, [tag](char* str) { + withString(env, nameStr, [tag](const char* str) { atrace_begin(tag, str); }); } @@ -70,22 +77,22 @@ static void android_os_Trace_nativeTraceEnd(JNIEnv*, jclass, jlong tag) { static void android_os_Trace_nativeAsyncTraceBegin(JNIEnv* env, jclass, jlong tag, jstring nameStr, jint cookie) { - withString(env, nameStr, [tag, cookie](char* str) { + withString(env, nameStr, [tag, cookie](const char* str) { atrace_async_begin(tag, str, cookie); }); } static void android_os_Trace_nativeAsyncTraceEnd(JNIEnv* env, jclass, jlong tag, jstring nameStr, jint cookie) { - withString(env, nameStr, [tag, cookie](char* str) { + withString(env, nameStr, [tag, cookie](const char* str) { atrace_async_end(tag, str, cookie); }); } static void android_os_Trace_nativeAsyncTraceForTrackBegin(JNIEnv* env, jclass, jlong tag, jstring trackStr, jstring nameStr, jint cookie) { - withString(env, trackStr, [env, tag, nameStr, cookie](char* track) { - withString(env, nameStr, [tag, track, cookie](char* name) { + withString(env, trackStr, [env, tag, nameStr, cookie](const char* track) { + withString(env, nameStr, [tag, track, cookie](const char* name) { atrace_async_for_track_begin(tag, track, name, cookie); }); }); @@ -93,7 +100,7 @@ static void android_os_Trace_nativeAsyncTraceForTrackBegin(JNIEnv* env, jclass, static void android_os_Trace_nativeAsyncTraceForTrackEnd(JNIEnv* env, jclass, jlong tag, jstring trackStr, jint cookie) { - withString(env, trackStr, [tag, cookie](char* track) { + withString(env, trackStr, [tag, cookie](const char* track) { atrace_async_for_track_end(tag, track, cookie); }); } @@ -108,15 +115,15 @@ static void android_os_Trace_nativeSetTracingEnabled(JNIEnv*, jclass, jboolean e static void android_os_Trace_nativeInstant(JNIEnv* env, jclass, jlong tag, jstring nameStr) { - withString(env, nameStr, [tag](char* str) { + withString(env, nameStr, [tag](const char* str) { atrace_instant(tag, str); }); } static void android_os_Trace_nativeInstantForTrack(JNIEnv* env, jclass, jlong tag, jstring trackStr, jstring nameStr) { - withString(env, trackStr, [env, tag, nameStr](char* track) { - withString(env, nameStr, [tag, track](char* name) { + withString(env, trackStr, [env, tag, nameStr](const char* track) { + withString(env, nameStr, [tag, track](const char* name) { atrace_instant_for_track(tag, track, name); }); }); diff --git a/core/tests/coretests/src/android/os/TraceTest.java b/core/tests/coretests/src/android/os/TraceTest.java index 5cad549d39bf0..d07187c2c9d80 100644 --- a/core/tests/coretests/src/android/os/TraceTest.java +++ b/core/tests/coretests/src/android/os/TraceTest.java @@ -33,7 +33,21 @@ public class TraceTest extends AndroidTestCase private int eMethodCalls = 0; private int fMethodCalls = 0; private int gMethodCalls = 0; - + + public void testNullStrings() { + Trace.traceCounter(Trace.TRACE_TAG_ACTIVITY_MANAGER, null, 42); + Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, null); + + Trace.asyncTraceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, null, 42); + Trace.asyncTraceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER, null, 42); + + Trace.asyncTraceForTrackBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, null, null, 42); + Trace.asyncTraceForTrackEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER, null, 42); + + Trace.instant(Trace.TRACE_TAG_ACTIVITY_MANAGER, null); + Trace.instantForTrack(Trace.TRACE_TAG_ACTIVITY_MANAGER, null, null); + } + @SmallTest public void testNativeTracingFromJava() {