fix: stop the diagnostic logger from throwing while logging

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

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.
+        }
+    }
+
+    /// <summary>
+    /// Renders an exception without trusting <see cref="Exception.ToString"/>, which walks the
+    /// stack trace and can throw on platforms with limited stack trace support, i.e. Nintendo Switch.
+    /// </summary>
+    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 "<exception details unavailable>";
+            }
+        }
     }
 
     internal static LogType GetUnityLogType(SentryLevel logLevel)

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);
     }
+
+    /// <summary>
+    /// Callers log from inside `catch` blocks that already handled the failure.
+    /// </summary>
+    [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");
+    }
 }