Add more bound checks and null guards

Author: markushi

sentry-android-core/api/sentry-android-core.api

@@ -566,7 +566,7 @@ public class io/sentry/android/core/anr/AnrException : java/lang/Exception {
 }
 
 public class io/sentry/android/core/anr/AnrProfile {
-	public final field endtimeMs J
+	public final field endTimeMs J
 	public final field stacks Ljava/util/List;
 	public final field startTimeMs J
 	public fun <init> (Ljava/util/List;)V

sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoEventProcessor.java

@@ -830,8 +830,10 @@ private void applyAnrProfile(
       final long anrTimestamp;
       if (anrTimestampObj != null) {
         anrTimestamp = anrTimestampObj;
-      } else {
+      } else if (event.getTimestamp() != null) {
         anrTimestamp = event.getTimestamp().getTime();
+      } else {
+        return;
       }
 
       AnrProfile anrProfile = null;
@@ -858,7 +860,7 @@ private void applyAnrProfile(
       }
 
       options.getLogger().log(SentryLevel.INFO, "ANR profile found");
-      if (anrTimestamp < anrProfile.startTimeMs || anrTimestamp > anrProfile.endtimeMs) {
+      if (anrTimestamp < anrProfile.startTimeMs || anrTimestamp > anrProfile.endTimeMs) {
         options.getLogger().log(SentryLevel.DEBUG, "ANR profile found, but doesn't match");
         return;
       }
@@ -886,6 +888,8 @@ private void applyAnrProfile(
           mechanism.setType("ANR");
           e.setMechanism(mechanism);
         }
+        // Replaces the original ANR exception with the profile-derived one,
+        // as we assume the profiling stacktrace is more detailed
         event.setExceptions(sentryException);
 
         if (profilerId != null) {

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfile.java

@@ -10,7 +10,7 @@ public class AnrProfile {
   public final List<AnrStackTrace> stacks;
 
   public final long startTimeMs;
-  public final long endtimeMs;
+  public final long endTimeMs;
 
   public AnrProfile(List<AnrStackTrace> stacks) {
     this.stacks = new ArrayList<>(stacks.size());
@@ -25,10 +25,10 @@ public AnrProfile(List<AnrStackTrace> stacks) {
       startTimeMs = this.stacks.get(0).timestampMs;
 
       // adding 10s to be less strict around end time
-      endtimeMs = this.stacks.get(this.stacks.size() - 1).timestampMs + 10_000L;
+      endTimeMs = this.stacks.get(this.stacks.size() - 1).timestampMs + 10_000L;
     } else {
       startTimeMs = 0L;
-      endtimeMs = 0L;
+      endTimeMs = 0L;
     }
   }
 }

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfileManager.java

@@ -8,6 +8,7 @@
 import io.sentry.SentryOptions;
 import io.sentry.cache.tape.ObjectQueue;
 import io.sentry.cache.tape.QueueFile;
+import io.sentry.util.Objects;
 import java.io.ByteArrayInputStream;
 import java.io.Closeable;
 import java.io.DataInputStream;
@@ -28,7 +29,11 @@ public class AnrProfileManager implements Closeable {
   @NotNull private final ObjectQueue<AnrStackTrace> queue;
 
   public AnrProfileManager(final @NotNull SentryOptions options) {
-    this(options, new File(options.getCacheDirPath(), "anr_profile"));
+    this(
+        options,
+        new File(
+            Objects.requireNonNull(options.getCacheDirPath(), "cacheDirPath is required"),
+            "anr_profile"));
   }
 
   public AnrProfileManager(final @NotNull SentryOptions options, final @NotNull File file) {

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java

@@ -20,6 +20,7 @@
 import java.io.File;
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -31,6 +32,7 @@ public class AnrProfilingIntegration
   public static final long POLLING_INTERVAL_MS = 66;
   private static final long THRESHOLD_SUSPICION_MS = 1000;
   public static final long THRESHOLD_ANR_MS = 4000;
+  static final int MAX_NUM_STACKS = (int) (10_000 / POLLING_INTERVAL_MS);
 
   private final AtomicBoolean enabled = new AtomicBoolean(true);
   private final Runnable updater = () -> lastMainThreadExecutionTime = SystemClock.uptimeMillis();
@@ -39,6 +41,7 @@ public class AnrProfilingIntegration
       new AutoClosableReentrantLock();
 
   private volatile long lastMainThreadExecutionTime = SystemClock.uptimeMillis();
+  final AtomicInteger numCollectedStacks = new AtomicInteger();
   private volatile MainThreadState mainThreadState = MainThreadState.IDLE;
   private volatile @Nullable AnrProfileManager profileManager;
   private volatile @NotNull ILogger logger = NoOpLogger.getInstance();
@@ -54,6 +57,11 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions
             "SentryAndroidOptions is required");
     this.logger = options.getLogger();
 
+    if (options.getCacheDirPath() == null) {
+      logger.log(SentryLevel.WARNING, "ANR Profiling is enabled but cacheDirPath is not set");
+      return;
+    }
+
     if (((SentryAndroidOptions) options).isEnableAnrProfiling()) {
       addIntegrationToSdkVersion("AnrProfiling");
       AppState.getInstance().addAppStateListener(this);
@@ -71,6 +79,7 @@ public void close() throws IOException {
       if (p != null) {
         p.close();
       }
+      profileManager = null;
     }
   }
 
@@ -138,7 +147,7 @@ public void run() {
         }
       }
     } catch (Throwable t) {
-      logger.log(SentryLevel.WARNING, "Failed execute AnrStacktraceIntegration", t);
+      logger.log(SentryLevel.WARNING, "Failed to execute AnrStacktraceIntegration", t);
     }
   }
 
@@ -152,29 +161,40 @@ protected void checkMainThread(final @NotNull Thread mainThread) throws IOExcept
     }
 
     if (mainThreadState == MainThreadState.IDLE && diff > THRESHOLD_SUSPICION_MS) {
-      logger.log(SentryLevel.DEBUG, "ANR: main thread is suspicious");
+      if (logger.isEnabled(SentryLevel.DEBUG)) {
+        logger.log(SentryLevel.DEBUG, "ANR: main thread is suspicious");
+      }
       mainThreadState = MainThreadState.SUSPICIOUS;
       clearStacks();
     }
 
     // if we are suspicious, we need to collect stack traces
     if (mainThreadState == MainThreadState.SUSPICIOUS
         || mainThreadState == MainThreadState.ANR_DETECTED) {
-      final long start = SystemClock.uptimeMillis();
-      final @NotNull AnrStackTrace trace =
-          new AnrStackTrace(System.currentTimeMillis(), mainThread.getStackTrace());
-      final long duration = SystemClock.uptimeMillis() - start;
-      logger.log(
-          SentryLevel.DEBUG,
-          "AnrWatchdog: capturing main thread stacktrace took " + duration + "ms");
-
-      addStackTrace(trace);
+      if (numCollectedStacks.get() < MAX_NUM_STACKS) {
+        final long start = SystemClock.uptimeMillis();
+        final @NotNull AnrStackTrace trace =
+            new AnrStackTrace(System.currentTimeMillis(), mainThread.getStackTrace());
+        final long duration = SystemClock.uptimeMillis() - start;
+        if (logger.isEnabled(SentryLevel.DEBUG)) {
+          logger.log(
+              SentryLevel.DEBUG,
+              "AnrWatchdog: capturing main thread stacktrace took " + duration + "ms");
+        }
+        addStackTrace(trace);
+      } else {
+        if (logger.isEnabled(SentryLevel.DEBUG)) {
+          logger.log(
+              SentryLevel.DEBUG,
+              "ANR: reached maximum number of collected stack traces, skipping further collection");
+        }
+      }
     }
 
-    // TODO is this still required,
-    // maybe add stop condition
     if (mainThreadState == MainThreadState.SUSPICIOUS && diff > THRESHOLD_ANR_MS) {
-      logger.log(SentryLevel.DEBUG, "ANR: main thread ANR threshold reached");
+      if (logger.isEnabled(SentryLevel.DEBUG)) {
+        logger.log(SentryLevel.DEBUG, "ANR: main thread ANR threshold reached");
+      }
       mainThreadState = MainThreadState.ANR_DETECTED;
     }
   }
