Extract plugin to temp directory before copying to final location, some minor cleanup

Dalamud/Interface/Textures/Internal/TextureManager.Wic.cs

@@ -91,7 +91,7 @@ internal sealed partial class TextureManager
                 throw new NullReferenceException($"{nameof(path)} cannot be null.");
 
             using var wrapAux = new WrapAux(wrap, true);
-            var pathTemp = Util.GetTempFileNameForFileReplacement(path);
+            var pathTemp = Util.GetReplaceableFileName(path);
             var trashfire = new List<Exception>();
             try
             {

Dalamud/Plugin/Internal/PluginManager.cs

@@ -48,6 +48,8 @@ internal class PluginManager : IInternalDisposableService
     /// </summary>
     public const int PluginWaitBeforeFreeDefault = 1000; // upped from 500ms, seems more stable
 
+    private const string BrokenMarkerFileName = ".broken";
+
     private static readonly ModuleLog Log = ModuleLog.Create<PluginManager>();
 
     private readonly object pluginListLock = new();
@@ -874,7 +876,7 @@ internal class PluginManager : IInternalDisposableService
     }
 
     /// <summary>
-    /// Cleanup disabled plugins. Does not target devPlugins.
+    /// Cleanup disabled and broken plugins. Does not target devPlugins.
     /// </summary>
     public void CleanupPlugins()
     {
@@ -882,6 +884,13 @@ internal class PluginManager : IInternalDisposableService
         {
             try
             {
+                if (File.Exists(Path.Combine(pluginDir.FullName, BrokenMarkerFileName)))
+                {
+                    Log.Warning("Cleaning up broken plugin {Name}", pluginDir.Name);
+                    pluginDir.Delete(true);
+                    continue;
+                }
+
                 var versionDirs = pluginDir.GetDirectories();
 
                 versionDirs = versionDirs
@@ -1423,7 +1432,8 @@ internal class PluginManager : IInternalDisposableService
         else
         {
             // If we are doing anything other than a fresh install, not having a workingPluginId is an error that must be fixed
-            Debug.Assert(inheritedWorkingPluginId != null, "inheritedWorkingPluginId != null");
+            if (inheritedWorkingPluginId != null)
+                throw new InvalidOperationException("Inherited WorkingPluginId must not be null");
         }
 
         // Ensure that we have a testing opt-in for this plugin if we are installing a testing version
@@ -1434,31 +1444,28 @@ internal class PluginManager : IInternalDisposableService
             this.configuration.QueueSave();
         }
 
-        var outputDir = new DirectoryInfo(Path.Combine(this.pluginDirectory.FullName, repoManifest.InternalName, version?.ToString() ?? string.Empty));
+        var pluginVersionsDir = new DirectoryInfo(Path.Combine(this.pluginDirectory.FullName, repoManifest.InternalName));
+        var tempOutputDir = new DirectoryInfo(FilesystemUtil.GetTempFileName());
+        var outputDir = new DirectoryInfo(Path.Combine(pluginVersionsDir.FullName, version?.ToString() ?? string.Empty));
+
+        FilesystemUtil.DeleteAndRecreateDirectory(tempOutputDir);
+        FilesystemUtil.DeleteAndRecreateDirectory(outputDir);
+
+        Log.Debug("Extracting plugin to {TempOutputDir}", tempOutputDir);
 
         try
         {
-            if (outputDir.Exists)
+            using var archive = new ZipArchive(zipStream);
-                outputDir.Delete(true);
 
-            outputDir.Create();
-        }
-        catch
-        {
-            // ignored, since the plugin may be loaded already
-        }
-
-        Log.Debug("Extracting to {OutputDir}", outputDir);
-
-        using (var archive = new ZipArchive(zipStream))
-        {
             foreach (var zipFile in archive.Entries)
             {
-                var outputFile = new FileInfo(Path.GetFullPath(Path.Combine(outputDir.FullName, zipFile.FullName)));
+                var outputFile = new FileInfo(
+                    Path.GetFullPath(Path.Combine(tempOutputDir.FullName, zipFile.FullName)));
 
-                if (!outputFile.FullName.StartsWith(outputDir.FullName, StringComparison.OrdinalIgnoreCase))
+                if (!outputFile.FullName.StartsWith(tempOutputDir.FullName, StringComparison.OrdinalIgnoreCase))
                 {
-                    throw new IOException("Trying to extract file outside of destination directory. See this link for more info: https://snyk.io/research/zip-slip-vulnerability");
+                    throw new IOException(
+                        "Trying to extract file outside of destination directory. See this link for more info: https://snyk.io/research/zip-slip-vulnerability");
                 }
 
                 if (outputFile.Directory == null)
@@ -1469,71 +1476,89 @@ internal class PluginManager : IInternalDisposableService
                 if (zipFile.Name.IsNullOrEmpty())
                 {
                     // Assuming Empty for Directory
-                    Log.Verbose($"ZipFile name is null or empty, treating as a directory: {outputFile.Directory.FullName}");
+                    Log.Verbose(
+                        "ZipFile name is null or empty, treating as a directory: {Path}", outputFile.Directory.FullName);
                     Directory.CreateDirectory(outputFile.Directory.FullName);
                     continue;
                 }
 
                 // Ensure directory is created
                 Directory.CreateDirectory(outputFile.Directory.FullName);
-
-                try
-                {
                 zipFile.ExtractToFile(outputFile.FullName, true);
             }
-                catch (Exception ex)
-                {
-                    if (outputFile.Extension.EndsWith("dll"))
-                    {
-                        throw new IOException($"Could not overwrite {zipFile.Name}: {ex.Message}");
-                    }
 
-                    Log.Error($"Could not overwrite {zipFile.Name}: {ex.Message}");
+            var tempDllFile = LocalPluginManifest.GetPluginFile(tempOutputDir, repoManifest);
-                }
+            var tempManifestFile = LocalPluginManifest.GetManifestFile(tempDllFile);
-            }
-        }
-
-        var dllFile = LocalPluginManifest.GetPluginFile(outputDir, repoManifest);
-        var manifestFile = LocalPluginManifest.GetManifestFile(dllFile);
 
             // We need to save the repoManifest due to how the repo fills in some fields that authors are not expected to use.
-        Util.WriteAllTextSafe(manifestFile.FullName, JsonConvert.SerializeObject(repoManifest, Formatting.Indented));
+            FilesystemUtil.WriteAllTextSafe(
+                tempManifestFile.FullName,
+                JsonConvert.SerializeObject(repoManifest, Formatting.Indented));
 
             // Reload as a local manifest, add some attributes, and save again.
-        var manifest = LocalPluginManifest.Load(manifestFile);
+            var tempManifest = LocalPluginManifest.Load(tempManifestFile);
 
-        if (manifest == null)
+            if (tempManifest == null)
                 throw new Exception("Plugin had no valid manifest");
 
-        if (manifest.InternalName != repoManifest.InternalName)
+            if (tempManifest.InternalName != repoManifest.InternalName)
             {
-            Directory.Delete(outputDir.FullName, true);
                 throw new Exception(
-                $"Distributed internal name does not match repo internal name: {manifest.InternalName} - {repoManifest.InternalName}");
+                    $"Distributed internal name does not match repo internal name: {tempManifest.InternalName} - {repoManifest.InternalName}");
             }
 
-        if (manifest.WorkingPluginId != Guid.Empty)
+            if (tempManifest.WorkingPluginId != Guid.Empty)
                 throw new Exception("Plugin shall not specify a WorkingPluginId");
 
-        manifest.WorkingPluginId = inheritedWorkingPluginId ?? Guid.NewGuid();
+            tempManifest.WorkingPluginId = inheritedWorkingPluginId ?? Guid.NewGuid();
 
             if (useTesting)
             {
-            manifest.Testing = true;
+                tempManifest.Testing = true;
             }
 
             // Document the url the plugin was installed from
-        manifest.InstalledFromUrl = repoManifest.SourceRepo.IsThirdParty ? repoManifest.SourceRepo.PluginMasterUrl : SpecialPluginSource.MainRepo;
+            tempManifest.InstalledFromUrl = repoManifest.SourceRepo.IsThirdParty
+                                                ? repoManifest.SourceRepo.PluginMasterUrl
+                                                : SpecialPluginSource.MainRepo;
 
-        manifest.Save(manifestFile, "installation");
+            tempManifest.Save(tempManifestFile, "installation");
 
-        Log.Information($"Installed plugin {manifest.Name} (testing={useTesting})");
+            // Copy the directory to the final location
+            Log.Debug("Copying plugin from {TempOutputDir} to {OutputDir}", tempOutputDir, outputDir);
+            FilesystemUtil.CopyFilesRecursively(tempOutputDir, outputDir);
 
-        var plugin = await this.LoadPluginAsync(dllFile, manifest, reason);
+            var finalDllFile = LocalPluginManifest.GetPluginFile(outputDir, repoManifest);
+            var finalManifestFile = LocalPluginManifest.GetManifestFile(finalDllFile);
+            var finalManifest = LocalPluginManifest.Load(finalManifestFile) ??
+                           throw new Exception("Plugin had no valid manifest after copy");
+
+            Log.Information("Installed plugin {InternalName} (testing={UseTesting})", tempManifest.Name, useTesting);
+            var plugin = await this.LoadPluginAsync(finalDllFile, finalManifest, reason);
 
             this.NotifyinstalledPluginsListChanged();
             return plugin;
         }
