Fix the stream was disposed before its real end (OrchardCMS#17878)

Author: infofromca

src/OrchardCore.Modules/OrchardCore.Media/Controllers/AdminController.cs

@@ -201,96 +201,90 @@ public async Task<IActionResult> Upload(string path, string extensions)
             (_, _, _) => Task.FromResult<IActionResult>(Ok(new { })),
             async (files) =>
             {
-                if (string.IsNullOrEmpty(path))
+                var result = await ProcessMediaUploadAsync(path, files, allowedExtensions);
+
+                return Ok(new { files = result.ToArray() });
+            });
+    }
+
+    private async Task<List<object>> ProcessMediaUploadAsync(string path, IEnumerable<IFormFile> files, HashSet<string> allowedExtensions)
+    {
+        if (string.IsNullOrEmpty(path))
+        {
+            path = string.Empty;
+        }
+
+        var result = new List<object>();
+
+        // Loop through each file in the request.
+        foreach (var file in files)
+        {
+            var extension = Path.GetExtension(file.FileName);
+
+            if (!allowedExtensions.Contains(extension))
+            {
+                result.Add(new
+                {
+                    name = file.FileName,
+                    size = file.Length,
+                    folder = path,
+                    error = S["This file extension is not allowed: {0}", extension].ToString(),
+                });
+
+                if (_logger.IsEnabled(LogLevel.Information))
                 {
-                    path = string.Empty;
+                    _logger.LogInformation("File extension not allowed: '{File}'", file.FileName);
                 }
 
-                var result = new List<object>();
+                continue;
+            }
+
+            var fileName = _mediaNameNormalizerService.NormalizeFileName(file.FileName);
 
-                // Loop through each file in the request.
-                foreach (var file in files)
+            Stream stream = null;
+            try
+            {
+                var mediaFilePath = _mediaFileStore.Combine(path, fileName);
+                stream = file.OpenReadStream();
+                mediaFilePath = await _mediaFileStore.CreateFileFromStreamAsync(mediaFilePath, stream);
+
+                var mediaFile = await _mediaFileStore.GetFileInfoAsync(mediaFilePath);
+
+                await PreCacheRemoteMedia(mediaFile);
+
+                result.Add(CreateFileResult(mediaFile));
+            }
+            catch (ExistsFileStoreException ex)
+            {
+                _logger.LogWarning(ex, "An error occurred while uploading a media");
+
+                result.Add(new
                 {
-                    var extension = Path.GetExtension(file.FileName);
-
-                    if (!allowedExtensions.Contains(extension))
-                    {
-                        result.Add(new
-                        {
-                            name = file.FileName,
-                            size = file.Length,
-                            folder = path,
-                            error = S["This file extension is not allowed: {0}", extension].ToString(),
-                        });
-
-                        if (_logger.IsEnabled(LogLevel.Information))
-                        {
-                            _logger.LogInformation("File extension not allowed: '{File}'", file.FileName);
-                        }
-
-                        continue;
-                    }
-
-                    var fileName = _mediaNameNormalizerService.NormalizeFileName(file.FileName);
-
-                    Stream stream = null;
-                    try
-                    {
-                        var mediaFilePath = _mediaFileStore.Combine(path, fileName);
-                        stream = file.OpenReadStream();
-                        mediaFilePath = await _mediaFileStore.CreateFileFromStreamAsync(mediaFilePath, stream);
-
-                        var mediaFile = await _mediaFileStore.GetFileInfoAsync(mediaFilePath);
-
-                        // The .NET AWS SDK, and only that from the built-in ones (but others maybe too), disposes
-                        // the stream. There's no better way to check for that than handling the exception. An
-                        // alternative would be to re-read the file for every other storage provider as well but
-                        // that would be wasteful.
-                        try
-                        {
-                            stream.Position = 0;
-                        }
-                        catch (ObjectDisposedException)
-                        {
-                            stream = null;
-                        }
-
-                        await PreCacheRemoteMedia(mediaFile, stream);
-
-                        result.Add(CreateFileResult(mediaFile));
-                    }
-                    catch (ExistsFileStoreException ex)
-                    {
-                        _logger.LogWarning(ex, "An error occurred while uploading a media");
-
-                        result.Add(new
-                        {
-                            name = fileName,
-                            size = file.Length,
-                            folder = path,
-                            error = ex.Message,
-                        });
-                    }
-                    catch (Exception ex)
-                    {
-                        _logger.LogError(ex, "An error occurred while uploading a media");
-
-                        result.Add(new
-                        {
-                            name = fileName,
-                            size = file.Length,
-                            folder = path,
-                            error = ex.Message,
-                        });
-                    }
-                    finally
-                    {
-                        stream?.Dispose();
-                    }
-                }
+                    name = fileName,
+                    size = file.Length,
+                    folder = path,
+                    error = ex.Message,
+                });
+            }
+            catch (Exception ex)
+            {
+                _logger.LogError(ex, "An error occurred while uploading a media");
 
-                return Ok(new { files = result.ToArray() });
-            });
+                result.Add(new
+                {
+                    name = fileName,
+                    size = file.Length,
+                    folder = path,
+                    error = ex.Message,
+                });
+            }
+            finally
+            {
+                stream?.Dispose();
+            }
+        }
+
+        return result;
     }
 
     [HttpPost]
