Dispose stream in initializer if loading file fails (#1829)

Fixes #1681

Author: twsouthwick

src/DocumentFormat.OpenXml.Framework/Features/StreamPackageFeature.cs

@@ -50,7 +50,17 @@ public StreamPackageFeature(Stream stream, PackageOpenMode openMode, bool isOwne
         Mode = initialMode == FileMode.Create ? Mode = FileMode.Open : initialMode;
         Access = openMode == PackageOpenMode.Read ? FileAccess.Read : FileAccess.ReadWrite;
 
-        InitializePackage(initialMode, Access);
+        try
+        {
+            InitializePackage(initialMode, Access);
+        }
+        catch when (isOwned)
+        {
+            // Ensure that the stream if created is disposed before leaving the constructor so we don't hold onto it
+            _stream?.Dispose();
+            throw;
+        }
+
         _isOwned = isOwned;
     }
 

test/DocumentFormat.OpenXml.Framework.Tests/Features/FilePackageFeatureTests.cs

@@ -0,0 +1,26 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using DocumentFormat.OpenXml.Builder;
+using System.IO;
+using Xunit;
+
+namespace DocumentFormat.OpenXml.Features.Tests;
+
+public class FilePackageFeatureTests
+{
+    [Fact]
+    public void OpenInvalidFileFailsGracefully()
+    {
+        // Arrange
+        var path = Path.GetTempFileName();
+        File.WriteAllText(path, "This is not a valid document");
+
+        // Act
+        Assert.Throws<FileFormatException>(() => new FilePackageFeature(path, PackageOpenMode.Read));
+
+        // Assert
+        using var reopened = File.Open(path, FileMode.Open, FileAccess.Read);
+        Assert.NotNull(reopened);
+    }
+}

test/DocumentFormat.OpenXml.Framework.Tests/Features/StreamPackageFeatureTests.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using DocumentFormat.OpenXml.Builder;
+using DocumentFormat.OpenXml.Framework;
 using DocumentFormat.OpenXml.Packaging;
 using NSubstitute;
 using System;
@@ -399,6 +400,37 @@ public void PartReload()
         Assert.Same(relationshipBefore, relationshipAfter);
     }
 
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public void OpenInvalidStreamFailsGracefully(bool isOwned)
+    {
+        // Arrange
+        var stream = new DisposingWatcherInvalidStream([1, 2, 3, 4]);
+
+        // Act
+        Assert.Throws<FileFormatException>(() => new StreamPackageFeature(stream, PackageOpenMode.Read, isOwned: isOwned));
+
+        // Assert
+        Assert.Equal(isOwned, stream.IsDisposed);
+    }
+
+    private sealed class DisposingWatcherInvalidStream : DelegatingStream
+    {
+        public DisposingWatcherInvalidStream(byte[] bytes)
+            : base(new MemoryStream(bytes))
+        {
+        }
+
+        public bool IsDisposed { get; private set; }
+
+        protected override void Dispose(bool disposing)
+        {
+            IsDisposed = true;
+            base.Dispose(disposing);
+        }
+    }
+
     private static readonly PartInfo Part1 = new(new("/part1", UriKind.Relative), "type1/content");
     private static readonly PartInfo Part2 = new(new("/part2", UriKind.Relative), "type2/content");
     private static readonly PartInfo PartRels = new(new("/_rels/.rels", UriKind.Relative), "application/vnd.openxmlformats-package.relationships+xml");