+        catch
+        {
+            // Attempt to clean up if we can
+            try
+            {
+                outputDir.Delete(true);
+            }
+            catch
+            {
+                // Write marker file if we can't, we'll try to do it at the next start
+                File.WriteAllText(Path.Combine(pluginVersionsDir.FullName, BrokenMarkerFileName), string.Empty);
+            }
+
+            throw;
+        }
+        finally
+        {
+            tempOutputDir.Delete(true);
+        }
+    }
 
     /// <summary>
     /// Load a plugin.
@@ -1547,7 +1572,6 @@ internal class PluginManager : IInternalDisposableService
     /// <returns>The loaded plugin.</returns>
     private async Task<LocalPlugin> LoadPluginAsync(FileInfo dllFile, LocalPluginManifest manifest, PluginLoadReason reason, bool isDev = false, bool isBoot = false, bool doNotLoad = false)
     {
-        var name = manifest?.Name ?? dllFile.Name;
         var loadPlugin = !doNotLoad;
 
         LocalPlugin? plugin;
@@ -1560,7 +1584,7 @@ internal class PluginManager : IInternalDisposableService
 
         if (isDev)
         {
-            Log.Information($"Loading dev plugin {name}");
+            Log.Information("Loading dev plugin {Name}", manifest.InternalName);
             plugin = new LocalDevPlugin(dllFile, manifest);
 
             // This is a dev plugin - turn ImGui asserts on by default if we haven't chosen yet
@@ -1569,7 +1593,7 @@ internal class PluginManager : IInternalDisposableService
         }
         else
         {
-            Log.Information($"Loading plugin {name}");
+            Log.Information("Loading plugin {Name}", manifest.InternalName);
             plugin = new LocalPlugin(dllFile, manifest);
         }
 
@@ -1663,7 +1687,7 @@ internal class PluginManager : IInternalDisposableService
                 }
                 else
                 {
-                    Log.Verbose($"{name} not loaded, wantToLoad:{wantedByAnyProfile} orphaned:{plugin.IsOrphaned}");
+                    Log.Verbose("{Name} not loaded, wantToLoad:{WantedByAnyProfile} orphaned:{IsOrphaned}", manifest.InternalName, wantedByAnyProfile, plugin.IsOrphaned);
                 }
             }
             catch (InvalidPluginException)

