From 5d784b2ef84cbb0aed379b0255ae6246d8dc13fe Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Tue, 2 Nov 2021 13:57:07 +0900
Subject: [PATCH] Perform a consistency check by decoding the newly written
 `skin.ini` during `ComputeHash`

As this has regressed twice now, let's play it safe and bail rather than
stack overflowing. Note that as all the underlying issues that could
trigger this have been fixed, no additional tests have been added. To
test, comment out `SkinManager.cs` line 228-229 to cause a failure. The
new logic will kick in and show a log output message, but all tests will
still (correctly) pass.
---
 osu.Game/Skinning/SkinManager.cs | 78 ++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs
index 8891fe3a06..35f4cbd3d0 100644
--- a/osu.Game/Skinning/SkinManager.cs
+++ b/osu.Game/Skinning/SkinManager.cs
@@ -194,50 +194,61 @@ namespace osu.Game.Skinning
             string nameLine = @$"Name: {item.Name}";
             string authorLine = @$"Author: {item.Creator}";
 
+            var newLines = new[]
+            {
+                @"// The following content was automatically added by osu! during import, based on filename / folder metadata.",
+                @"[General]",
+                nameLine,
+                authorLine,
+            };
+
             var existingFile = item.Files.SingleOrDefault(f => f.Filename.Equals(@"skin.ini", StringComparison.OrdinalIgnoreCase));
 
-            if (existingFile != null)
+            if (existingFile == null)
             {
-                List<string> additionalLines = new List<string>();
+                // In the case a skin doesn't have a skin.ini yet, let's create one.
+                writeNewSkinIni();
+                return;
+            }
 
-                additionalLines.AddRange(new[]
+            using (Stream stream = new MemoryStream())
+            {
+                using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true))
                 {
-                    @"",
-                    @"// The following content was automatically added by osu! during import, based on filename / folder metadata.",
-                    @"[General]",
-                    nameLine,
-                    authorLine,
-                });
-
-                using (Stream stream = new MemoryStream())
-                {
-                    using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true))
+                    using (var existingStream = Files.Storage.GetStream(existingFile.FileInfo.StoragePath))
+                    using (var sr = new StreamReader(existingStream))
                     {
-                        using (var existingStream = Files.Storage.GetStream(existingFile.FileInfo.StoragePath))
-                        using (var sr = new StreamReader(existingStream))
-                        {
-                            string line;
-                            while ((line = sr.ReadLine()) != null)
-                                sw.WriteLine(line);
-                        }
-
-                        foreach (string line in additionalLines)
+                        string line;
+                        while ((line = sr.ReadLine()) != null)
                             sw.WriteLine(line);
                     }
 
-                    ReplaceFile(item, existingFile, stream);
+                    sw.WriteLine();
+
+                    foreach (string line in newLines)
+                        sw.WriteLine(line);
+                }
+
+                ReplaceFile(item, existingFile, stream);
+
+                // can be removed 20220502.
+                if (!ensureIniWasUpdated(item))
+                {
+                    Logger.Log($"Skin {item}'s skin.ini had issues and has been removed. Please report this and provide the problematic skin.", LoggingTarget.Database, LogLevel.Important);
+
+                    DeleteFile(item, item.Files.SingleOrDefault(f => f.Filename.Equals(@"skin.ini", StringComparison.OrdinalIgnoreCase)));
+                    writeNewSkinIni();
                 }
             }
-            else
+
+            void writeNewSkinIni()
             {
                 using (Stream stream = new MemoryStream())
                 {
                     using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true))
                     {
-                        sw.WriteLine(@"[General]");
-                        sw.WriteLine(nameLine);
-                        sw.WriteLine(authorLine);
-                        sw.WriteLine(@"Version: latest");
+                        foreach (string line in newLines)
+                            sw.WriteLine(line);
                     }
 
                     AddFile(item, stream, @"skin.ini");
@@ -245,6 +256,17 @@ namespace osu.Game.Skinning
             }
         }
 
+        private bool ensureIniWasUpdated(SkinInfo item)
+        {
+            // This is a final consistency check to ensure that hash computation doesn't enter an infinite loop.
+            // With other changes to the surrounding code this should never be hit, but until we are 101% sure that there
+            // are no other cases let's avoid a hard startup crash by bailing and alerting.
+
+            var instance = GetSkin(item);
+
+            return instance.Configuration.SkinInfo.Name == item.Name;
+        }
+
         protected override Task Populate(SkinInfo model, ArchiveReader archive, CancellationToken cancellationToken = default)
         {
             var instance = GetSkin(model);