From bdb42df4dc50d004f4abcafe543005e7eba5ad55 Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Tue, 13 Oct 2026 10:29:07 +0100 Subject: [PATCH] Make raw LZMA setup and disposal exception-safe --- docs/API.md | 2 + .../Compressors/LZMA/LZ/LzOutWindow.Async.cs | 17 ++- .../Compressors/LZMA/LZ/LzOutWindow.cs | 17 ++- .../Compressors/LZMA/Lzma2EncoderStream.cs | 2 +- .../Compressors/LZMA/LzmaStream.Async.cs | 99 ++++++++++---- .../Compressors/LZMA/LzmaStream.cs | 124 +++++++++++++----- .../Streams/BufferPoolLifetimeTests.cs | 25 ++++ .../Streams/LzmaLifetimeTests.cs | 80 +++++++++++ 8 files changed, 291 insertions(+), 75 deletions(-) create mode 100644 tests/SharpCompress.Test/Streams/LzmaLifetimeTests.cs diff --git a/docs/API.md b/docs/API.md index bc4388d0..c8e2541f 100644 --- a/docs/API.md +++ b/docs/API.md @@ -836,6 +836,8 @@ using (var reader = ReaderFactory.OpenReader(stream)) `SharpCompressStream` rejects reading and starting recording after disposal. Its pooled ring buffer is released even when disposal of the underlying stream fails. +`LzmaStream` also rejects reads and writes after disposal. Failed initialization releases internal buffers without closing the supplied stream; successful construction retains the specified `leaveOpen` semantics. Disposal releases internal buffers even if finalization or underlying stream disposal throws. + `SharpCompress.Compressors.LZMA.Utilities.CrcCheckStream` validates its expected CRC on disposal. A mismatch throws `ArchiveOperationException`, but pooled buffers are still released. Repeated disposal is safe, including after failed CRC validation. Once disposed, `CanWrite` is `false`, and `Write` / `WriteAsync` throw `ObjectDisposedException`. ## Related Documentation diff --git a/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.Async.cs b/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.Async.cs index db3e87a9..6c7f541f 100644 --- a/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.Async.cs +++ b/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.Async.cs @@ -24,13 +24,20 @@ internal partial class OutWindow : IAsyncDisposable public async ValueTask DisposeAsync() { - await ReleaseStreamAsync().ConfigureAwait(false); - if (_buffer is null) + try { - return; + await ReleaseStreamAsync().ConfigureAwait(false); + } + finally + { + var buffer = _buffer; + _buffer = null; + _stream = null; + if (buffer is not null) + { + _pool.Return(buffer); + } } - _pool.Return(_buffer); - _buffer = null; } private async ValueTask FlushAsync(CancellationToken cancellationToken = default) diff --git a/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.cs b/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.cs index 81da8cf0..ef79fcdc 100644 --- a/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.cs +++ b/src/SharpCompress/Compressors/LZMA/LZ/LzOutWindow.cs @@ -58,13 +58,20 @@ internal partial class OutWindow : IDisposable public void Dispose() { - ReleaseStream(); - if (_buffer is null) + try { - return; + ReleaseStream(); + } + finally + { + var buffer = _buffer; + _buffer = null; + _stream = null; + if (buffer is not null) + { + _pool.Return(buffer); + } } - _pool.Return(_buffer); - _buffer = null; } public void Reset() diff --git a/src/SharpCompress/Compressors/LZMA/Lzma2EncoderStream.cs b/src/SharpCompress/Compressors/LZMA/Lzma2EncoderStream.cs index d2d6d501..7440ff66 100644 --- a/src/SharpCompress/Compressors/LZMA/Lzma2EncoderStream.cs +++ b/src/SharpCompress/Compressors/LZMA/Lzma2EncoderStream.cs @@ -354,7 +354,7 @@ internal sealed class Lzma2EncoderStream : Stream props[3] = (byte)(_dictionarySize >> 16); props[4] = (byte)(_dictionarySize >> 24); - var decoder = new Decoder(); + using var decoder = new Decoder(); decoder.SetDecoderProperties(props); using var input = new MemoryStream(compressedData, 0, compressedSize, writable: false); diff --git a/src/SharpCompress/Compressors/LZMA/LzmaStream.Async.cs b/src/SharpCompress/Compressors/LZMA/LzmaStream.Async.cs index c89fb923..cdb0c91f 100644 --- a/src/SharpCompress/Compressors/LZMA/LzmaStream.Async.cs +++ b/src/SharpCompress/Compressors/LZMA/LzmaStream.Async.cs @@ -43,24 +43,32 @@ public partial class LzmaStream isLzma2, leaveOpen ); - if (!isLzma2) + try { - if (presetDictionary != null) + if (!isLzma2) { - await lzma._outWindow.TrainAsync(presetDictionary).ConfigureAwait(false); - } + if (presetDictionary != null) + { + await lzma._outWindow.TrainAsync(presetDictionary).ConfigureAwait(false); + } - await lzma._rangeDecoder.InitAsync(inputStream).ConfigureAwait(false); - } - else - { - if (presetDictionary != null) - { - await lzma._outWindow.TrainAsync(presetDictionary).ConfigureAwait(false); - lzma._needDictReset = false; + await lzma._rangeDecoder.InitAsync(inputStream).ConfigureAwait(false); } + else + { + if (presetDictionary != null) + { + await lzma._outWindow.TrainAsync(presetDictionary).ConfigureAwait(false); + lzma._needDictReset = false; + } + } + return lzma; + } + catch + { + await lzma.ReleaseBuffersAsync().ConfigureAwait(false); + throw; } - return lzma; } /*public static async ValueTask CreateAsync( @@ -175,6 +183,7 @@ public partial class LzmaStream CancellationToken cancellationToken ) { + ThrowIfDisposed(); if (_endReached) { return 0; @@ -289,6 +298,7 @@ public partial class LzmaStream CancellationToken cancellationToken = default ) { + ThrowIfDisposed(); if (_endReached) { return 0; @@ -407,6 +417,7 @@ public partial class LzmaStream CancellationToken cancellationToken ) { + ThrowIfDisposed(); cancellationToken.ThrowIfCancellationRequested(); if (_encoder != null) { @@ -422,6 +433,7 @@ public partial class LzmaStream CancellationToken cancellationToken = default ) { + ThrowIfDisposed(); cancellationToken.ThrowIfCancellationRequested(); if (_encoder != null) { @@ -443,30 +455,59 @@ public partial class LzmaStream return; } _isDisposed = true; - - if (_encoder != null) + try { - _position = await _encoder.CodeAsync(null, true).ConfigureAwait(false); - _encoder.Dispose(); - } - - if (!_leaveOpen) - { - if (_inputStream is IAsyncDisposable asyncDisposableInputStream) + if (_encoder != null) { - await asyncDisposableInputStream.DisposeAsync().ConfigureAwait(false); - } - else - { - _inputStream?.Dispose(); + _position = await _encoder.CodeAsync(null, true).ConfigureAwait(false); + } + } + finally + { + try + { + await ReleaseBuffersAsync().ConfigureAwait(false); + } + finally + { + if (!_leaveOpen) + { + if (_inputStream is IAsyncDisposable asyncDisposableInputStream) + { + await asyncDisposableInputStream.DisposeAsync().ConfigureAwait(false); + } + else + { + _inputStream?.Dispose(); + } + } } } - await _outWindow.DisposeAsync().ConfigureAwait(false); - ReturnAsyncHeaderBuffer(); #if !LEGACY_DOTNET || NETSTANDARD2_1 await base.DisposeAsync().ConfigureAwait(false); #endif GC.SuppressFinalize(this); } + + private async ValueTask ReleaseBuffersAsync() + { + try + { +#pragma warning disable VSTHRD103 // Encoder.Dispose only returns buffers; finalization is separate. + _encoder?.Dispose(); +#pragma warning restore VSTHRD103 + } + finally + { + try + { + await _outWindow.DisposeAsync().ConfigureAwait(false); + } + finally + { + ReturnAsyncHeaderBuffer(); + } + } + } } diff --git a/src/SharpCompress/Compressors/LZMA/LzmaStream.cs b/src/SharpCompress/Compressors/LZMA/LzmaStream.cs index f45b33a3..d2a22a4e 100644 --- a/src/SharpCompress/Compressors/LZMA/LzmaStream.cs +++ b/src/SharpCompress/Compressors/LZMA/LzmaStream.cs @@ -54,24 +54,23 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable if (!isLzma2) { _dictionarySize = BinaryPrimitives.ReadInt32LittleEndian(properties.AsSpan(1)); - _outWindow.Create(_dictionarySize); - _decoder = new Decoder(); _decoder.SetDecoderProperties(properties); Properties = properties; _availableBytes = outputSize < 0 ? long.MaxValue : outputSize; _rangeDecoderLimit = inputSize; + // Validate decoder properties before acquiring the dictionary buffer. + _outWindow.Create(_dictionarySize); } else { _dictionarySize = 2 | (properties[0] & 1); _dictionarySize <<= (properties[0] >> 1) + 11; - _outWindow.Create(_dictionarySize); - Properties = new byte[1]; _availableBytes = 0; + _outWindow.Create(_dictionarySize); } } @@ -123,24 +122,32 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable isLzma2, leaveOpen ); - if (!isLzma2) + try { - if (presetDictionary != null) + if (!isLzma2) { - lzma._outWindow.Train(presetDictionary); - } + if (presetDictionary != null) + { + lzma._outWindow.Train(presetDictionary); + } - lzma._rangeDecoder.Init(inputStream); - } - else - { - if (presetDictionary != null) - { - lzma._outWindow.Train(presetDictionary); - lzma._needDictReset = false; + lzma._rangeDecoder.Init(inputStream); } + else + { + if (presetDictionary != null) + { + lzma._outWindow.Train(presetDictionary); + lzma._needDictReset = false; + } + } + return lzma; + } + catch + { + lzma.ReleaseBuffers(); + throw; } - return lzma; } private LzmaStream(LzmaEncoderProperties properties, bool isLzma2) @@ -175,21 +182,28 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable ) { var lzma = new LzmaStream(properties, isLzma2); - - lzma._encoder!.SetStreams(null, outputStream, -1, -1); - - if (presetDictionary != null) + try { - lzma._encoder.Train(presetDictionary); + lzma._encoder!.SetStreams(null, outputStream, -1, -1); + + if (presetDictionary != null) + { + lzma._encoder.Train(presetDictionary); + } + return lzma; + } + catch + { + lzma.ReleaseBuffers(); + throw; } - return lzma; } - public override bool CanRead => _encoder == null; + public override bool CanRead => !_isDisposed && _encoder == null; public override bool CanSeek => false; - public override bool CanWrite => _encoder != null; + public override bool CanWrite => !_isDisposed && _encoder != null; public override void Flush() { } @@ -202,21 +216,58 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable return; } _isDisposed = true; - if (disposing) + try { - if (_encoder != null) + if (disposing && _encoder != null) { _position = _encoder.Code(null, true); - _encoder.Dispose(); } - if (!_leaveOpen) - { - _inputStream?.Dispose(); - } - _outWindow.Dispose(); - ReturnAsyncHeaderBuffer(); } - base.Dispose(disposing); + finally + { + if (disposing) + { + try + { + ReleaseBuffers(); + } + finally + { + if (!_leaveOpen) + { + _inputStream?.Dispose(); + } + } + } + base.Dispose(disposing); + } + } + + private void ReleaseBuffers() + { + try + { + _encoder?.Dispose(); + } + finally + { + try + { + _outWindow.Dispose(); + } + finally + { + ReturnAsyncHeaderBuffer(); + } + } + } + + private void ThrowIfDisposed() + { + if (_isDisposed) + { + throw new ObjectDisposedException(nameof(LzmaStream)); + } } private byte[] GetAsyncHeaderBuffer() => _asyncHeaderBuffer ??= ArrayPool.Shared.Rent(6); @@ -242,6 +293,7 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable public override int Read(byte[] buffer, int offset, int count) { + ThrowIfDisposed(); if (_endReached) { return 0; @@ -337,6 +389,7 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable public override int ReadByte() { + ThrowIfDisposed(); if (_endReached) { return -1; @@ -506,6 +559,7 @@ public partial class LzmaStream : Stream, IStreamStack, IAsyncDisposable public override void Write(byte[] buffer, int offset, int count) { + ThrowIfDisposed(); if (_encoder != null) { _position = _encoder.Code(new MemoryStream(buffer, offset, count), false); diff --git a/tests/SharpCompress.Test/Streams/BufferPoolLifetimeTests.cs b/tests/SharpCompress.Test/Streams/BufferPoolLifetimeTests.cs index 28781b8c..dd26ba01 100644 --- a/tests/SharpCompress.Test/Streams/BufferPoolLifetimeTests.cs +++ b/tests/SharpCompress.Test/Streams/BufferPoolLifetimeTests.cs @@ -16,6 +16,31 @@ namespace SharpCompress.Test.Streams; public class BufferPoolLifetimeTests { + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task OutWindow_FailedFlush_ReturnsBuffer(bool asyncDispose) + { + var pool = new TrackingPool(); + using var output = new ThrowingOutputStream(); + var window = new OutWindow(pool); + window.Create(32); + window.Init(output); + window.SetLimit(1); + window.PutByte(42); + if (asyncDispose) + { + await Assert.ThrowsAsync(async () => await window.DisposeAsync()); + } + else + { + Assert.Throws(() => window.Dispose()); + } + window.Dispose(); + Assert.Equal(1, pool.ReturnCount); + Assert.Empty(pool.Active); + } + [Theory] [InlineData(false)] [InlineData(true)] diff --git a/tests/SharpCompress.Test/Streams/LzmaLifetimeTests.cs b/tests/SharpCompress.Test/Streams/LzmaLifetimeTests.cs new file mode 100644 index 00000000..a3decf6a --- /dev/null +++ b/tests/SharpCompress.Test/Streams/LzmaLifetimeTests.cs @@ -0,0 +1,80 @@ +using System; +using System.IO; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; +using SharpCompress.Compressors.LZMA; +using Xunit; + +namespace SharpCompress.Test.Streams; + +public class LzmaLifetimeTests +{ + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task FailedFinalizationReleasesEncoderBuffers(bool asyncDispose) + { + using var output = new ThrowingOutput(); + var stream = LzmaStream.Create(new LzmaEncoderProperties(), false, output); + var encoder = GetField(stream, "_encoder")!; + if (asyncDispose) + { + await Assert.ThrowsAsync(async () => await stream.DisposeAsync()); + } + else + { + Assert.Throws(() => stream.Dispose()); + } + Assert.Null(GetField(encoder, "_matchFinder")); + stream.Dispose(); + Assert.False(stream.CanWrite); + Assert.Throws(() => stream.Write(new byte[1], 0, 1)); + await Assert.ThrowsAsync(() => + stream.WriteAsync(new byte[1], 0, 1, CancellationToken.None) + ); + } + + [Fact] + public async Task DisposedLzma2ReaderCannotRentHeaderBuffer() + { + using var input = new MemoryStream(new byte[] { 0 }); + var stream = LzmaStream.Create(new byte[] { 0 }, input, leaveOpen: true); + stream.Dispose(); + await Assert.ThrowsAsync(() => + stream.ReadAsync(new byte[1], 0, 1, CancellationToken.None) + ); + Assert.Null(GetField(stream, "_asyncHeaderBuffer")); + Assert.False(stream.CanRead); + Assert.True(input.CanRead); + } + + [Fact] + public void InvalidPropertiesLeaveInputOpen() + { + using var input = new MemoryStream(); + Assert.Throws(() => + LzmaStream.Create(new byte[] { 225, 0, 0, 16, 0 }, input) + ); + Assert.True(input.CanRead); + } + + private static object? GetField(object instance, string name) => + instance + .GetType() + .GetField(name, BindingFlags.Instance | BindingFlags.NonPublic)! + .GetValue(instance); + + private sealed class ThrowingOutput : MemoryStream + { + public override void WriteByte(byte value) => + throw new IOException("Injected output failure"); + + public override Task WriteAsync( + byte[] buffer, + int offset, + int count, + CancellationToken cancellationToken + ) => Task.FromException(new IOException("Injected output failure")); + } +}