Dalamud/Plugin/Internal/Profiles/ProfileManager.cs

@@ -93,7 +93,7 @@ internal class ProfileManager : IServiceType
 
         if (!wasInAnyProfile && addIfNotDeclared)
         {
-            Log.Warning("'{Guid}'('{InternalName}') was not in any profile, adding to default with {Default}", workingPluginId, internalName, defaultState);
+            Log.Warning("{Guid}({InternalName}) was not in any profile, adding to default with {Default}", workingPluginId, internalName, defaultState);
             await this.DefaultProfile.AddOrUpdateAsync(workingPluginId, internalName, defaultState, false);
 
             return defaultState;

Dalamud/Plugin/Internal/Types/LocalPlugin.cs

@@ -605,6 +605,10 @@ internal class LocalPlugin : IAsyncDisposable
         if (this.loader != null)
             return;
 
+        this.DllFile.Refresh();
+        if (!this.DllFile.Exists)
+            throw new Exception($"Plugin DLL file at '{this.DllFile.FullName}' did not exist, cannot load.");
+
         try
         {
             this.loader = PluginLoader.CreateFromAssemblyFile(this.DllFile.FullName, SetupLoaderConfig);

Dalamud/Plugin/Internal/Types/Manifest/LocalPluginManifest.cs

@@ -57,15 +57,15 @@ internal record LocalPluginManifest : PluginManifest, ILocalPluginManifest
     /// <param name="reason">The reason the manifest was saved.</param>
     public void Save(FileInfo manifestFile, string reason)
     {
-        Log.Verbose("Saving manifest for '{PluginName}' because '{Reason}'", this.InternalName, reason);
+        Log.Verbose("Saving manifest for {PluginName} because {Reason}", this.InternalName, reason);
 
         try
         {
-            Util.WriteAllTextSafe(manifestFile.FullName, JsonConvert.SerializeObject(this, Formatting.Indented));
+            FilesystemUtil.WriteAllTextSafe(manifestFile.FullName, JsonConvert.SerializeObject(this, Formatting.Indented));
         }
         catch
         {
-            Log.Error("Could not write out manifest for '{PluginName}' because '{Reason}'", this.InternalName, reason);
+            Log.Error("Could not write out manifest for {PluginName} because {Reason}", this.InternalName, reason);
             throw;
         }
     }
@@ -78,7 +78,7 @@ internal record LocalPluginManifest : PluginManifest, ILocalPluginManifest
     public static LocalPluginManifest? Load(FileInfo manifestFile) => JsonConvert.DeserializeObject<LocalPluginManifest>(File.ReadAllText(manifestFile.FullName));
 
     /// <summary>
-    /// A standardized way to get the plugin DLL name that should accompany a manifest file. May not exist.
+    /// A standardized way to get the plugin DLL name that should accompany a manifest file.
     /// </summary>
     /// <param name="dir">Manifest directory.</param>
     /// <param name="manifest">The manifest.</param>
@@ -86,7 +86,7 @@ internal record LocalPluginManifest : PluginManifest, ILocalPluginManifest
     public static FileInfo GetPluginFile(DirectoryInfo dir, PluginManifest manifest) => new(Path.Combine(dir.FullName, $"{manifest.InternalName}.dll"));
 
     /// <summary>
-    /// A standardized way to get the manifest file that should accompany a plugin DLL. May not exist.
+    /// A standardized way to get the manifest file that should accompany a plugin DLL.
     /// </summary>
     /// <param name="dllFile">The plugin DLL.</param>
     /// <returns>The <see cref="PluginManifest"/> file.</returns>

Dalamud/Storage/ReliableFileStorage.cs

@@ -122,7 +122,7 @@ internal class ReliableFileStorage : IInternalDisposableService
         {
             if (this.db == null)
             {
-                Util.WriteAllBytesSafe(path, bytes);
+                FilesystemUtil.WriteAllBytesSafe(path, bytes);
                 return;
             }
 
@@ -146,7 +146,7 @@ internal class ReliableFileStorage : IInternalDisposableService
                     this.db.Update(file);
                 }
 
-                Util.WriteAllBytesSafe(path, bytes);
+                FilesystemUtil.WriteAllBytesSafe(path, bytes);
             });
         }
     }