@@ -572,24 +566,19 @@ private string GetCacheBustingMediaPublicUrl(string path) =>
     // this, the Media Library page will try to load the thumbnail without a cache busting parameter, since
     // ShellFileVersionProvider won't find it in the local cache.
     // This is not required for files moved across folders, because the folder will be reopened anyway.
-    private async Task PreCacheRemoteMedia(IFileStoreEntry mediaFile, Stream stream = null)
+    private async Task PreCacheRemoteMedia(IFileStoreEntry mediaFile)
     {
         var mediaFileStoreCache = _serviceProvider.GetService<IMediaFileStoreCache>();
         if (mediaFileStoreCache == null)
         {
             return;
         }
 
-        Stream localStream = null;
-
-        if (stream == null)
-        {
-            stream = localStream = await _mediaFileStore.GetFileStreamAsync(mediaFile);
-        }
+        var localStream = await _mediaFileStore.GetFileStreamAsync(mediaFile);
 
         try
         {
-            await mediaFileStoreCache.SetCacheAsync(stream, mediaFile, HttpContext.RequestAborted);
+            await mediaFileStoreCache.SetCacheAsync(localStream, mediaFile, HttpContext.RequestAborted);
         }
         finally
         {

src/OrchardCore/OrchardCore.Media.Core/DefaultMediaFileStore.cs

@@ -169,16 +169,22 @@ public virtual async Task<string> CreateFileFromStreamAsync(string path, Stream
 
                 foreach (var mediaCreatingEventHandler in _mediaCreatingEventHandlers)
                 {
-                    // Creating stream disposed by using.
-                    using var creatingStream = outputStream;
-
-                    // Stop disposal of inputStream, as creating stream is the object to dispose.
-                    inputStream = null;
+                    var creatingStream = outputStream;
 
                     // Outputstream must be created by event handler.
                     outputStream = null;
 
-                    outputStream = await mediaCreatingEventHandler.MediaCreatingAsync(context, creatingStream);
+                    try
+                    {
+                        outputStream = await mediaCreatingEventHandler.MediaCreatingAsync(context, creatingStream);
+                    }
+                    finally
+                    {
+                        if (creatingStream != outputStream && creatingStream != inputStream)
+                        {
+                            creatingStream.Dispose();
+                        }
+                    }
                 }
 
                 return await _fileStore.CreateFileFromStreamAsync(context.Path, outputStream, overwrite);

test/OrchardCore.Tests/Modules/OrchardCore.Media/DefaultMediaFileStoreTests.cs

@@ -0,0 +1,115 @@
+using System.IO;
+using System.Threading.Tasks;
+using System.Collections.Generic;
+using Moq;
+using Xunit;
+using OrchardCore.Media.Core;
+using OrchardCore.FileStorage;
+using OrchardCore.Media.Events;
+using Microsoft.Extensions.Logging;
+
+namespace OrchardCore.Tests.Modules.OrchardCore.Media;
+
+public class DefaultMediaFileStoreTests
+{
+    [Fact]
+    public async Task CreateFileFromStreamAsync_NoHandlers_CallsFileStoreDirectly()
+    {
+        // Arrange
+        var fileStoreMock = new Mock<IFileStore>();
+        var loggerMock = new Mock<ILogger<DefaultMediaFileStore>>();
+        var inputStream = new MemoryStream();
+        fileStoreMock
+            .Setup(x => x.CreateFileFromStreamAsync("test.txt", inputStream, false))
+            .ReturnsAsync("result");
+
+        var store = new DefaultMediaFileStore(
+            fileStoreMock.Object,
+            "",
+            "",
+            new List<IMediaEventHandler>(),
+            new List<IMediaCreatingEventHandler>(),
+            loggerMock.Object);
+
+        // Act
+        var result = await store.CreateFileFromStreamAsync("test.txt", inputStream);
+
+        // Assert
+        Assert.Equal("result", result);
+        fileStoreMock.Verify(x => x.CreateFileFromStreamAsync("test.txt", inputStream, false), Times.Once);
+    }
+
+    [Fact]
+    public async Task CreateFileFromStreamAsync_HandlerReturnsInputStream_PassesInputStreamToFileStore()
+    {
+        // Arrange
+        var fileStoreMock = new Mock<IFileStore>();
+        var loggerMock = new Mock<ILogger<DefaultMediaFileStore>>();
+        var inputStream = new MemoryStream();
+        var handlerMock = new Mock<IMediaCreatingEventHandler>();
+        handlerMock
+            .Setup(x => x.MediaCreatingAsync(It.IsAny<MediaCreatingContext>(), inputStream))
+            .ReturnsAsync(inputStream);
+
+        fileStoreMock
+            .Setup(x => x.CreateFileFromStreamAsync("test.txt", inputStream, false))
+            .ReturnsAsync("result");
+
+        var store = new DefaultMediaFileStore(
+            fileStoreMock.Object,
+            "",
+            "",
+            new List<IMediaEventHandler>(),
+            new[] { handlerMock.Object },
+            loggerMock.Object);
+
+        // Act
+        var result = await store.CreateFileFromStreamAsync("test.txt", inputStream);
+
+        // Assert
+        Assert.Equal("result", result);
+        fileStoreMock.Verify(x => x.CreateFileFromStreamAsync("test.txt", inputStream, false), Times.Once);
+    }
+
+    [Fact]
+    public async Task CreateFileFromStreamAsync_HandlersCreateNewStreams_PassesCorrectStreams()
+    {
+        // Arrange
+        var fileStoreMock = new Mock<IFileStore>();
+        var loggerMock = new Mock<ILogger<DefaultMediaFileStore>>();
+        var inputStream = new MemoryStream();
+        var stream1 = new MemoryStream();
+        var stream2 = new MemoryStream();
+
+        var handler1 = new Mock<IMediaCreatingEventHandler>();
+        var handler2 = new Mock<IMediaCreatingEventHandler>();
+
+        handler1
+            .Setup(x => x.MediaCreatingAsync(It.IsAny<MediaCreatingContext>(), inputStream))
+            .ReturnsAsync(stream1);
+        handler2
+            .Setup(x => x.MediaCreatingAsync(It.IsAny<MediaCreatingContext>(), stream1))
+            .ReturnsAsync(stream2);
+
+        fileStoreMock
+            .Setup(x => x.CreateFileFromStreamAsync("test.txt", stream2, false))
+            .ReturnsAsync("result");
+
+        var store = new DefaultMediaFileStore(
+            fileStoreMock.Object,
+            "",
+            "",
+            new List<IMediaEventHandler>(),
+            new[] { handler1.Object, handler2.Object },
+            loggerMock.Object);
+
+        // Act
+        var result = await store.CreateFileFromStreamAsync("test.txt", inputStream);
+
+        // Assert
+        Assert.Equal("result", result);
+        handler1.Verify(x => x.MediaCreatingAsync(It.IsAny<MediaCreatingContext>(), inputStream), Times.Once);
+        handler2.Verify(x => x.MediaCreatingAsync(It.IsAny<MediaCreatingContext>(), stream1), Times.Once);
+        fileStoreMock.Verify(x => x.CreateFileFromStreamAsync("test.txt", stream2, false), Times.Once);
+    }
+}