diff --git a/CHANGELOG.md b/CHANGELOG.md index 107b916808a..5a97ba246a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ ### Fixes +- Prevent a `StackOverflowError` when a `beforeSend`, `beforeBreadcrumb`, or `beforeSendLog` callback triggers another capture (directly or through a logging integration such as Timber) ([#5737](https://github.com/getsentry/sentry-java/pull/5737)) + - Captures made from within a user callback (event, transaction, breadcrumb, or log) are now dropped while that callback runs, instead of recursing. Captures made by event processors are unaffected. - Record byte-level client reports when event processors discard logs or trace metrics ([#5718](https://github.com/getsentry/sentry-java/pull/5718)) - Name the device-info caching thread `SentryDeviceInfoCache` so all threads spawned by the SDK are identifiable ([#5684](https://github.com/getsentry/sentry-java/pull/5684)) - Apply byte-category rate limits to log and trace metric envelope items ([#5716](https://github.com/getsentry/sentry-java/pull/5716)) diff --git a/sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberIntegrationTest.kt b/sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberIntegrationTest.kt index 43a45da7bb3..3c2cabfd5cc 100644 --- a/sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberIntegrationTest.kt +++ b/sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberIntegrationTest.kt @@ -1,10 +1,14 @@ package io.sentry.android.timber import io.sentry.IScopes +import io.sentry.ITransportFactory +import io.sentry.ScopesAdapter +import io.sentry.Sentry import io.sentry.SentryLevel import io.sentry.SentryLogLevel import io.sentry.SentryOptions import io.sentry.protocol.SdkVersion +import io.sentry.transport.ITransport import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals @@ -12,6 +16,7 @@ import kotlin.test.assertTrue import org.mockito.kotlin.any import org.mockito.kotlin.mock import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever import timber.log.Timber class SentryTimberIntegrationTest { @@ -112,4 +117,44 @@ class SentryTimberIntegrationTest { assertTrue(fixture.options.sdkVersion!!.integrationSet.contains("Timber")) } + + @Test + fun `a beforeSend callback that logs via Timber does not recurse`() { + // End-to-end guard against SDK-CRASHES-JAVA-3T3H style recursion: with a real Sentry instance, + // a beforeSend callback that logs through the planted SentryTimberTree must not loop back into + // capture forever. + val transport = mock() + val transportFactory = mock() + whenever(transportFactory.create(any(), any())).thenReturn(transport) + + var beforeSendInvocations = 0 + Sentry.init { options -> + options.dsn = "https://key@sentry.io/123" + options.setTransportFactory(transportFactory) + options.beforeSend = + SentryOptions.BeforeSendCallback { event, _ -> + beforeSendInvocations++ + Timber.e("logging from beforeSend") + event + } + } + Timber.plant( + SentryTimberTree( + ScopesAdapter.getInstance(), + SentryLevel.ERROR, + SentryLevel.INFO, + SentryLogLevel.INFO, + ) + ) + + try { + Timber.e("outer error") + + // Without the core re-entrancy guard this recurses until a StackOverflowError. The nested + // Timber.e is dropped before its own beforeSend, so the callback runs exactly once. + assertEquals(1, beforeSendInvocations) + } finally { + Sentry.close() + } + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 09ede4804a3..a80618412db 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -7893,6 +7893,12 @@ public final class io/sentry/util/ScopesUtil { public static fun printScopesChain (Lio/sentry/IScopes;)V } +public final class io/sentry/util/SentryCallbackReentrancyGuard { + public static fun enter ()V + public static fun exit ()V + public static fun isActive ()Z +} + public final class io/sentry/util/SentryRandom { public fun ()V public static fun current ()Lio/sentry/util/Random; diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index f5c57f5ac5d..ad5acd17ee8 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -16,6 +16,7 @@ import io.sentry.util.ExceptionUtils; import io.sentry.util.Objects; import io.sentry.util.Pair; +import io.sentry.util.SentryCallbackReentrancyGuard; import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Collection; @@ -465,6 +466,7 @@ public Queue getBreadcrumbs() { @NotNull Breadcrumb breadcrumb, final @NotNull Hint hint) { try { + SentryCallbackReentrancyGuard.enter(); breadcrumb = callback.execute(breadcrumb, hint); } catch (Throwable e) { options @@ -477,6 +479,8 @@ public Queue getBreadcrumbs() { if (e.getMessage() != null) { breadcrumb.setData("sentry:message", e.getMessage()); } + } finally { + SentryCallbackReentrancyGuard.exit(); } return breadcrumb; } @@ -493,6 +497,10 @@ public void addBreadcrumb(@NotNull Breadcrumb breadcrumb, @Nullable Hint hint) { if (breadcrumb == null || breadcrumbs instanceof DisabledQueue) { return; } + // Drop breadcrumbs added from within a user callback to prevent recursion. + if (SentryCallbackReentrancyGuard.isActive()) { + return; + } SentryOptions.BeforeBreadcrumbCallback callback = options.getBeforeBreadcrumb(); if (callback != null) { if (hint == null) { diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 02934b1ed7d..be77e3a469f 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -107,6 +107,15 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul @NotNull SentryEvent event, final @Nullable IScope scope, @Nullable Hint hint) { Objects.requireNonNull(event, "SentryEvent is required."); + if (SentryCallbackReentrancyGuard.isActive()) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Event captured from within a callback (beforeSend/beforeBreadcrumb/beforeSendLog) was dropped to prevent recursion."); + return SentryId.EMPTY_ID; + } + if (hint == null) { hint = new Hint(); } @@ -237,6 +246,7 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul options.getSessionReplay().getBeforeErrorSampling(); if (beforeErrorSampling != null) { try { + SentryCallbackReentrancyGuard.enter(); shouldCaptureReplay = beforeErrorSampling.execute(event, hint); } catch (Throwable e) { options @@ -246,6 +256,8 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul "The beforeErrorSampling callback threw an exception. Proceeding with replay capture.", e); shouldCaptureReplay = true; + } finally { + SentryCallbackReentrancyGuard.exit(); } } if (shouldCaptureReplay) { @@ -300,6 +312,15 @@ private void finalizeTransaction(final @NotNull IScope scope, final @NotNull Hin @NotNull SentryReplayEvent event, final @Nullable IScope scope, @Nullable Hint hint) { Objects.requireNonNull(event, "SessionReplay is required."); + if (SentryCallbackReentrancyGuard.isActive()) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Replay event captured from within a callback was dropped to prevent recursion."); + return SentryId.EMPTY_ID; + } + if (hint == null) { hint = new Hint(); } @@ -929,11 +950,14 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint options.getBeforeEnvelopeCallback(); if (beforeEnvelopeCallback != null) { try { + SentryCallbackReentrancyGuard.enter(); beforeEnvelopeCallback.execute(envelope, hint); } catch (Throwable e) { options .getLogger() .log(SentryLevel.ERROR, "The BeforeEnvelope callback threw an exception.", e); + } finally { + SentryCallbackReentrancyGuard.exit(); } } @@ -957,6 +981,15 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint final @Nullable ProfilingTraceData profilingTraceData) { Objects.requireNonNull(transaction, "Transaction is required."); + if (SentryCallbackReentrancyGuard.isActive()) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Transaction captured from within a callback was dropped to prevent recursion."); + return SentryId.EMPTY_ID; + } + if (hint == null) { hint = new Hint(); } @@ -1171,6 +1204,15 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint @Override public @NotNull SentryId captureFeedback( final @NotNull Feedback feedback, @Nullable Hint hint, final @NotNull IScope scope) { + if (SentryCallbackReentrancyGuard.isActive()) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Feedback captured from within a callback was dropped to prevent recursion."); + return SentryId.EMPTY_ID; + } + SentryEvent event = new SentryEvent(); event.getContexts().setFeedback(feedback); @@ -1281,6 +1323,15 @@ public void captureSession(final @NotNull Session session, final @Nullable Hint @ApiStatus.Experimental @Override public void captureLog(@Nullable SentryLogEvent logEvent, @Nullable IScope scope) { + if (SentryCallbackReentrancyGuard.isActive()) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Log captured from within a callback was dropped to prevent recursion."); + return; + } + if (logEvent != null && scope != null) { logEvent = processLogEvent(logEvent, scope.getEventProcessors()); if (logEvent == null) { @@ -1335,6 +1386,15 @@ public void captureMetric( @Nullable SentryMetricsEvent metricsEvent, final @Nullable IScope scope, @Nullable Hint hint) { + if (SentryCallbackReentrancyGuard.isActive()) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Metric captured from within a callback was dropped to prevent recursion."); + return; + } + if (hint == null) { hint = new Hint(); } @@ -1595,6 +1655,7 @@ private void sortBreadcrumbsByDate( final SentryOptions.BeforeSendCallback beforeSend = options.getBeforeSend(); if (beforeSend != null) { try { + SentryCallbackReentrancyGuard.enter(); event = beforeSend.execute(event, hint); } catch (Throwable e) { options @@ -1606,6 +1667,8 @@ private void sortBreadcrumbsByDate( // drop event in case of an error in beforeSend due to PII concerns event = null; + } finally { + SentryCallbackReentrancyGuard.exit(); } } return event; @@ -1617,6 +1680,7 @@ private void sortBreadcrumbsByDate( options.getBeforeSendTransaction(); if (beforeSendTransaction != null) { try { + SentryCallbackReentrancyGuard.enter(); transaction = beforeSendTransaction.execute(transaction, hint); } catch (Throwable e) { options @@ -1628,6 +1692,8 @@ private void sortBreadcrumbsByDate( // drop transaction in case of an error in beforeSend due to PII concerns transaction = null; + } finally { + SentryCallbackReentrancyGuard.exit(); } } return transaction; @@ -1638,6 +1704,7 @@ private void sortBreadcrumbsByDate( final SentryOptions.BeforeSendCallback beforeSendFeedback = options.getBeforeSendFeedback(); if (beforeSendFeedback != null) { try { + SentryCallbackReentrancyGuard.enter(); event = beforeSendFeedback.execute(event, hint); } catch (Throwable e) { options @@ -1646,6 +1713,8 @@ private void sortBreadcrumbsByDate( // drop feedback in case of an error in beforeSend due to PII concerns event = null; + } finally { + SentryCallbackReentrancyGuard.exit(); } } return event; @@ -1656,6 +1725,7 @@ private void sortBreadcrumbsByDate( final SentryOptions.BeforeSendReplayCallback beforeSendReplay = options.getBeforeSendReplay(); if (beforeSendReplay != null) { try { + SentryCallbackReentrancyGuard.enter(); event = beforeSendReplay.execute(event, hint); } catch (Throwable e) { options @@ -1667,6 +1737,8 @@ private void sortBreadcrumbsByDate( // drop event in case of an error in beforeSend due to PII concerns event = null; + } finally { + SentryCallbackReentrancyGuard.exit(); } } return event; @@ -1677,6 +1749,7 @@ private void sortBreadcrumbsByDate( options.getLogs().getBeforeSend(); if (beforeSendLog != null) { try { + SentryCallbackReentrancyGuard.enter(); event = beforeSendLog.execute(event); } catch (Throwable e) { options @@ -1688,6 +1761,8 @@ private void sortBreadcrumbsByDate( // drop event in case of an error in beforeSendLog due to PII concerns event = null; + } finally { + SentryCallbackReentrancyGuard.exit(); } } return event; @@ -1699,6 +1774,7 @@ private void sortBreadcrumbsByDate( options.getMetrics().getBeforeSend(); if (beforeSendMetric != null) { try { + SentryCallbackReentrancyGuard.enter(); event = beforeSendMetric.execute(event, hint); } catch (Throwable e) { options @@ -1710,6 +1786,8 @@ private void sortBreadcrumbsByDate( // drop event in case of an error in beforeSendMetric due to PII concerns event = null; + } finally { + SentryCallbackReentrancyGuard.exit(); } } return event; diff --git a/sentry/src/main/java/io/sentry/util/SentryCallbackReentrancyGuard.java b/sentry/src/main/java/io/sentry/util/SentryCallbackReentrancyGuard.java new file mode 100644 index 00000000000..50af3fbb07e --- /dev/null +++ b/sentry/src/main/java/io/sentry/util/SentryCallbackReentrancyGuard.java @@ -0,0 +1,54 @@ +package io.sentry.util; + +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.Nullable; + +/** + * Thread-local re-entrancy guard that marks whether a user-supplied {@code before*} callback + * ({@code beforeSend}, {@code beforeBreadcrumb}, {@code beforeSendLog}, ...) is currently executing + * on the current thread. + * + *