Dalamud/Utility/FilesystemUtil.cs

@@ -0,0 +1,116 @@
+using System.ComponentModel;
+using System.IO;
+using System.Text;
+
+using Windows.Win32.Storage.FileSystem;
+
+namespace Dalamud.Utility;
+
+/// <summary>
+/// Helper functions for filesystem operations.
+/// </summary>
+public static class FilesystemUtil
+{
+    /// <summary>
+    /// Overwrite text in a file by first writing it to a temporary file, and then
+    /// moving that file to the path specified.
+    /// </summary>
+    /// <param name="path">The path of the file to write to.</param>
+    /// <param name="text">The text to write.</param>
+    public static void WriteAllTextSafe(string path, string text)
+    {
+        WriteAllTextSafe(path, text, Encoding.UTF8);
+    }
+
+    /// <summary>
+    /// Overwrite text in a file by first writing it to a temporary file, and then
+    /// moving that file to the path specified.
+    /// </summary>
+    /// <param name="path">The path of the file to write to.</param>
+    /// <param name="text">The text to write.</param>
+    /// <param name="encoding">Encoding to use.</param>
+    public static void WriteAllTextSafe(string path, string text, Encoding encoding)
+    {
+        WriteAllBytesSafe(path, encoding.GetBytes(text));
+    }
+
+    /// <summary>
+    /// Overwrite data in a file by first writing it to a temporary file, and then
+    /// moving that file to the path specified.
+    /// </summary>
+    /// <param name="path">The path of the file to write to.</param>
+    /// <param name="bytes">The data to write.</param>
+    public static unsafe void WriteAllBytesSafe(string path, byte[] bytes)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(path);
+
+        // Open the temp file
+        var tempPath = path + ".tmp";
+
+        using var tempFile = Windows.Win32.PInvoke.CreateFile(
+            tempPath,
+            (uint)(FILE_ACCESS_RIGHTS.FILE_GENERIC_READ | FILE_ACCESS_RIGHTS.FILE_GENERIC_WRITE),
+            FILE_SHARE_MODE.FILE_SHARE_NONE,
+            null,
+            FILE_CREATION_DISPOSITION.CREATE_ALWAYS,
+            FILE_FLAGS_AND_ATTRIBUTES.FILE_ATTRIBUTE_NORMAL,
+            null);
+
+        if (tempFile.IsInvalid)
+            throw new Win32Exception();
+
+        // Write the data
+        uint bytesWritten = 0;
+        if (!Windows.Win32.PInvoke.WriteFile(tempFile, new ReadOnlySpan<byte>(bytes), &bytesWritten, null))
+            throw new Win32Exception();
+
+        if (bytesWritten != bytes.Length)
+            throw new Exception($"Could not write all bytes to temp file ({bytesWritten} of {bytes.Length})");
+
+        if (!Windows.Win32.PInvoke.FlushFileBuffers(tempFile))
+            throw new Win32Exception();
+
+        tempFile.Close();
+
+        if (!Windows.Win32.PInvoke.MoveFileEx(tempPath, path, MOVE_FILE_FLAGS.MOVEFILE_REPLACE_EXISTING | MOVE_FILE_FLAGS.MOVEFILE_WRITE_THROUGH))
+            throw new Win32Exception();
+    }
+
+    /// <summary>
+    /// Generates a temporary file name.
+    /// </summary>
+    /// <returns>A temporary file name that is almost guaranteed to be unique.</returns>
+    internal static string GetTempFileName()
+    {
+        // https://stackoverflow.com/a/50413126
+        return Path.Combine(Path.GetTempPath(), "dalamud_" + Guid.NewGuid());
+    }
+
+    /// <summary>
+    /// Copy files recursively from one directory to another.
+    /// </summary>
+    /// <param name="source">The source directory.</param>
+    /// <param name="target">The target directory.</param>
+    internal static void CopyFilesRecursively(DirectoryInfo source, DirectoryInfo target)
+    {
+        foreach (var dir in source.GetDirectories())
+            CopyFilesRecursively(dir, target.CreateSubdirectory(dir.Name));
+
+        foreach (var file in source.GetFiles())
+            file.CopyTo(Path.Combine(target.FullName, file.Name));
+    }
+
+    /// <summary>
+    /// Delete and recreate a directory.
+    /// </summary>
+    /// <param name="dir">The directory to delete and recreate.</param>
+    internal static void DeleteAndRecreateDirectory(DirectoryInfo dir)
+    {
+        if (dir.Exists)
+        {
+            dir.Delete(true);
+        }
+
+        dir.Create();
+    }
+}

