fix: discard corrupt cache files instead of looping on them (resulting in an OOM exception)

src/Sentry/Internal/Extensions/StreamExtensions.cs

@@ -2,8 +2,22 @@ namespace Sentry.Internal.Extensions;
 
 internal static class StreamExtensions
 {
+    /// <summary>
+    /// Reads a single line from the stream.
+    /// </summary>
+    /// <param name="stream">The stream to read from.</param>
+    /// <param name="maxLength">
+    /// When supplied, the maximum number of bytes the line may occupy. Callers that know what a
+    /// reasonable length looks like should pass it, so that a corrupt stream containing no newline
+    /// fails fast instead of being buffered into memory in its entirety.
+    /// </param>
+    /// <param name="cancellationToken">The cancellation token.</param>
+    /// <exception cref="InvalidDataException">
+    /// No newline was found within <paramref name="maxLength"/> bytes.
+    /// </exception>
     public static async Task<byte[]> ReadLineAsync(
         this Stream stream,
+        int? maxLength = null,
         CancellationToken cancellationToken = default)
     {
         // This approach avoids reading one byte at a time.
@@ -37,6 +51,12 @@ public static async Task<byte[]> ReadLineAsync(
             }
 
             result.Write(buffer.Array, 0, bytesRead);
+
+            if (maxLength is { } limit && result.Length > limit)
+            {
+                throw new InvalidDataException(
+                    $"Expected a line of at most {limit} bytes but found no newline within that many.");
+            }
         }
 
         stream.Position -= overreach;

src/Sentry/Internal/Http/CachingTransport.cs

@@ -327,16 +327,32 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell
 
         _options.LogDebug("Reading cached envelope: {0}", file);
 
-        try
-        {
-            var stream = _fileSystem.OpenFileForReading(file);
+        var stream = _fileSystem.OpenFileForReading(file);
 #if NETFRAMEWORK || NETSTANDARD2_0
-            using (stream)
+        using (stream)
 #else
-            await using (stream.ConfigureAwait(false))
+        await using (stream.ConfigureAwait(false))
 #endif
+        {
+            Envelope? envelope = null;
+            try
+            {
+                envelope = await Envelope.DeserializeAsync(stream, cancellation).ConfigureAwait(false);
+            }
+            catch (OperationCanceledException)
+            {
+                // We're shutting down rather than failing to read the file, so leave it be.
+                throw;
+            }
+            catch (Exception ex)
             {
-                using (var envelope = await Envelope.DeserializeAsync(stream, cancellation).ConfigureAwait(false))
+                // Discard if we can't read the file - to avoid an infinite retry loop
+                LogFailureWithDiscard(file, ex);
+            }
+
+            if (envelope is not null)
+            {
+                using (envelope)
                 {
                     // Don't even try to send it if we are requesting cancellation.
                     cancellation.ThrowIfCancellationRequested();
@@ -385,11 +401,6 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell
                 }
             }
         }
-        catch (JsonException ex)
-        {
-            // Log deserialization errors
-            LogFailureWithDiscard(file, ex);
-        }
 
         // Envelope & file stream must be disposed prior to reaching this point
 
@@ -399,18 +410,7 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell
 
     private void LogFailureWithDiscard(string file, Exception ex)
     {
-        string? envelopeContents = null;
-        try
-        {
-            if (_fileSystem.FileExists(file))
-            {
-                envelopeContents = _fileSystem.ReadAllTextFromFile(file);
-            }
-        }
-        // ReSharper disable once EmptyGeneralCatchClause
-        catch
-        {
-        }
+        var envelopeContents = TryReadContentsForLogging(file);
 
         if (envelopeContents == null)
         {
@@ -422,6 +422,46 @@ private void LogFailureWithDiscard(string file, Exception ex)
         }
     }
 
+    /// <summary>
+    /// Only corrupt files get here and they can be huge, so don't read the whole thing
+    /// </summary>
+    private string? TryReadContentsForLogging(string file)
+    {
+        const int maxLength = 8 * 1024;
+
+        try
+        {
+            if (!_fileSystem.FileExists(file))
+            {
+                return null;
+            }
+
+            using var stream = _fileSystem.OpenFileForReading(file);
+
+            // One byte past the limit, so a full buffer means there was more to read
+            var buffer = new byte[maxLength + 1];
+            var read = 0;
+            while (read < buffer.Length)
+            {
+                var bytesRead = stream.Read(buffer, read, buffer.Length - read);
+                if (bytesRead <= 0)
+                {
+                    break;
+                }
+
+                read += bytesRead;
+            }
+
+            return read > maxLength
+                ? Encoding.UTF8.GetString(buffer, 0, maxLength) + "... (truncated)"
+                : Encoding.UTF8.GetString(buffer, 0, read);
+        }
+        catch
+        {
+            return null;
+        }
+    }
+
     // Gets the next cache file and moves it to "processing"
     private async Task<string?> TryPrepareNextCacheFileAsync(CancellationToken cancellationToken = default)
     {

src/Sentry/Protocol/Envelopes/Envelope.cs

@@ -11,6 +11,12 @@ namespace Sentry.Protocol.Envelopes;
 /// </summary>
 public sealed class Envelope : ISerializable, IDisposable
 {
+    /// <summary>
+    /// The envelope header is a single short JSON object.
+    /// Bounding the read stops us buffering a whole corrupt file into memory.
+    /// </summary>
+    internal const int MaxHeaderLineLength = 64 * 1024;
+
     // caches the event id from the header
     private SentryId? _eventId;
 
@@ -519,7 +525,7 @@ internal static Envelope FromMetric(TraceMetric metric)
         Stream stream,
         CancellationToken cancellationToken = default)
     {
-        var buffer = await stream.ReadLineAsync(cancellationToken).ConfigureAwait(false);
+        var buffer = await stream.ReadLineAsync(MaxHeaderLineLength, cancellationToken).ConfigureAwait(false);
 
         var header =
             Json.Parse(buffer, JsonExtensions.GetDictionaryOrNull)

src/Sentry/Protocol/Envelopes/EnvelopeItem.cs

@@ -30,6 +30,12 @@ public sealed class EnvelopeItem : ISerializable, IDisposable
     private const string LengthKey = "length";
     private const string FileNameKey = "filename";
 
+    /// <summary>
+    /// An item header is a single short JSON object.
+    /// Bounding the read stops us buffering a whole corrupt file into memory.
+    /// </summary>
+    internal const int MaxHeaderLineLength = 64 * 1024;
+
     /// <summary>
     /// Header associated with this envelope item.
     /// </summary>
@@ -431,7 +437,7 @@ internal static EnvelopeItem FromMetric(TraceMetric metric)
         Stream stream,
         CancellationToken cancellationToken = default)
     {
-        var buffer = await stream.ReadLineAsync(cancellationToken).ConfigureAwait(false);
+        var buffer = await stream.ReadLineAsync(MaxHeaderLineLength, cancellationToken).ConfigureAwait(false);
 
         return
             Json.Parse(buffer, JsonExtensions.GetDictionaryOrNull)

test/Sentry.Tests/Internals/Extensions/StreamExtensionsTests.cs

@@ -0,0 +1,49 @@
+using Sentry.Internal.Extensions;
+
+namespace Sentry.Tests.Internals.Extensions;
+
+public class StreamExtensionsTests
+{
+    private const int MaxLength = 1024;
+
+    [Fact]
+    public async Task ReadLineAsync_LineWithinLimit_ReadsIt()
+    {
+        // Arrange
+        var line = new string('a', MaxLength);
+        using var stream = (line + "\nrest").ToMemoryStream();
+
+        // Act
+        var result = await stream.ReadLineAsync(MaxLength);
+
+        // Assert
+        Encoding.UTF8.GetString(result).Should().Be(line);
+    }
+
+    [Fact]
+    public async Task ReadLineAsync_NoNewlineWithinLimit_ThrowsWithoutReadingTheRest()
+    {
+        // Arrange
+        using var stream = new MemoryStream(new byte[16 * MaxLength]);
+
+        // Act
+        await Assert.ThrowsAsync<InvalidDataException>(async () => await stream.ReadLineAsync(MaxLength));
+
+        // Assert
+        stream.Position.Should().BeLessThan(2 * MaxLength);
+    }
+
+    [Fact]
+    public async Task ReadLineAsync_NoMaxLength_ReadsToTheEnd()
+    {
+        // Arrange
+        var line = new string('a', 16 * MaxLength);
+        using var stream = line.ToMemoryStream();
+
+        // Act
+        var result = await stream.ReadLineAsync();
+
+        // Assert
+        Encoding.UTF8.GetString(result).Should().Be(line);
+    }
+}

test/Sentry.Tests/Internals/Http/CachingTransportTests.cs

@@ -300,6 +300,38 @@ public async Task Handle_Malformed_Envelopes_Gracefully()
         _options.FileSystem.FileExists(filePath).Should().BeFalse();
     }
 
+    [Fact]
+    public async Task Handle_Oversized_Malformed_Envelopes_Gracefully()
+    {
+        // Arrange
+        var cacheDirectoryPath =
+            _options.TryGetProcessSpecificCacheDirectoryPath() ??
+            throw new InvalidOperationException("Cache directory or DSN is not set.");
+        var processingDirectoryPath = Path.Combine(cacheDirectoryPath, "__processing");
+        var fileName = $"{Guid.NewGuid()}.envelope";
+        var filePath = Path.Combine(processingDirectoryPath, fileName);
+
+        _options.FileSystem.CreateDirectory(processingDirectoryPath);
+        _options.FileSystem.CreateFileForWriting(filePath, out var file);
+
+        // A crash mid-write leaves a file with no header and no newline anywhere in it
+        var zeroes = new byte[4 * Envelope.MaxHeaderLineLength];
+        file.Write(zeroes, 0, zeroes.Length);
+        file.Dispose();
+
+        // Act
+        using var innerTransport = new FakeTransport();
+        await using var transport = CachingTransport.Create(innerTransport, _options, startWorker: false);
+        await transport.FlushAsync(); // Flush the worker to process
+
+        // Assert
+        _options.FileSystem.FileExists(filePath).Should().BeFalse();
+
+        var entry = _logger.Entries
+            .Should().ContainSingle(x => x.Message.Contains("discarding cached envelope")).Subject;
+        entry.Message.Should().Contain("(truncated)");
+    }
+
     [Fact]
     public async Task NonTransientExceptionShouldLog()
     {

test/Sentry.Tests/SentrySdkTests.cs

@@ -346,6 +346,64 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed(bool? testDel
     }
 #endif
 
+    [Fact]
+    public async Task Init_WithOversizedCorruptCacheFile_DiscardsItAndStarts()
+    {
+        // Arrange
+        using var cacheDirectory = new TempDirectory();
+
+        var cachePath = new SentryOptions { Dsn = ValidDsn, CacheDirectoryPath = cacheDirectory.Path }
+            .TryGetProcessSpecificCacheDirectoryPath();
+        Directory.CreateDirectory(cachePath!);
+
+        // A crash mid-write leaves a file with no header and no newline anywhere in it
+        var file = Path.Combine(cachePath!, "poison.envelope");
+        File.WriteAllBytes(file, new byte[2 * Envelope.MaxHeaderLineLength]);
+
+        // Act
+        await RunSdk();
+
+        // Assert
+        File.Exists(file).Should().BeFalse();
+
+        var entries = ((TestOutputDiagnosticLogger)_logger).Entries;
+        entries.Should().Contain(x =>
+            x.Message.Contains("discarding cached envelope") && x.Message.Contains("(truncated)"));
+
+        // The next launch is where the file used to come back out of __processing
+        await RunSdk();
+
+        Directory.GetFiles(cacheDirectory.Path, "*.envelope", SearchOption.AllDirectories)
+            .Should().BeEmpty();
+
+        async Task RunSdk()
+        {
+            SentryOptions options = null;
+            try
+            {
+                using var _ = SentrySdk.Init(o =>
+                {
+                    o.DisableAppDomainProcessExitFlush();
+
+                    o.Dsn = ValidDsn;
+                    o.Debug = true;
+                    o.DiagnosticLogger = _logger;
+                    o.CacheDirectoryPath = cacheDirectory.Path;
+                    o.InitCacheFlushTimeout = TimeSpan.FromSeconds(30);
+                    o.Transport = Substitute.For<ITransport>();
+                    o.AutoSessionTracking = false;
+                    o.InitNativeSdks = false;
+                    options = o;
+                });
+            }
+            finally
+            {
+                var cachingTransport = (CachingTransport)options!.Transport;
+                await cachingTransport!.StopWorkerAsync();
+            }
+        }
+    }
+
     [Fact]
     public void Disposable_MultipleCalls_NoOp()
     {