From 2ab34ebca967b5bdac6e49a3b12f4b7e982b72be Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Mon, 14 Oct 2024 09:00:54 +0200 Subject: [PATCH 1/6] [QA] Replace SecureRandom with vendored Random (#3783) * Replace SecureRandom with vendored Random * Changelog * Fix failing test --- CHANGELOG.md | 1 + .../android/core/AnrV2EventProcessor.java | 8 +- .../android/replay/ReplayIntegration.kt | 4 +- .../replay/capture/BufferCaptureStrategy.kt | 4 +- .../io/sentry/android/replay/util/Sampling.kt | 4 +- .../capture/BufferCaptureStrategyTest.kt | 4 +- sentry/api/sentry.api | 13 + .../src/main/java/io/sentry/SentryClient.java | 6 +- .../main/java/io/sentry/TracesSampler.java | 8 +- .../sentry/cache/PersistingScopeObserver.java | 6 + .../java/io/sentry/metrics/MetricsHelper.java | 4 +- .../src/main/java/io/sentry/util/Random.java | 466 ++++++++++++++++++ .../test/java/io/sentry/TracesSamplerTest.kt | 4 +- 13 files changed, 509 insertions(+), 23 deletions(-) create mode 100644 sentry/src/main/java/io/sentry/util/Random.java diff --git a/CHANGELOG.md b/CHANGELOG.md index f6db963eb3..84040594a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - Deprecate `enableTracing` option ([#3777](https://github.com/getsentry/sentry-java/pull/3777)) +- Vendor `java.util.Random` and replace `java.security.SecureRandom` usages ([#3783](https://github.com/getsentry/sentry-java/pull/3783)) ## 7.15.0 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java index d58d04b7f8..0399b634fb 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java @@ -54,8 +54,8 @@ import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.User; import io.sentry.util.HintUtils; +import io.sentry.util.Random; import java.io.File; -import java.security.SecureRandom; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -83,7 +83,7 @@ public final class AnrV2EventProcessor implements BackfillingEventProcessor { private final @NotNull SentryExceptionFactory sentryExceptionFactory; - private final @Nullable SecureRandom random; + private final @Nullable Random random; public AnrV2EventProcessor( final @NotNull Context context, @@ -96,7 +96,7 @@ public AnrV2EventProcessor( final @NotNull Context context, final @NotNull SentryAndroidOptions options, final @NotNull BuildInfoProvider buildInfoProvider, - final @Nullable SecureRandom random) { + final @Nullable Random random) { this.context = ContextUtils.getApplicationContext(context); this.options = options; this.buildInfoProvider = buildInfoProvider; @@ -180,7 +180,7 @@ private boolean sampleReplay(final @NotNull SentryEvent event) { try { // we have to sample here with the old sample rate, because it may change between app launches - final @NotNull SecureRandom random = this.random != null ? this.random : new SecureRandom(); + final @NotNull Random random = this.random != null ? this.random : new Random(); final double replayErrorSampleRateDouble = Double.parseDouble(replayErrorSampleRate); if (replayErrorSampleRateDouble < random.nextDouble()) { options diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt index 82d20cf3c1..ee9223cc80 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt @@ -35,9 +35,9 @@ import io.sentry.transport.ICurrentDateProvider import io.sentry.util.FileUtils import io.sentry.util.HintUtils import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion +import io.sentry.util.Random import java.io.Closeable import java.io.File -import java.security.SecureRandom import java.util.LinkedList import java.util.concurrent.atomic.AtomicBoolean import kotlin.LazyThreadSafetyMode.NONE @@ -78,7 +78,7 @@ public class ReplayIntegration( private var hub: IHub? = null private var recorder: Recorder? = null private var gestureRecorder: GestureRecorder? = null - private val random by lazy { SecureRandom() } + private val random by lazy { Random() } private val rootViewsSpy by lazy(NONE) { RootViewsSpy.install() } // TODO: probably not everything has to be thread-safe here diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index 247888f47d..2dbb2a1746 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -18,8 +18,8 @@ import io.sentry.android.replay.util.submitSafely import io.sentry.protocol.SentryId import io.sentry.transport.ICurrentDateProvider import io.sentry.util.FileUtils +import io.sentry.util.Random import java.io.File -import java.security.SecureRandom import java.util.Date import java.util.concurrent.ScheduledExecutorService @@ -27,7 +27,7 @@ internal class BufferCaptureStrategy( private val options: SentryOptions, private val hub: IHub?, private val dateProvider: ICurrentDateProvider, - private val random: SecureRandom, + private val random: Random, executor: ScheduledExecutorService? = null, replayCacheProvider: ((replayId: SentryId, recorderConfig: ScreenshotRecorderConfig) -> ReplayCache)? = null ) : BaseCaptureStrategy(options, hub, dateProvider, executor = executor, replayCacheProvider = replayCacheProvider) { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Sampling.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Sampling.kt index 8acb6b00a6..5ec46ea962 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Sampling.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Sampling.kt @@ -1,8 +1,8 @@ package io.sentry.android.replay.util -import java.security.SecureRandom +import io.sentry.util.Random -internal fun SecureRandom.sample(rate: Double?): Boolean { +internal fun Random.sample(rate: Double?): Boolean { if (rate != null) { return !(rate < this.nextDouble()) // bad luck } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt index 337ba525fc..625306cb8e 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/capture/BufferCaptureStrategyTest.kt @@ -20,6 +20,7 @@ import io.sentry.android.replay.capture.BufferCaptureStrategyTest.Fixture.Compan import io.sentry.protocol.SentryId import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider +import io.sentry.util.Random import org.awaitility.kotlin.await import org.junit.Rule import org.junit.rules.TemporaryFolder @@ -35,7 +36,6 @@ import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import java.io.File -import java.security.SecureRandom import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -97,7 +97,7 @@ class BufferCaptureStrategyTest { options, hub, dateProvider, - SecureRandom(), + Random(), mock { doAnswer { invocation -> (invocation.arguments[0] as Runnable).run() diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index ac1eb2bc8a..89684c7ae5 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -5808,6 +5808,19 @@ public final class io/sentry/util/PropagationTargetsUtils { public static fun contain (Ljava/util/List;Ljava/net/URI;)Z } +public final class io/sentry/util/Random : java/io/Serializable { + public fun ()V + public fun (J)V + public fun nextBoolean ()Z + public fun nextBytes ([B)V + public fun nextDouble ()D + public fun nextFloat ()F + public fun nextInt ()I + public fun nextInt (I)I + public fun nextLong ()J + public fun setSeed (J)V +} + public final class io/sentry/util/SampleRateUtils { public fun ()V public static fun isValidProfilesSampleRate (Ljava/lang/Double;)Z diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 6868894340..27529d100b 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -17,10 +17,10 @@ import io.sentry.util.CheckInUtils; import io.sentry.util.HintUtils; import io.sentry.util.Objects; +import io.sentry.util.Random; import io.sentry.util.TracingUtils; import java.io.Closeable; import java.io.IOException; -import java.security.SecureRandom; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -40,7 +40,7 @@ public final class SentryClient implements ISentryClient, IMetricsClient { private final @NotNull SentryOptions options; private final @NotNull ITransport transport; - private final @Nullable SecureRandom random; + private final @Nullable Random random; private final @NotNull SortBreadcrumbsByDate sortBreadcrumbsByDate = new SortBreadcrumbsByDate(); private final @NotNull IMetricsAggregator metricsAggregator; @@ -67,7 +67,7 @@ public boolean isEnabled() { ? new MetricsAggregator(options, this) : NoopMetricsAggregator.getInstance(); - this.random = options.getSampleRate() == null ? null : new SecureRandom(); + this.random = options.getSampleRate() == null ? null : new Random(); } private boolean shouldApplyScopeData( diff --git a/sentry/src/main/java/io/sentry/TracesSampler.java b/sentry/src/main/java/io/sentry/TracesSampler.java index e0ce111037..5e5b808333 100644 --- a/sentry/src/main/java/io/sentry/TracesSampler.java +++ b/sentry/src/main/java/io/sentry/TracesSampler.java @@ -1,7 +1,7 @@ package io.sentry; import io.sentry.util.Objects; -import java.security.SecureRandom; +import io.sentry.util.Random; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; @@ -10,14 +10,14 @@ final class TracesSampler { private static final @NotNull Double DEFAULT_TRACES_SAMPLE_RATE = 1.0; private final @NotNull SentryOptions options; - private final @NotNull SecureRandom random; + private final @NotNull Random random; public TracesSampler(final @NotNull SentryOptions options) { - this(Objects.requireNonNull(options, "options are required"), new SecureRandom()); + this(Objects.requireNonNull(options, "options are required"), new Random()); } @TestOnly - TracesSampler(final @NotNull SentryOptions options, final @NotNull SecureRandom random) { + TracesSampler(final @NotNull SentryOptions options, final @NotNull Random random) { this.options = options; this.random = random; } diff --git a/sentry/src/main/java/io/sentry/cache/PersistingScopeObserver.java b/sentry/src/main/java/io/sentry/cache/PersistingScopeObserver.java index 7c186cf99d..908e2c66e4 100644 --- a/sentry/src/main/java/io/sentry/cache/PersistingScopeObserver.java +++ b/sentry/src/main/java/io/sentry/cache/PersistingScopeObserver.java @@ -133,6 +133,12 @@ public void setReplayId(@NotNull SentryId replayId) { @SuppressWarnings("FutureReturnValueIgnored") private void serializeToDisk(final @NotNull Runnable task) { + if (Thread.currentThread().getName().contains("SentryExecutor")) { + // we're already on the sentry executor thread, so we can just execute it directly + task.run(); + return; + } + try { options .getExecutorService() diff --git a/sentry/src/main/java/io/sentry/metrics/MetricsHelper.java b/sentry/src/main/java/io/sentry/metrics/MetricsHelper.java index c4353c53a3..37fd4523a4 100644 --- a/sentry/src/main/java/io/sentry/metrics/MetricsHelper.java +++ b/sentry/src/main/java/io/sentry/metrics/MetricsHelper.java @@ -1,7 +1,7 @@ package io.sentry.metrics; import io.sentry.MeasurementUnit; -import java.security.SecureRandom; +import io.sentry.util.Random; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -27,7 +27,7 @@ public final class MetricsHelper { private static final char TAGS_ESCAPE_CHAR = '\\'; private static long FLUSH_SHIFT_MS = - (long) (new SecureRandom().nextFloat() * (ROLLUP_IN_SECONDS * 1000f)); + (long) (new Random().nextFloat() * (ROLLUP_IN_SECONDS * 1000f)); public static long getTimeBucketKey(final long timestampMs) { final long seconds = timestampMs / 1000; diff --git a/sentry/src/main/java/io/sentry/util/Random.java b/sentry/src/main/java/io/sentry/util/Random.java new file mode 100644 index 0000000000..cbd81824df --- /dev/null +++ b/sentry/src/main/java/io/sentry/util/Random.java @@ -0,0 +1,466 @@ +/* + * Copyright (c) 1995, 2013, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package io.sentry.util; + +import java.util.concurrent.atomic.AtomicLong; +import org.jetbrains.annotations.ApiStatus; + +/** + * A simplified version of {@link java.util.Random} that we use for sampling, which is much faster + * than {@link java.security.SecureRandom}. This is necessary so that some security tools do not + * flag our Random usage as potentially insecure. + */ +@ApiStatus.Internal +public final class Random implements java.io.Serializable { + /** use serialVersionUID from JDK 1.1 for interoperability */ + private static final long serialVersionUID = 3905348978240129619L; + + /** + * The internal state associated with this pseudorandom number generator. (The specs for the + * methods in this class describe the ongoing computation of this value.) + */ + private final AtomicLong seed; + + private static final long multiplier = 0x5DEECE66DL; + private static final long addend = 0xBL; + private static final long mask = (1L << 48) - 1; + + private static final double DOUBLE_UNIT = 0x1.0p-53; // 1.0 / (1L << 53) + + // IllegalArgumentException messages + static final String BadBound = "bound must be positive"; + + /** + * Creates a new random number generator. This constructor sets the seed of the random number + * generator to a value very likely to be distinct from any other invocation of this constructor. + */ + public Random() { + this(seedUniquifier() ^ System.nanoTime()); + } + + private static long seedUniquifier() { + // L'Ecuyer, "Tables of Linear Congruential Generators of + // Different Sizes and Good Lattice Structure", 1999 + for (; ; ) { + long current = seedUniquifier.get(); + long next = current * 1181783497276652981L; + if (seedUniquifier.compareAndSet(current, next)) return next; + } + } + + private static final AtomicLong seedUniquifier = new AtomicLong(8682522807148012L); + + /** + * Creates a new random number generator using a single {@code long} seed. The seed is the initial + * value of the internal state of the pseudorandom number generator which is maintained by method + * {@link #next}. + * + *

The invocation {@code new Random(seed)} is equivalent to: + * + * 

{@code
+   * Random rnd = new Random();
+   * rnd.setSeed(seed);
+   * }
+ * + * @param seed the initial seed + * @see #setSeed(long) + */ + public Random(long seed) { + if (getClass() == Random.class) this.seed = new AtomicLong(initialScramble(seed)); + else { + // subclass might have overriden setSeed + this.seed = new AtomicLong(); + setSeed(seed); + } + } + + private static long initialScramble(long seed) { + return (seed ^ multiplier) & mask; + } + + /** + * Sets the seed of this random number generator using a single {@code long} seed. The general + * contract of {@code setSeed} is that it alters the state of this random number generator object + * so as to be in exactly the same state as if it had just been created with the argument {@code + * seed} as a seed. The method {@code setSeed} is implemented by class {@code Random} by + * atomically updating the seed to + * + * 
{@code (seed ^ 0x5DEECE66DL) & ((1L << 48) - 1)}
+ * + *

The implementation of {@code setSeed} by class {@code Random} happens to use only 48 bits of + * the given seed. In general, however, an overriding method may use all 64 bits of the {@code + * long} argument as a seed value. + * + * @param seed the initial seed + */ + public synchronized void setSeed(long seed) { + this.seed.set(initialScramble(seed)); + } + + /** + * Generates the next pseudorandom number. Subclasses should override this, as this is used by all + * other methods. + * + *

The general contract of {@code next} is that it returns an {@code int} value and if the + * argument {@code bits} is between {@code 1} and {@code 32} (inclusive), then that many low-order + * bits of the returned value will be (approximately) independently chosen bit values, each of + * which is (approximately) equally likely to be {@code 0} or {@code 1}. The method {@code next} + * is implemented by class {@code Random} by atomically updating the seed to + * + * 

{@code (seed * 0x5DEECE66DL + 0xBL) & ((1L << 48) - 1)}
+ * + * and returning + * + * 
{@code (int)(seed >>> (48 - bits))}.
+ * + * This is a linear congruential pseudorandom number generator, as defined by D. H. Lehmer and + * described by Donald E. Knuth in The Art of Computer Programming, Volume 2: + * Seminumerical Algorithms, section 3.2.1. + * + * @param bits random bits + * @return the next pseudorandom value from this random number generator's sequence + * @since 1.1 + */ + private int next(int bits) { + long oldseed, nextseed; + AtomicLong seed = this.seed; + do { + oldseed = seed.get(); + nextseed = (oldseed * multiplier + addend) & mask; + } while (!seed.compareAndSet(oldseed, nextseed)); + return (int) (nextseed >>> (48 - bits)); + } + + /** + * Generates random bytes and places them into a user-supplied byte array. The number of random + * bytes produced is equal to the length of the byte array. + * + *

The method {@code nextBytes} is implemented by class {@code Random} as if by: + * + * 

{@code
+   * public void nextBytes(byte[] bytes) {
+   *   for (int i = 0; i < bytes.length; )
+   *     for (int rnd = nextInt(), n = Math.min(bytes.length - i, 4);
+   *          n-- > 0; rnd >>= 8)
+   *       bytes[i++] = (byte)rnd;
+   * }
+   * }
+ * + * @param bytes the byte array to fill with random bytes + * @throws NullPointerException if the byte array is null + * @since 1.1 + */ + public void nextBytes(byte[] bytes) { + for (int i = 0, len = bytes.length; i < len; ) + for (int rnd = nextInt(), n = Math.min(len - i, Integer.SIZE / Byte.SIZE); + n-- > 0; + rnd >>= Byte.SIZE) bytes[i++] = (byte) rnd; + } + + /** + * The form of nextLong used by LongStream Spliterators. If origin is greater than bound, acts as + * unbounded form of nextLong, else as bounded form. + * + * @param origin the least value, unless greater than bound + * @param bound the upper bound (exclusive), must not equal origin + * @return a pseudorandom value + */ + final long internalNextLong(long origin, long bound) { + long r = nextLong(); + if (origin < bound) { + long n = bound - origin, m = n - 1; + if ((n & m) == 0L) // power of two + r = (r & m) + origin; + else if (n > 0L) { // reject over-represented candidates + for (long u = r >>> 1; // ensure nonnegative + u + m - (r = u % n) < 0L; // rejection check + u = nextLong() >>> 1) // retry + ; + r += origin; + } else { // range not representable as long + while (r < origin || r >= bound) r = nextLong(); + } + } + return r; + } + + /** + * The form of nextInt used by IntStream Spliterators. For the unbounded case: uses nextInt(). For + * the bounded case with representable range: uses nextInt(int bound) For the bounded case with + * unrepresentable range: uses nextInt() + * + * @param origin the least value, unless greater than bound + * @param bound the upper bound (exclusive), must not equal origin + * @return a pseudorandom value + */ + final int internalNextInt(int origin, int bound) { + if (origin < bound) { + int n = bound - origin; + if (n > 0) { + return nextInt(n) + origin; + } else { // range not representable as int + int r; + do { + r = nextInt(); + } while (r < origin || r >= bound); + return r; + } + } else { + return nextInt(); + } + } + + /** + * The form of nextDouble used by DoubleStream Spliterators. + * + * @param origin the least value, unless greater than bound + * @param bound the upper bound (exclusive), must not equal origin + * @return a pseudorandom value + */ + final double internalNextDouble(double origin, double bound) { + double r = nextDouble(); + if (origin < bound) { + r = r * (bound - origin) + origin; + if (r >= bound) // correct for rounding + r = Double.longBitsToDouble(Double.doubleToLongBits(bound) - 1); + } + return r; + } + + /** + * Returns the next pseudorandom, uniformly distributed {@code int} value from this random number + * generator's sequence. The general contract of {@code nextInt} is that one {@code int} value is + * pseudorandomly generated and returned. All 232 possible {@code int} values are + * produced with (approximately) equal probability. + * + *

The method {@code nextInt} is implemented by class {@code Random} as if by: + * + * 

{@code
+   * public int nextInt() {
+   *   return next(32);
+   * }
+   * }
+ * + * @return the next pseudorandom, uniformly distributed {@code int} value from this random number + * generator's sequence + */ + public int nextInt() { + return next(32); + } + + /** + * Returns a pseudorandom, uniformly distributed {@code int} value between 0 (inclusive) and the + * specified value (exclusive), drawn from this random number generator's sequence. The general + * contract of {@code nextInt} is that one {@code int} value in the specified range is + * pseudorandomly generated and returned. All {@code bound} possible {@code int} values are + * produced with (approximately) equal probability. The method {@code nextInt(int bound)} is + * implemented by class {@code Random} as if by: + * + * 
{@code
+   * public int nextInt(int bound) {
+   *   if (bound <= 0)
+   *     throw new IllegalArgumentException("bound must be positive");
+   *
+   *   if ((bound & -bound) == bound)  // i.e., bound is a power of 2
+   *     return (int)((bound * (long)next(31)) >> 31);
+   *
+   *   int bits, val;
+   *   do {
+   *       bits = next(31);
+   *       val = bits % bound;
+   *   } while (bits - val + (bound-1) < 0);
+   *   return val;
+   * }
+   * }
+ * + *

The hedge "approximately" is used in the foregoing description only because the next method + * is only approximately an unbiased source of independently chosen bits. If it were a perfect + * source of randomly chosen bits, then the algorithm shown would choose {@code int} values from + * the stated range with perfect uniformity. + * + *

The algorithm is slightly tricky. It rejects values that would result in an uneven + * distribution (due to the fact that 2^31 is not divisible by n). The probability of a value + * being rejected depends on n. The worst case is n=2^30+1, for which the probability of a reject + * is 1/2, and the expected number of iterations before the loop terminates is 2. + * + *

The algorithm treats the case where n is a power of two specially: it returns the correct + * number of high-order bits from the underlying pseudo-random number generator. In the absence of + * special treatment, the correct number of low-order bits would be returned. Linear + * congruential pseudo-random number generators such as the one implemented by this class are + * known to have short periods in the sequence of values of their low-order bits. Thus, this + * special case greatly increases the length of the sequence of values returned by successive + * calls to this method if n is a small power of two. + * + * @param bound the upper bound (exclusive). Must be positive. + * @return the next pseudorandom, uniformly distributed {@code int} value between zero (inclusive) + * and {@code bound} (exclusive) from this random number generator's sequence + * @throws IllegalArgumentException if bound is not positive + * @since 1.2 + */ + public int nextInt(int bound) { + if (bound <= 0) throw new IllegalArgumentException(BadBound); + + int r = next(31); + int m = bound - 1; + if ((bound & m) == 0) // i.e., bound is a power of 2 + r = (int) ((bound * (long) r) >> 31); + else { + for (int u = r; u - (r = u % bound) + m < 0; u = next(31)) + ; + } + return r; + } + + /** + * Returns the next pseudorandom, uniformly distributed {@code long} value from this random number + * generator's sequence. The general contract of {@code nextLong} is that one {@code long} value + * is pseudorandomly generated and returned. + * + *

The method {@code nextLong} is implemented by class {@code Random} as if by: + * + * 

{@code
+   * public long nextLong() {
+   *   return ((long)next(32) << 32) + next(32);
+   * }
+   * }
+ * + * Because class {@code Random} uses a seed with only 48 bits, this algorithm will not return all + * possible {@code long} values. + * + * @return the next pseudorandom, uniformly distributed {@code long} value from this random number + * generator's sequence + */ + @SuppressWarnings("UnnecessaryParentheses") + public long nextLong() { + // it's okay that the bottom word remains signed. + return ((long) (next(32)) << 32) + next(32); + } + + /** + * Returns the next pseudorandom, uniformly distributed {@code boolean} value from this random + * number generator's sequence. The general contract of {@code nextBoolean} is that one {@code + * boolean} value is pseudorandomly generated and returned. The values {@code true} and {@code + * false} are produced with (approximately) equal probability. + * + *

The method {@code nextBoolean} is implemented by class {@code Random} as if by: + * + * 

{@code
+   * public boolean nextBoolean() {
+   *   return next(1) != 0;
+   * }
+   * }
+ * + * @return the next pseudorandom, uniformly distributed {@code boolean} value from this random + * number generator's sequence + * @since 1.2 + */ + public boolean nextBoolean() { + return next(1) != 0; + } + + /** + * Returns the next pseudorandom, uniformly distributed {@code float} value between {@code 0.0} + * and {@code 1.0} from this random number generator's sequence. + * + *

The general contract of {@code nextFloat} is that one {@code float} value, chosen + * (approximately) uniformly from the range {@code 0.0f} (inclusive) to {@code 1.0f} (exclusive), + * is pseudorandomly generated and returned. All 224 possible {@code float} values of + * the form m x 2-24, where m is a positive integer less than + * 224, are produced with (approximately) equal probability. + * + *

The method {@code nextFloat} is implemented by class {@code Random} as if by: + * + * 

{@code
+   * public float nextFloat() {
+   *   return next(24) / ((float)(1 << 24));
+   * }
+   * }
+ * + *

The hedge "approximately" is used in the foregoing description only because the next method + * is only approximately an unbiased source of independently chosen bits. If it were a perfect + * source of randomly chosen bits, then the algorithm shown would choose {@code float} values from + * the stated range with perfect uniformity. + * + *

[In early versions of Java, the result was incorrectly calculated as: + * + * 

{@code
+   * return next(30) / ((float)(1 << 30));
+   * }
+ * + * This might seem to be equivalent, if not better, but in fact it introduced a slight + * nonuniformity because of the bias in the rounding of floating-point numbers: it was slightly + * more likely that the low-order bit of the significand would be 0 than that it would be 1.] + * + * @return the next pseudorandom, uniformly distributed {@code float} value between {@code 0.0} + * and {@code 1.0} from this random number generator's sequence + */ + public float nextFloat() { + return next(24) / ((float) (1 << 24)); + } + + /** + * Returns the next pseudorandom, uniformly distributed {@code double} value between {@code 0.0} + * and {@code 1.0} from this random number generator's sequence. + * + *

The general contract of {@code nextDouble} is that one {@code double} value, chosen + * (approximately) uniformly from the range {@code 0.0d} (inclusive) to {@code 1.0d} (exclusive), + * is pseudorandomly generated and returned. + * + *

The method {@code nextDouble} is implemented by class {@code Random} as if by: + * + * 

{@code
+   * public double nextDouble() {
+   *   return (((long)next(26) << 27) + next(27))
+   *     / (double)(1L << 53);
+   * }
+   * }
+ * + *

The hedge "approximately" is used in the foregoing description only because the {@code next} + * method is only approximately an unbiased source of independently chosen bits. If it were a + * perfect source of randomly chosen bits, then the algorithm shown would choose {@code double} + * values from the stated range with perfect uniformity. + * + *

[In early versions of Java, the result was incorrectly calculated as: + * + * 

{@code
+   * return (((long)next(27) << 27) + next(27))
+   *   / (double)(1L << 54);
+   * }
+ * + * This might seem to be equivalent, if not better, but in fact it introduced a large + * nonuniformity because of the bias in the rounding of floating-point numbers: it was three times + * as likely that the low-order bit of the significand would be 0 than that it would be 1! This + * nonuniformity probably doesn't matter much in practice, but we strive for perfection.] + * + * @return the next pseudorandom, uniformly distributed {@code double} value between {@code 0.0} + * and {@code 1.0} from this random number generator's sequence + * @see Math#random + */ + @SuppressWarnings("UnnecessaryParentheses") + public double nextDouble() { + return (((long) (next(26)) << 27) + next(27)) * DOUBLE_UNIT; + } +} diff --git a/sentry/src/test/java/io/sentry/TracesSamplerTest.kt b/sentry/src/test/java/io/sentry/TracesSamplerTest.kt index 11d58766d6..52e294c54d 100644 --- a/sentry/src/test/java/io/sentry/TracesSamplerTest.kt +++ b/sentry/src/test/java/io/sentry/TracesSamplerTest.kt @@ -1,11 +1,11 @@ package io.sentry +import io.sentry.util.Random import org.mockito.kotlin.any import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.whenever -import java.security.SecureRandom import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -23,7 +23,7 @@ class TracesSamplerTest { profilesSamplerCallback: SentryOptions.ProfilesSamplerCallback? = null, logger: ILogger? = null ): TracesSampler { - val random = mock() + val random = mock() if (randomResult != null) { whenever(random.nextDouble()).thenReturn(randomResult) } From a9c767ee938ced7114056047fc5e2ebe0e9fe6e1 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 14 Oct 2024 13:32:51 +0200 Subject: [PATCH 2/6] [QA] Move NDK scope sync to background thread (#3754) * Move NDK scope sync to background thread * Update Changelog * Cleanup code --- CHANGELOG.md | 1 + sentry-android-ndk/build.gradle.kts | 2 +- .../sentry/android/ndk/NdkScopeObserver.java | 70 ++++++++++++------- .../android/ndk/NdkScopeObserverTest.kt | 38 ++++++++++ 4 files changed, 84 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84040594a6..a18600c608 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Deprecate `enableTracing` option ([#3777](https://github.com/getsentry/sentry-java/pull/3777)) - Vendor `java.util.Random` and replace `java.security.SecureRandom` usages ([#3783](https://github.com/getsentry/sentry-java/pull/3783)) +- Fix potential ANRs due to NDK scope sync ([#3754](https://github.com/getsentry/sentry-java/pull/3754)) ## 7.15.0 diff --git a/sentry-android-ndk/build.gradle.kts b/sentry-android-ndk/build.gradle.kts index f6564cd97f..f1c4873053 100644 --- a/sentry-android-ndk/build.gradle.kts +++ b/sentry-android-ndk/build.gradle.kts @@ -105,6 +105,6 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib, KotlinCompilerVersion.VERSION)) testImplementation(Config.TestLibs.kotlinTestJunit) - testImplementation(Config.TestLibs.mockitoKotlin) + testImplementation(projects.sentryTestSupport) } diff --git a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java index 009bba9b81..2350c419be 100644 --- a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java +++ b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java @@ -31,12 +31,18 @@ public NdkScopeObserver(final @NotNull SentryOptions options) { @Override public void setUser(final @Nullable User user) { try { - if (user == null) { - // remove user if its null - nativeScope.removeUser(); - } else { - nativeScope.setUser(user.getId(), user.getEmail(), user.getIpAddress(), user.getUsername()); - } + options + .getExecutorService() + .submit( + () -> { + if (user == null) { + // remove user if its null + nativeScope.removeUser(); + } else { + nativeScope.setUser( + user.getId(), user.getEmail(), user.getIpAddress(), user.getUsername()); + } + }); } catch (Throwable e) { options.getLogger().log(SentryLevel.ERROR, e, "Scope sync setUser has an error."); } @@ -45,24 +51,36 @@ public void setUser(final @Nullable User user) { @Override public void addBreadcrumb(final @NotNull Breadcrumb crumb) { try { - String level = null; - if (crumb.getLevel() != null) { - level = crumb.getLevel().name().toLowerCase(Locale.ROOT); - } - final String timestamp = DateUtils.getTimestamp(crumb.getTimestamp()); + options + .getExecutorService() + .submit( + () -> { + String level = null; + if (crumb.getLevel() != null) { + level = crumb.getLevel().name().toLowerCase(Locale.ROOT); + } + final String timestamp = DateUtils.getTimestamp(crumb.getTimestamp()); - String data = null; - try { - final Map dataRef = crumb.getData(); - if (!dataRef.isEmpty()) { - data = options.getSerializer().serialize(dataRef); - } - } catch (Throwable e) { - options.getLogger().log(SentryLevel.ERROR, e, "Breadcrumb data is not serializable."); - } + String data = null; + try { + final Map dataRef = crumb.getData(); + if (!dataRef.isEmpty()) { + data = options.getSerializer().serialize(dataRef); + } + } catch (Throwable e) { + options + .getLogger() + .log(SentryLevel.ERROR, e, "Breadcrumb data is not serializable."); + } - nativeScope.addBreadcrumb( - level, crumb.getMessage(), crumb.getCategory(), crumb.getType(), timestamp, data); + nativeScope.addBreadcrumb( + level, + crumb.getMessage(), + crumb.getCategory(), + crumb.getType(), + timestamp, + data); + }); } catch (Throwable e) { options.getLogger().log(SentryLevel.ERROR, e, "Scope sync addBreadcrumb has an error."); } @@ -71,7 +89,7 @@ public void addBreadcrumb(final @NotNull Breadcrumb crumb) { @Override public void setTag(final @NotNull String key, final @NotNull String value) { try { - nativeScope.setTag(key, value); + options.getExecutorService().submit(() -> nativeScope.setTag(key, value)); } catch (Throwable e) { options.getLogger().log(SentryLevel.ERROR, e, "Scope sync setTag(%s) has an error.", key); } @@ -80,7 +98,7 @@ public void setTag(final @NotNull String key, final @NotNull String value) { @Override public void removeTag(final @NotNull String key) { try { - nativeScope.removeTag(key); + options.getExecutorService().submit(() -> nativeScope.removeTag(key)); } catch (Throwable e) { options.getLogger().log(SentryLevel.ERROR, e, "Scope sync removeTag(%s) has an error.", key); } @@ -89,7 +107,7 @@ public void removeTag(final @NotNull String key) { @Override public void setExtra(final @NotNull String key, final @NotNull String value) { try { - nativeScope.setExtra(key, value); + options.getExecutorService().submit(() -> nativeScope.setExtra(key, value)); } catch (Throwable e) { options.getLogger().log(SentryLevel.ERROR, e, "Scope sync setExtra(%s) has an error.", key); } @@ -98,7 +116,7 @@ public void setExtra(final @NotNull String key, final @NotNull String value) { @Override public void removeExtra(final @NotNull String key) { try { - nativeScope.removeExtra(key); + options.getExecutorService().submit(() -> nativeScope.removeExtra(key)); } catch (Throwable e) { options .getLogger() diff --git a/sentry-android-ndk/src/test/java/io/sentry/android/ndk/NdkScopeObserverTest.kt b/sentry-android-ndk/src/test/java/io/sentry/android/ndk/NdkScopeObserverTest.kt index 335a7679e1..110f794cf9 100644 --- a/sentry-android-ndk/src/test/java/io/sentry/android/ndk/NdkScopeObserverTest.kt +++ b/sentry-android-ndk/src/test/java/io/sentry/android/ndk/NdkScopeObserverTest.kt @@ -6,8 +6,13 @@ import io.sentry.JsonSerializer import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.protocol.User +import io.sentry.test.DeferredExecutorService +import io.sentry.test.ImmediateExecutorService +import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify import kotlin.test.Test @@ -17,6 +22,7 @@ class NdkScopeObserverTest { val nativeScope = mock() val options = SentryOptions().apply { setSerializer(JsonSerializer(mock())) + executorService = ImmediateExecutorService() } fun getSut(): NdkScopeObserver { @@ -111,4 +117,36 @@ class NdkScopeObserverTest { eq(data) ) } + + @Test + fun `scope sync utilizes executor service`() { + val executorService = DeferredExecutorService() + fixture.options.executorService = executorService + val sut = fixture.getSut() + + sut.setTag("a", "b") + sut.removeTag("a") + sut.setExtra("a", "b") + sut.removeExtra("a") + sut.setUser(User()) + sut.addBreadcrumb(Breadcrumb()) + + // as long as the executor service is not run, the scope sync is not called + verify(fixture.nativeScope, never()).setTag(any(), any()) + verify(fixture.nativeScope, never()).removeTag(any()) + verify(fixture.nativeScope, never()).setExtra(any(), any()) + verify(fixture.nativeScope, never()).removeExtra(any()) + verify(fixture.nativeScope, never()).setUser(any(), any(), any(), any()) + verify(fixture.nativeScope, never()).addBreadcrumb(any(), any(), any(), any(), any(), any()) + + // when the executor service is run, the scope sync is called + executorService.runAll() + + verify(fixture.nativeScope).setTag(any(), any()) + verify(fixture.nativeScope).removeTag(any()) + verify(fixture.nativeScope).setExtra(any(), any()) + verify(fixture.nativeScope).removeExtra(any()) + verify(fixture.nativeScope).setUser(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) + verify(fixture.nativeScope).addBreadcrumb(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) + } } From 6259a9fdb9ec4b75001c93401e1d0b1c18541dfb Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 14 Oct 2024 15:53:13 +0200 Subject: [PATCH 3/6] [QA] Offload System.loadLibrary call to background thread (#3670) * Offload System.loadLibrary call to background thread * Update Changelog * Update CHANGELOG.md --- CHANGELOG.md | 1 + .../java/io/sentry/android/ndk/SentryNdk.java | 54 ++++++++++++++----- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a18600c608..cf0d89cca0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Deprecate `enableTracing` option ([#3777](https://github.com/getsentry/sentry-java/pull/3777)) - Vendor `java.util.Random` and replace `java.security.SecureRandom` usages ([#3783](https://github.com/getsentry/sentry-java/pull/3783)) - Fix potential ANRs due to NDK scope sync ([#3754](https://github.com/getsentry/sentry-java/pull/3754)) +- Fix potential ANRs due to NDK System.loadLibrary calls ([#3670](https://github.com/getsentry/sentry-java/pull/3670)) ## 7.15.0 diff --git a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/SentryNdk.java b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/SentryNdk.java index 1ddc04c524..7245516b49 100644 --- a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/SentryNdk.java +++ b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/SentryNdk.java @@ -1,23 +1,40 @@ package io.sentry.android.ndk; import io.sentry.android.core.SentryAndroidOptions; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @ApiStatus.Internal public final class SentryNdk { + private static final @NotNull CountDownLatch loadLibraryLatch = new CountDownLatch(1); + private SentryNdk() {} static { - // On older Android versions, it was necessary to manually call "`System.loadLibrary` on all - // transitive dependencies before loading [the] main library." - // The dependencies of `libsentry.so` are currently `lib{c,m,dl,log}.so`. - // See - // https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#changes-to-library-dependency-resolution - System.loadLibrary("log"); - System.loadLibrary("sentry"); - System.loadLibrary("sentry-android"); + new Thread( + () -> { + // On older Android versions, it was necessary to manually call "`System.loadLibrary` + // on all + // transitive dependencies before loading [the] main library." + // The dependencies of `libsentry.so` are currently `lib{c,m,dl,log}.so`. + // See + // https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#changes-to-library-dependency-resolution + try { + System.loadLibrary("log"); + System.loadLibrary("sentry"); + System.loadLibrary("sentry-android"); + } catch (Throwable t) { + // ignored + // if loadLibrary() fails, the later init() will throw an exception anyway + } finally { + loadLibraryLatch.countDown(); + } + }, + "SentryNdkLoadLibs") + .start(); } private static native void initSentryNative(@NotNull final SentryAndroidOptions options); @@ -31,14 +48,23 @@ private SentryNdk() {} */ public static void init(@NotNull final SentryAndroidOptions options) { SentryNdkUtil.addPackage(options.getSdkVersion()); - initSentryNative(options); + try { + if (loadLibraryLatch.await(2000, TimeUnit.MILLISECONDS)) { + initSentryNative(options); - // only add scope sync observer if the scope sync is enabled. - if (options.isEnableScopeSync()) { - options.addScopeObserver(new NdkScopeObserver(options)); - } + // only add scope sync observer if the scope sync is enabled. + if (options.isEnableScopeSync()) { + options.addScopeObserver(new NdkScopeObserver(options)); + } - options.setDebugImagesLoader(new DebugImagesLoader(options, new NativeModuleListLoader())); + options.setDebugImagesLoader(new DebugImagesLoader(options, new NativeModuleListLoader())); + } else { + throw new IllegalStateException("Timeout waiting for Sentry NDK library to load"); + } + } catch (InterruptedException e) { + throw new IllegalStateException( + "Thread interrupted while waiting for NDK libs to be loaded", e); + } } /** Closes the NDK integration */ From 9c193193a14a18f407e55d14eb051c0a4e55eec6 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 14 Oct 2024 21:36:37 +0200 Subject: [PATCH 4/6] Add meta option to attach ANR thread dumps (#3791) * Add meta option to attach ANR thread dumps * Update Changelog --- CHANGELOG.md | 4 +++ .../android/core/ManifestMetadataReader.java | 5 +++- .../core/ManifestMetadataReaderTest.kt | 25 +++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf0d89cca0..09d00018de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Features + +- Add meta option to attach ANR thread dumps ([#3791](https://github.com/getsentry/sentry-java/pull/3791)) + ### Fixes - Deprecate `enableTracing` option ([#3777](https://github.com/getsentry/sentry-java/pull/3777)) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java b/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java index e42f68cab0..56cd506fe6 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java @@ -26,8 +26,8 @@ final class ManifestMetadataReader { static final String SAMPLE_RATE = "io.sentry.sample-rate"; static final String ANR_ENABLE = "io.sentry.anr.enable"; static final String ANR_REPORT_DEBUG = "io.sentry.anr.report-debug"; - static final String ANR_TIMEOUT_INTERVAL_MILLIS = "io.sentry.anr.timeout-interval-millis"; + static final String ANR_ATTACH_THREAD_DUMPS = "io.sentry.anr.attach-thread-dumps"; static final String AUTO_INIT = "io.sentry.auto-init"; static final String NDK_ENABLE = "io.sentry.ndk.enable"; @@ -176,6 +176,9 @@ static void applyMetadata( ANR_TIMEOUT_INTERVAL_MILLIS, options.getAnrTimeoutIntervalMillis())); + options.setAttachAnrThreadDump( + readBool(metadata, logger, ANR_ATTACH_THREAD_DUMPS, options.isAttachAnrThreadDump())); + final String dsn = readString(metadata, logger, DSN, options.getDsn()); final boolean enabled = readBool(metadata, logger, ENABLE_SENTRY, options.isEnabled()); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt index e068af7b1c..25b2e0191c 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt @@ -246,6 +246,31 @@ class ManifestMetadataReaderTest { assertEquals(5000.toLong(), fixture.options.anrTimeoutIntervalMillis) } + @Test + fun `applyMetadata reads anr attach thread dump to options`() { + // Arrange + val bundle = bundleOf(ManifestMetadataReader.ANR_ATTACH_THREAD_DUMPS to true) + val context = fixture.getContext(metaData = bundle) + + // Act + ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider) + + // Assert + assertEquals(true, fixture.options.isAttachAnrThreadDump) + } + + @Test + fun `applyMetadata reads anr attach thread dump to options and keeps default`() { + // Arrange + val context = fixture.getContext() + + // Act + ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider) + + // Assert + assertEquals(false, fixture.options.isAttachAnrThreadDump) + } + @Test fun `applyMetadata reads activity breadcrumbs to options`() { // Arrange From 8fdee54dfe034a0d21efbddc1613263d542fdf1c Mon Sep 17 00:00:00 2001 From: Stefano Date: Tue, 15 Oct 2024 12:39:29 +0200 Subject: [PATCH 5/6] fix invalid profiles when the transaction name is empty (#3747) * when the transaction name is empty, the profiles default to "unknown" --- CHANGELOG.md | 1 + .../android/core/AndroidTransactionProfilerTest.kt | 11 +++++++++++ .../src/main/java/io/sentry/ProfilingTraceData.java | 2 +- .../main/java/io/sentry/ProfilingTransactionData.java | 2 +- sentry/src/test/java/io/sentry/JsonSerializerTest.kt | 4 ++-- 5 files changed, 16 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09d00018de..3f3171a03b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixes +- fix invalid profiles when the transaction name is empty ([#3747](https://github.com/getsentry/sentry-java/pull/3747)) - Deprecate `enableTracing` option ([#3777](https://github.com/getsentry/sentry-java/pull/3777)) - Vendor `java.util.Random` and replace `java.security.SecureRandom` usages ([#3783](https://github.com/getsentry/sentry-java/pull/3783)) - Fix potential ANRs due to NDK scope sync ([#3754](https://github.com/getsentry/sentry-java/pull/3754)) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt index 02cda7d23b..df64e20b16 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt @@ -360,6 +360,17 @@ class AndroidTransactionProfilerTest { verify(mockExecutorService, never()).submit(any>()) } + @Test + fun `profiling transaction with empty name fallbacks to unknown`() { + val profiler = fixture.getSut(context) + profiler.start() + profiler.bindTransaction(fixture.transaction1) + val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null, fixture.options) + assertNotNull(profilingTraceData) + assertEquals("unknown", profilingTraceData.transactionName) + assertEquals("unknown", profilingTraceData.transactions.first().name) + } + @Test fun `profiler does not throw if traces cannot be written to disk`() { File(fixture.options.profilingTracesDirPath!!).setWritable(false) diff --git a/sentry/src/main/java/io/sentry/ProfilingTraceData.java b/sentry/src/main/java/io/sentry/ProfilingTraceData.java index 17332b5931..3998735917 100644 --- a/sentry/src/main/java/io/sentry/ProfilingTraceData.java +++ b/sentry/src/main/java/io/sentry/ProfilingTraceData.java @@ -144,7 +144,7 @@ public ProfilingTraceData( // Transaction info this.transactions = transactions; - this.transactionName = transactionName; + this.transactionName = transactionName.isEmpty() ? "unknown" : transactionName; this.durationNs = durationNanos; // App info diff --git a/sentry/src/main/java/io/sentry/ProfilingTransactionData.java b/sentry/src/main/java/io/sentry/ProfilingTransactionData.java index 045b859f05..84d149ef15 100644 --- a/sentry/src/main/java/io/sentry/ProfilingTransactionData.java +++ b/sentry/src/main/java/io/sentry/ProfilingTransactionData.java @@ -29,7 +29,7 @@ public ProfilingTransactionData( @NotNull ITransaction transaction, @NotNull Long startNs, @NotNull Long startCpuMs) { this.id = transaction.getEventId().toString(); this.traceId = transaction.getSpanContext().getTraceId().toString(); - this.name = transaction.getName(); + this.name = transaction.getName().isEmpty() ? "unknown" : transaction.getName(); this.relativeStartNs = startNs; this.relativeStartCpuMs = startCpuMs; } diff --git a/sentry/src/test/java/io/sentry/JsonSerializerTest.kt b/sentry/src/test/java/io/sentry/JsonSerializerTest.kt index ba8ee84d51..37c1870288 100644 --- a/sentry/src/test/java/io/sentry/JsonSerializerTest.kt +++ b/sentry/src/test/java/io/sentry/JsonSerializerTest.kt @@ -569,7 +569,7 @@ class JsonSerializerTest { mapOf( "trace_id" to "00000000000000000000000000000000", "relative_cpu_end_ms" to null, - "name" to "", + "name" to "unknown", "relative_start_ns" to 1, "relative_end_ns" to null, "id" to "00000000000000000000000000000000", @@ -578,7 +578,7 @@ class JsonSerializerTest { mapOf( "trace_id" to "00000000000000000000000000000000", "relative_cpu_end_ms" to null, - "name" to "", + "name" to "unknown", "relative_start_ns" to 2, "relative_end_ns" to null, "id" to "00000000000000000000000000000000", From 274c2956310c25995e412e743b2187b4664d1e29 Mon Sep 17 00:00:00 2001 From: Karl Heinz Struggl Date: Tue, 15 Oct 2024 06:23:20 -0700 Subject: [PATCH 6/6] chore: Add action to warn about potentially risky PR changes (#3726) * adds config+action to warn about risky PR changes * updates wording of warning PR comment * added risky files --- .github/file-filters.yml | 12 +++++ .../workflows/changes-in-high-risk-code.yml | 49 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 .github/file-filters.yml create mode 100644 .github/workflows/changes-in-high-risk-code.yml diff --git a/.github/file-filters.yml b/.github/file-filters.yml new file mode 100644 index 0000000000..2b81e2f0b6 --- /dev/null +++ b/.github/file-filters.yml @@ -0,0 +1,12 @@ +# This is used by the action https://github.com/dorny/paths-filter + +high_risk_code: &high_risk_code + # Transport classes + - "sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java" + - "sentry/src/main/java/io/sentry/transport/HttpConnection.java" + - "sentry/src/main/java/io/sentry/transport/QueuedThreadPoolExecutor.java" + - "sentry/src/main/java/io/sentry/transport/RateLimiter.java" + - "sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java" + + # Class used by hybrid SDKs + - "sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java" diff --git a/.github/workflows/changes-in-high-risk-code.yml b/.github/workflows/changes-in-high-risk-code.yml new file mode 100644 index 0000000000..64decbe48f --- /dev/null +++ b/.github/workflows/changes-in-high-risk-code.yml @@ -0,0 +1,49 @@ +name: Changes In High Risk Code +on: + pull_request: + +# https://docs.github.com/en/actions/using-jobs/using-concurrency#example-using-a-fallback-value +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + files-changed: + name: Detect changed files + runs-on: ubuntu-latest + # Map a step output to a job output + outputs: + high_risk_code: ${{ steps.changes.outputs.high_risk_code }} + high_risk_code_files: ${{ steps.changes.outputs.high_risk_code_files }} + steps: + - uses: actions/checkout@v4 + - name: Get changed files + id: changes + uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 + with: + token: ${{ github.token }} + filters: .github/file-filters.yml + + # Enable listing of files matching each filter. + # Paths to files will be available in `${FILTER_NAME}_files` output variable. + list-files: csv + + validate-high-risk-code: + if: needs.files-changed.outputs.high_risk_code == 'true' + needs: files-changed + runs-on: ubuntu-latest + steps: + - name: Comment on PR to notify of changes in high risk files + uses: actions/github-script@v7 + env: + high_risk_code: ${{ needs.files-changed.outputs.high_risk_code_files }} + with: + script: | + const highRiskFiles = process.env.high_risk_code; + const fileList = highRiskFiles.split(',').map(file => `- [ ] ${file}`).join('\n'); + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: `### 🚨 Detected changes in high risk code 🚨 \n High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:\n ${fileList}` + })