diff --git a/src/Sentry/Internal/Extensions/StreamExtensions.cs b/src/Sentry/Internal/Extensions/StreamExtensions.cs
index e9d4d7a705..20ddf21a92 100644
--- a/src/Sentry/Internal/Extensions/StreamExtensions.cs
+++ b/src/Sentry/Internal/Extensions/StreamExtensions.cs
@@ -2,8 +2,22 @@ namespace Sentry.Internal.Extensions;
internal static class StreamExtensions
{
+ ///
+ /// Reads a single line from the stream.
+ ///
+ /// The stream to read from.
+ ///
+ /// 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.
+ ///
+ /// The cancellation token.
+ ///
+ /// No newline was found within bytes.
+ ///
public static async Task 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 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;
diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs
index f2117f60b2..aff92b36a9 100644
--- a/src/Sentry/Internal/Http/CachingTransport.cs
+++ b/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)
}
}
+ ///
+ /// Only corrupt files get here and they can be huge, so don't read the whole thing
+ ///
+ 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 TryPrepareNextCacheFileAsync(CancellationToken cancellationToken = default)
{
diff --git a/src/Sentry/Protocol/Envelopes/Envelope.cs b/src/Sentry/Protocol/Envelopes/Envelope.cs
index 10c6df12d8..0174dbd32a 100644
--- a/src/Sentry/Protocol/Envelopes/Envelope.cs
+++ b/src/Sentry/Protocol/Envelopes/Envelope.cs
@@ -11,6 +11,12 @@ namespace Sentry.Protocol.Envelopes;
///
public sealed class Envelope : ISerializable, IDisposable
{
+ ///
+ /// The envelope header is a single short JSON object.
+ /// Bounding the read stops us buffering a whole corrupt file into memory.
+ ///
+ 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)
diff --git a/src/Sentry/Protocol/Envelopes/EnvelopeItem.cs b/src/Sentry/Protocol/Envelopes/EnvelopeItem.cs
index 9e8ecf9380..5ca761e55e 100644
--- a/src/Sentry/Protocol/Envelopes/EnvelopeItem.cs
+++ b/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";
+ ///
+ /// An item header is a single short JSON object.
+ /// Bounding the read stops us buffering a whole corrupt file into memory.
+ ///
+ internal const int MaxHeaderLineLength = 64 * 1024;
+
///
/// Header associated with this envelope item.
///
@@ -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)
diff --git a/test/Sentry.Tests/Internals/Extensions/StreamExtensionsTests.cs b/test/Sentry.Tests/Internals/Extensions/StreamExtensionsTests.cs
new file mode 100644
index 0000000000..9928b9426f
--- /dev/null
+++ b/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(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);
+ }
+}
diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs
index f50029edab..a38c303465 100644
--- a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs
+++ b/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()
{
diff --git a/test/Sentry.Tests/SentrySdkTests.cs b/test/Sentry.Tests/SentrySdkTests.cs
index a601c91bc0..c0dde5651f 100644
--- a/test/Sentry.Tests/SentrySdkTests.cs
+++ b/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();
+ o.AutoSessionTracking = false;
+ o.InitNativeSdks = false;
+ options = o;
+ });
+ }
+ finally
+ {
+ var cachingTransport = (CachingTransport)options!.Transport;
+ await cachingTransport!.StopWorkerAsync();
+ }
+ }
+ }
+
[Fact]
public void Disposable_MultipleCalls_NoOp()
{