diff --git a/CHANGELOG.md b/CHANGELOG.md index 1271dd85f..cd63869a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixes +- Hardened the `UnityLogger` to no longer throw when failing to format a log message. ([#2832](https://github.com/getsentry/sentry-unity/pull/2832)) - IL2CPP line numbers now work on Android x86/x86_64 builds. il2cpp fails to report the image UUID there, so the SDK falls back to looking the debug image up by name ([#2817](https://github.com/getsentry/sentry-unity/pull/2817)) ### Dependencies diff --git a/src/Sentry.Unity/UnityLogger.cs b/src/Sentry.Unity/UnityLogger.cs index 4f7ef6b0a..fc9672b7f 100644 --- a/src/Sentry.Unity/UnityLogger.cs +++ b/src/Sentry.Unity/UnityLogger.cs @@ -30,7 +30,56 @@ public void Log(SentryLevel logLevel, string? message, Exception? exception = nu return; } - _logger.Log(GetUnityLogType(logLevel), LogTag, $"({logLevel.ToString()}) {Format(message, args)} {exception}"); + // A diagnostic logger must never take its caller down with it. Callers routinely log from + // inside a `catch` that already handled the failure. + string logMessage; + try + { + logMessage = $"({logLevel.ToString()}) {Format(message, args)} {Describe(exception)}"; + } + catch (Exception e) + { + // Keep the raw message - a bad format string shouldn't erase what the caller wanted to say. + logMessage = $"({logLevel.ToString()}) {message} (formatting failed: {e.GetType().Name})"; + } + + try + { + _logger.Log(GetUnityLogType(logLevel), LogTag, logMessage); + } + catch + { + // Reporting a logging failure would have to go through the very thing that just failed. + } + } + + /// + /// Renders an exception without trusting , which walks the + /// stack trace and can throw on platforms with limited stack trace support, i.e. Nintendo Switch. + /// + private static string Describe(Exception? exception) + { + if (exception is null) + { + return Empty; + } + + try + { + return exception.ToString(); + } + catch (Exception e) + { + try + { + return $"{exception.GetType().FullName}: {exception.Message}" + + $" (stack trace unavailable: {e.GetType().Name})"; + } + catch + { + return ""; + } + } } internal static LogType GetUnityLogType(SentryLevel logLevel) diff --git a/test/Sentry.Unity.Tests/UnityLoggerTests.cs b/test/Sentry.Unity.Tests/UnityLoggerTests.cs index daad31632..cbd763b07 100644 --- a/test/Sentry.Unity.Tests/UnityLoggerTests.cs +++ b/test/Sentry.Unity.Tests/UnityLoggerTests.cs @@ -1,3 +1,4 @@ +using System; using NUnit.Framework; using Sentry.Unity.Tests.SharedClasses; using UnityEngine; @@ -50,4 +51,52 @@ public void Log_SetsTag() // The format is: "(logType, tag, message)" StringAssert.AreEqualIgnoringCase(UnityLogger.LogTag, testLogger.Logs[0].Item2); } + + /// + /// Callers log from inside `catch` blocks that already handled the failure. + /// + [Test] + public void Log_ExceptionThrowsWhileBeingStringified_DoesNotPropagateAndStillLogs() + { + var testLogger = new UnityTestLogger(); + var logger = new UnityLogger(new SentryOptions { DiagnosticLevel = SentryLevel.Debug }, testLogger); + + Assert.DoesNotThrow(() => + logger.Log(SentryLevel.Error, "Something failed", new ThrowingToStringException())); + + Assert.AreEqual(1, testLogger.Logs.Count); + var message = testLogger.Logs[0].Item3; + StringAssert.Contains("Something failed", message); + StringAssert.Contains(nameof(ThrowingToStringException), message); + } + + [Test] + public void Log_MessageAndArgumentsDoNotMatch_DoesNotPropagateAndKeepsTheRawMessage() + { + var testLogger = new UnityTestLogger(); + var logger = new UnityLogger(new SentryOptions { DiagnosticLevel = SentryLevel.Debug }, testLogger); + + // More placeholders than arguments - string.Format throws on this. + Assert.DoesNotThrow(() => logger.Log(SentryLevel.Debug, "{0} {1} {2}", null, "only-one")); + + Assert.AreEqual(1, testLogger.Logs.Count); + // The unformatted message is still worth more than a bare "formatting failed". + StringAssert.Contains("{0} {1} {2}", testLogger.Logs[0].Item3); + } + + private sealed class ThrowingToStringException : Exception + { + public ThrowingToStringException() + { } + + public ThrowingToStringException(string message) : base(message) + { } + + public ThrowingToStringException(string message, Exception innerException) : base(message, innerException) + { } + + public override string ToString() => throw new InvalidOperationException("cannot stringify"); + + public override string StackTrace => throw new InvalidOperationException("no stack trace here"); + } }