From 53efb3cd98658b5df8ed4126c71bd77fedacbe58 Mon Sep 17 00:00:00 2001
From: Taylor Southwick <twsouthwick@outlook.com>
Date: Tue, 19 Nov 2024 15:58:06 -0800
Subject: [PATCH] Dispose stream in initializer if loading file fails

Fixes #1681
---
 .../Features/StreamPackageFeature.cs          | 12 ++++++-
 .../Features/FilePackageFeatureTests.cs       | 26 +++++++++++++++
 .../Features/StreamPackageFeatureTests.cs     | 32 +++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 test/DocumentFormat.OpenXml.Framework.Tests/Features/FilePackageFeatureTests.cs

diff --git a/src/DocumentFormat.OpenXml.Framework/Features/StreamPackageFeature.cs b/src/DocumentFormat.OpenXml.Framework/Features/StreamPackageFeature.cs
index c1316c54e..7decb5809 100644
--- a/src/DocumentFormat.OpenXml.Framework/Features/StreamPackageFeature.cs
+++ b/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;
     }
 
diff --git a/test/DocumentFormat.OpenXml.Framework.Tests/Features/FilePackageFeatureTests.cs b/test/DocumentFormat.OpenXml.Framework.Tests/Features/FilePackageFeatureTests.cs
new file mode 100644
index 000000000..21d696762
--- /dev/null
+++ b/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);
+    }
+}
diff --git a/test/DocumentFormat.OpenXml.Framework.Tests/Features/StreamPackageFeatureTests.cs b/test/DocumentFormat.OpenXml.Framework.Tests/Features/StreamPackageFeatureTests.cs
index fca9956a8..1a98fd917 100644
--- a/test/DocumentFormat.OpenXml.Framework.Tests/Features/StreamPackageFeatureTests.cs
+++ b/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");