@@ -192,8 +212,12 @@ protected AnrProfileManager getProfileManager() {
       if (profileManager == null) {
         final @NotNull SentryOptions opts =
             Objects.requireNonNull(options, "Options can't be null");
+        final @Nullable String cacheDirPath = opts.getCacheDirPath();
+        if (cacheDirPath == null) {
+          throw new IllegalStateException("cacheDirPath is required for ANR profiling");
+        }
         final @NotNull File currentFile =
-            AnrProfileRotationHelper.getFileForRecording(new File(opts.getCacheDirPath()));
+            AnrProfileRotationHelper.getFileForRecording(new File(cacheDirPath));
         profileManager = new AnrProfileManager(opts, currentFile);
       }
 
@@ -202,10 +226,12 @@ protected AnrProfileManager getProfileManager() {
   }
 
   private void clearStacks() throws IOException {
+    numCollectedStacks.set(0);
     getProfileManager().clear();
   }
 
   private void addStackTrace(@NotNull final AnrStackTrace trace) throws IOException {
+    numCollectedStacks.incrementAndGet();
     getProfileManager().add(trace);
   }
 

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrStackTrace.java

@@ -12,6 +12,8 @@
 @ApiStatus.Internal
 public final class AnrStackTrace implements Comparable<AnrStackTrace> {
 
+  private static final int MAX_STACK_LENGTH = 1000;
+
   public final StackTraceElement[] stack;
   public final long timestampMs;
 
@@ -47,6 +49,9 @@ public static AnrStackTrace deserialize(final @NotNull DataInputStream dis) thro
       if (version == 1) {
         final long timestampMs = dis.readLong();
         final int stackLength = dis.readInt();
+        if (stackLength < 0 || stackLength > MAX_STACK_LENGTH) {
+          return null;
+        }
         final @NotNull StackTraceElement[] stack = new StackTraceElement[stackLength];
 
         for (int i = 0; i < stackLength; i++) {

sentry-android-core/src/test/java/io/sentry/android/core/anr/AnrProfilingIntegrationTest.kt

@@ -186,7 +186,10 @@ class AnrProfilingIntegrationTest {
     assertEquals(AnrProfilingIntegration.MainThreadState.ANR_DETECTED, integration.state)
     assertEquals(2, integration.profileManager.load().stacks.size)
 
-    integration.close()
+    for (i in 0 until AnrProfilingIntegration.MAX_NUM_STACKS + 1) {
+      integration.checkMainThread(mainThread)
+    }
+    assertEquals(AnrProfilingIntegration.MAX_NUM_STACKS, integration.numCollectedStacks.get())
   }
 
   @Test

sentry-android-core/src/test/java/io/sentry/android/core/anr/AnrStackTraceTest.kt

@@ -94,4 +94,34 @@ class AnrStackTraceTest {
     assertNull(deserialized.stack[0].fileName)
     assertEquals(42, deserialized.stack[1].lineNumber)
   }
+
+  @Test
+  fun `deserialize returns null for oversized stack length`() {
+    val bytes = ByteArrayOutputStream()
+    val dos = DataOutputStream(bytes)
+    dos.writeShort(1) // version
+    dos.writeLong(1234567890L) // timestamp
+    dos.writeInt(1001) // stackLength exceeds MAX_STACK_LENGTH
+    dos.flush()
+
+    val dis = DataInputStream(ByteArrayInputStream(bytes.toByteArray()))
+    val deserialized = AnrStackTrace.deserialize(dis)
+
+    assertNull(deserialized)
+  }
+
+  @Test
+  fun `deserialize returns null for negative stack length`() {
+    val bytes = ByteArrayOutputStream()
+    val dos = DataOutputStream(bytes)
+    dos.writeShort(1) // version
+    dos.writeLong(1234567890L) // timestamp
+    dos.writeInt(-1) // negative stackLength
+    dos.flush()
+
+    val dis = DataInputStream(ByteArrayInputStream(bytes.toByteArray()))
+    val deserialized = AnrStackTrace.deserialize(dis)
+
+    assertNull(deserialized)
+  }
 }