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.
This commit is contained in:
Dean Herbert 2021-11-02 13:57:07 +09:00
parent 2e66ab453d
commit 5d784b2ef8

View File

@ -194,50 +194,61 @@ namespace osu.Game.Skinning
string nameLine = @$"Name: {item.Name}"; string nameLine = @$"Name: {item.Name}";
string authorLine = @$"Author: {item.Creator}"; 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)); 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))
{ {
@"", using (var existingStream = Files.Storage.GetStream(existingFile.FileInfo.StoragePath))
@"// The following content was automatically added by osu! during import, based on filename / folder metadata.", using (var sr = new StreamReader(existingStream))
@"[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)) string line;
using (var sr = new StreamReader(existingStream)) while ((line = sr.ReadLine()) != null)
{
string line;
while ((line = sr.ReadLine()) != null)
sw.WriteLine(line);
}
foreach (string line in additionalLines)
sw.WriteLine(line); 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 (Stream stream = new MemoryStream())
{ {
using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true))
{ {
sw.WriteLine(@"[General]"); foreach (string line in newLines)
sw.WriteLine(nameLine); sw.WriteLine(line);
sw.WriteLine(authorLine);
sw.WriteLine(@"Version: latest");
} }
AddFile(item, stream, @"skin.ini"); 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) protected override Task Populate(SkinInfo model, ArchiveReader archive, CancellationToken cancellationToken = default)
{ {
var instance = GetSkin(model); var instance = GetSkin(model);