A callback that itself triggers another SDK capture on the same thread — directly, or + * transitively through a logging integration that routes back into Sentry (e.g. Timber or the + * Gradle plugin's logcat instrumentation) — would otherwise recurse indefinitely and throw {@link + * StackOverflowError}. Capture entry points consult {@link #isActive()} and drop the nested capture + * while a callback is running. + * + *

The guard is set ONLY around each callback's {@code execute(...)} invocation, never around the + * whole capture pipeline, so captures made by event processors (which run outside the callback) are + * not affected. + * + *

The guard is a depth counter rather than a boolean so that nested {@link #exit()} calls cannot + * clear it while an outer callback is still running. Capture entry points drop while a callback is + * active, so callbacks should never nest — but a capture path lacking an entry check must not + * silently disarm the guard for the rest of the outer callback. + */ +@ApiStatus.Internal +public final class SentryCallbackReentrancyGuard { + + private static final ThreadLocal depth = new ThreadLocal<>(); + + private SentryCallbackReentrancyGuard() {} + + /** Whether a user callback is currently executing on this thread. */ + public static boolean isActive() { + final @Nullable Integer current = depth.get(); + return current != null && current > 0; + } + + /** Marks that a user callback is starting to execute on this thread. */ + public static void enter() { + final @Nullable Integer current = depth.get(); + depth.set(current == null ? 1 : current + 1); + } + + /** Marks that the user callback finished executing on this thread. */ + public static void exit() { + final @Nullable Integer current = depth.get(); + if (current == null || current <= 1) { + depth.remove(); + } else { + depth.set(current - 1); + } + } +} diff --git a/sentry/src/test/java/io/sentry/ScopeTest.kt b/sentry/src/test/java/io/sentry/ScopeTest.kt index 4b0047fdc18..bf1eda4c747 100644 --- a/sentry/src/test/java/io/sentry/ScopeTest.kt +++ b/sentry/src/test/java/io/sentry/ScopeTest.kt @@ -372,6 +372,28 @@ class ScopeTest { assertFalse(called) } + @Test + fun `when beforeBreadcrumb adds another breadcrumb, the nested breadcrumb is dropped and does not recurse`() { + var invocations = 0 + lateinit var scope: Scope + val options = + SentryOptions().apply { + beforeBreadcrumb = + SentryOptions.BeforeBreadcrumbCallback { breadcrumb, _ -> + invocations++ + scope.addBreadcrumb(Breadcrumb()) + breadcrumb + } + } + + scope = Scope(options) + scope.addBreadcrumb(Breadcrumb()) + + // Callback runs only for the outer breadcrumb; the nested one is dropped before its callback. + assertEquals(1, invocations) + assertEquals(1, scope.breadcrumbs.count()) + } + @Test fun `when adding breadcrumb and maxBreadcrumb is not 0, beforeBreadcrumb is executed`() { var called = false diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 5b407bfef3d..e7ef9ece193 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -284,6 +284,108 @@ class SentryClientTest { ) } + @Test + fun `when beforeSend captures another event, the nested capture is dropped and does not recurse`() { + var invocations = 0 + lateinit var sut: SentryClient + fixture.sentryOptions.setBeforeSend { e, _ -> + invocations++ + sut.captureEvent(SentryEvent()) + e + } + sut = fixture.getSut() + + sut.captureEvent(SentryEvent()) + + // Callback runs only for the outer event; the nested capture is dropped before its callback. + assertEquals(1, invocations) + verify(fixture.transport, times(1)).send(any(), anyOrNull()) + } + + @Test + fun `when beforeSend captures a log, the nested log is dropped`() { + val scope = createScope() + fixture.sentryOptions.logs.isEnabled = true + lateinit var sut: SentryClient + fixture.sentryOptions.setBeforeSend { e, _ -> + sut.captureLog( + SentryLogEvent(SentryId(), SentryNanotimeDate(), "nested", SentryLogLevel.WARN), + scope, + ) + e + } + sut = fixture.getSut() + + sut.captureEvent(SentryEvent()) + + // The shared guard spans capture types: a log emitted from beforeSend is dropped too. + verify(fixture.loggerBatchProcessor, never()).add(any()) + verify(fixture.transport, times(1)).send(any(), anyOrNull()) + } + + @Test + fun `when beforeSendLog logs again, the nested log is dropped and does not recurse`() { + val scope = createScope() + fixture.sentryOptions.logs.isEnabled = true + var invocations = 0 + lateinit var sut: SentryClient + fixture.sentryOptions.logs.setBeforeSend { l -> + invocations++ + sut.captureLog( + SentryLogEvent(SentryId(), SentryNanotimeDate(), "nested", SentryLogLevel.WARN), + scope, + ) + l + } + sut = fixture.getSut() + + sut.captureLog( + SentryLogEvent(SentryId(), SentryNanotimeDate(), "outer", SentryLogLevel.WARN), + scope, + ) + + assertEquals(1, invocations) + verify(fixture.loggerBatchProcessor, times(1)).add(any()) + } + + @Test + fun `when beforeSend captures feedback before an event, the guard is not cleared prematurely`() { + val scope = createScope() + var invocations = 0 + lateinit var sut: SentryClient + fixture.sentryOptions.setBeforeSend { e, _ -> + invocations++ + // Capturing feedback must not clear the re-entrancy guard for captures that follow it in the + // same callback, otherwise the captureEvent below would recurse. + sut.captureFeedback(Feedback("feedback"), null, scope) + sut.captureEvent(SentryEvent()) + e + } + sut = fixture.getSut() + + sut.captureEvent(SentryEvent()) + + assertEquals(1, invocations) + verify(fixture.transport, times(1)).send(any(), anyOrNull()) + } + + @Test + fun `when beforeSendFeedback captures feedback again, the nested capture is dropped and does not recurse`() { + val scope = createScope() + var invocations = 0 + lateinit var sut: SentryClient + fixture.sentryOptions.setBeforeSendFeedback { e, _ -> + invocations++ + sut.captureFeedback(Feedback("nested"), null, scope) + e + } + sut = fixture.getSut() + + sut.captureFeedback(Feedback("outer"), null, scope) + + assertEquals(1, invocations) + } + @Test fun `when beforeSendLog is set, callback is invoked`() { val scope = createScope() @@ -3125,6 +3227,25 @@ class SentryClientTest { assertTrue(beforeEnvelopeCalled) } + @Test + fun `when beforeEnvelopeCallback captures another event, the nested capture is dropped and does not recurse`() { + var invocations = 0 + lateinit var sut: SentryClient + val options = { options: SentryOptions -> + options.beforeEnvelopeCallback = + SentryOptions.BeforeEnvelopeCallback { _, _ -> + invocations++ + sut.captureEvent(SentryEvent()) + } + } + sut = fixture.getSut(options) + + sut.captureEvent(SentryEvent(), Hint()) + + assertEquals(1, invocations) + verify(fixture.transport, times(1)).send(any(), anyOrNull()) + } + @Test fun `beforeEnvelopeCallback may fail, but the transport is still sends the envelope `() { val sut = @@ -3353,6 +3474,24 @@ class SentryClientTest { assertFalse(called) } + @Test + fun `when beforeErrorSampling captures another event, the nested capture is dropped and does not recurse`() { + var invocations = 0 + lateinit var sut: SentryClient + fixture.sentryOptions.sessionReplay.beforeErrorSampling = + SentryReplayOptions.BeforeErrorSamplingCallback { _, _ -> + invocations++ + sut.captureEvent(SentryEvent().apply { exceptions = listOf(SentryException()) }) + true + } + sut = fixture.getSut() + + sut.captureEvent(SentryEvent().apply { exceptions = listOf(SentryException()) }) + + assertEquals(1, invocations) + verify(fixture.transport, times(1)).send(any(), anyOrNull()) + } + @Test fun `beforeErrorSampling returning false skips captureReplay`() { var called = false