Dalamud/Utility/Util.cs

@@ -604,10 +604,9 @@ public static class Util
     /// </summary>
     /// <param name="path">The path of the file to write to.</param>
     /// <param name="text">The text to write.</param>
-    public static void WriteAllTextSafe(string path, string text)
+    [Api13ToDo("Remove.")]
-    {
+    [Obsolete("Replaced with FilesystemUtil.WriteAllTextSafe()")]
-        WriteAllTextSafe(path, text, Encoding.UTF8);
+    public static void WriteAllTextSafe(string path, string text) => FilesystemUtil.WriteAllTextSafe(path, text);
-    }
 
     /// <summary>
     /// Overwrite text in a file by first writing it to a temporary file, and then
@@ -616,10 +615,9 @@ public static class Util
     /// <param name="path">The path of the file to write to.</param>
     /// <param name="text">The text to write.</param>
     /// <param name="encoding">Encoding to use.</param>
-    public static void WriteAllTextSafe(string path, string text, Encoding encoding)
+    [Api13ToDo("Remove.")]
-    {
+    [Obsolete("Replaced with FilesystemUtil.WriteAllTextSafe()")]
-        WriteAllBytesSafe(path, encoding.GetBytes(text));
+    public static void WriteAllTextSafe(string path, string text, Encoding encoding) => FilesystemUtil.WriteAllTextSafe(path, text, encoding);
-    }
 
     /// <summary>
     /// Overwrite data in a file by first writing it to a temporary file, and then
@@ -627,41 +625,9 @@ public static class Util
     /// </summary>
     /// <param name="path">The path of the file to write to.</param>
     /// <param name="bytes">The data to write.</param>
-    public static unsafe void WriteAllBytesSafe(string path, byte[] bytes)
+    [Api13ToDo("Remove.")]
-    {
+    [Obsolete("Replaced with FilesystemUtil.WriteAllBytesSafe()")]
-        ArgumentException.ThrowIfNullOrEmpty(path);
+    public static void WriteAllBytesSafe(string path, byte[] bytes) => FilesystemUtil.WriteAllBytesSafe(path, bytes);
-
-        // Open the temp file
-        var tempPath = path + ".tmp";
-
-        using var tempFile = Windows.Win32.PInvoke.CreateFile(
-            tempPath,
-            (uint)(FILE_ACCESS_RIGHTS.FILE_GENERIC_READ | FILE_ACCESS_RIGHTS.FILE_GENERIC_WRITE),
-            FILE_SHARE_MODE.FILE_SHARE_NONE,
-            null,
-            FILE_CREATION_DISPOSITION.CREATE_ALWAYS,
-            FILE_FLAGS_AND_ATTRIBUTES.FILE_ATTRIBUTE_NORMAL,
-            null);
-
-        if (tempFile.IsInvalid)
-            throw new Win32Exception();
-
-        // Write the data
-        uint bytesWritten = 0;
-        if (!Windows.Win32.PInvoke.WriteFile(tempFile, new ReadOnlySpan<byte>(bytes), &bytesWritten, null))
-            throw new Win32Exception();
-
-        if (bytesWritten != bytes.Length)
-            throw new Exception($"Could not write all bytes to temp file ({bytesWritten} of {bytes.Length})");
-
-        if (!Windows.Win32.PInvoke.FlushFileBuffers(tempFile))
-            throw new Win32Exception();
-
-        tempFile.Close();
-
-        if (!Windows.Win32.PInvoke.MoveFileEx(tempPath, path, MOVE_FILE_FLAGS.MOVEFILE_REPLACE_EXISTING | MOVE_FILE_FLAGS.MOVEFILE_WRITE_THROUGH))
-            throw new Win32Exception();
-    }
 
     /// <summary>Gets a temporary file name, for use as the sourceFileName in
     /// <see cref="File.Replace(string,string,string?)"/>.</summary>
@@ -669,7 +635,7 @@ public static class Util
     /// <returns>A temporary file name that should be usable with <see cref="File.Replace(string,string,string?)"/>.
     /// </returns>
     /// <remarks>No write operation is done on the filesystem.</remarks>
-    public static string GetTempFileNameForFileReplacement(string targetFile)
+    public static string GetReplaceableFileName(string targetFile)
     {
         Span<byte> buf = stackalloc byte[9];
         Random.Shared.NextBytes(buf);