Merge pull request #18715 from bdach/ruleset-mod-hardening

Add several protections when creating game-global available mods

Templates/Rulesets/ruleset-empty/osu.Game.Rulesets.EmptyFreeform/EmptyFreeformRuleset.cs

@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.Collections.Generic;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -41,7 +42,7 @@ namespace osu.Game.Rulesets.EmptyFreeform
                     return new[] { new EmptyFreeformModAutoplay() };
 
                 default:
-                    return new Mod[] { null };
+                    return Array.Empty<Mod>();
             }
         }
 

Templates/Rulesets/ruleset-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs

@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.Collections.Generic;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Sprites;
@@ -37,7 +38,7 @@ namespace osu.Game.Rulesets.Pippidon
                     return new[] { new PippidonModAutoplay() };
 
                 default:
-                    return new Mod[] { null };
+                    return Array.Empty<Mod>();
             }
         }
 

Templates/Rulesets/ruleset-scrolling-empty/osu.Game.Rulesets.EmptyScrolling/EmptyScrollingRuleset.cs

@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.Collections.Generic;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Sprites;
@@ -34,7 +35,7 @@ namespace osu.Game.Rulesets.EmptyScrolling
                     return new[] { new EmptyScrollingModAutoplay() };
 
                 default:
-                    return new Mod[] { null };
+                    return Array.Empty<Mod>();
             }
         }
 

Templates/Rulesets/ruleset-scrolling-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs

@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.Collections.Generic;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Sprites;
@@ -34,7 +35,7 @@ namespace osu.Game.Rulesets.Pippidon
                     return new[] { new PippidonModAutoplay() };
 
                 default:
-                    return new Mod[] { null };
+                    return Array.Empty<Mod>();
             }
         }
 

osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs

@@ -0,0 +1,81 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+#nullable enable
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Testing;
+using osu.Game.Beatmaps;
+using osu.Game.Rulesets;
+using osu.Game.Rulesets.Difficulty;
+using osu.Game.Rulesets.Mods;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Rulesets.UI;
+using osu.Game.Tests.Visual;
+
+namespace osu.Game.Tests.Rulesets
+{
+    [HeadlessTest]
+    public class TestSceneBrokenRulesetHandling : OsuTestScene
+    {
+        [Resolved]
+        private OsuGameBase gameBase { get; set; } = null!;
+
+        [SetUpSteps]
+        public void SetUpSteps()
+        {
+            AddStep("reset ruleset", () => Ruleset.Value = new OsuRuleset().RulesetInfo);
+        }
+
+        [Test]
+        public void TestNullModsReturnedByRulesetAreIgnored()
+        {
+            AddStep("set ruleset with null mods", () => Ruleset.Value = new TestRulesetWithNullMods().RulesetInfo);
+            AddAssert("no null mods in available mods", () => gameBase.AvailableMods.Value.SelectMany(kvp => kvp.Value).All(mod => mod != null));
+        }
+
+        [Test]
+        public void TestRulesetRevertedIfModsCannotBeRetrieved()
+        {
+            RulesetInfo ruleset = null!;
+
+            AddStep("store current ruleset", () => ruleset = Ruleset.Value);
+
+            AddStep("set API incompatible ruleset", () => Ruleset.Value = new TestAPIIncompatibleRuleset().RulesetInfo);
+            AddAssert("ruleset not changed", () => Ruleset.Value.Equals(ruleset));
+        }
+
+#nullable disable // purposefully disabling nullability to simulate broken or unannotated API user code.
+
+        private class TestRulesetWithNullMods : Ruleset
+        {
+            public override string ShortName => "nullmods";
+            public override string Description => "nullmods";
+
+            public override IEnumerable<Mod> GetModsFor(ModType type) => new Mod[] { null };
+
+            public override DrawableRuleset CreateDrawableRulesetWith(IBeatmap beatmap, IReadOnlyList<Mod> mods = null) => null;
+            public override IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap) => null;
+            public override DifficultyCalculator CreateDifficultyCalculator(IWorkingBeatmap beatmap) => null;
+        }
+
+        private class TestAPIIncompatibleRuleset : Ruleset
+        {
+            public override string ShortName => "incompatible";
+            public override string Description => "incompatible";
+
+            // simulate API incompatibility by throwing similar exceptions.
+            public override IEnumerable<Mod> GetModsFor(ModType type) => throw new MissingMethodException();
+
+            public override DrawableRuleset CreateDrawableRulesetWith(IBeatmap beatmap, IReadOnlyList<Mod> mods = null) => null;
+            public override IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap) => null;
+            public override DifficultyCalculator CreateDifficultyCalculator(IWorkingBeatmap beatmap) => null;
+        }
+
+#nullable enable
+    }
+}

osu.Game/OsuGameBase.cs

@@ -159,7 +159,7 @@ namespace osu.Game
         /// <summary>
         /// Mods available for the current <see cref="Ruleset"/>.
         /// </summary>
-        public readonly Bindable<Dictionary<ModType, IReadOnlyList<Mod>>> AvailableMods = new Bindable<Dictionary<ModType, IReadOnlyList<Mod>>>();
+        public readonly Bindable<Dictionary<ModType, IReadOnlyList<Mod>>> AvailableMods = new Bindable<Dictionary<ModType, IReadOnlyList<Mod>>>(new Dictionary<ModType, IReadOnlyList<Mod>>());
 
         private BeatmapDifficultyCache difficultyCache;
 
@@ -511,21 +511,36 @@ namespace osu.Game
             if (instance == null)
             {
                 // reject the change if the ruleset is not available.
-                Ruleset.Value = r.OldValue?.Available == true ? r.OldValue : RulesetStore.AvailableRulesets.First();
+                revertRulesetChange();
                 return;
             }
 
             var dict = new Dictionary<ModType, IReadOnlyList<Mod>>();
 
-            foreach (ModType type in Enum.GetValues(typeof(ModType)))
+            try
             {
-                dict[type] = instance.GetModsFor(type).ToList();
+                foreach (ModType type in Enum.GetValues(typeof(ModType)))
+                {
+                    dict[type] = instance.GetModsFor(type)
+                                         // Rulesets should never return null mods, but let's be defensive just in case.
+                                         // ReSharper disable once ConditionIsAlwaysTrueOrFalse
+                                         .Where(mod => mod != null)
+                                         .ToList();
+                }
+            }
+            catch (Exception e)
+            {
+                Logger.Error(e, $"Could not load mods for \"{instance.RulesetInfo.Name}\" ruleset. Current ruleset has been rolled back.");
+                revertRulesetChange();
+                return;
             }
 
             if (!SelectedMods.Disabled)
                 SelectedMods.Value = Array.Empty<Mod>();
 
             AvailableMods.Value = dict;
+
+            void revertRulesetChange() => Ruleset.Value = r.OldValue?.Available == true ? r.OldValue : RulesetStore.AvailableRulesets.First();
         }
 
         private int allowableExceptions;

osu.Game/Rulesets/Ruleset.cs

@@ -93,6 +93,15 @@ namespace osu.Game.Rulesets
             return AllMods.FirstOrDefault(m => m is T)?.CreateInstance() as T;
         }
 
+        /// <summary>
+        /// Creates an enumerable with mods that are supported by the ruleset for the supplied <paramref name="type"/>.
+        /// </summary>
+        /// <remarks>
+        /// If there are no applicable mods from the given <paramref name="type"/> in this ruleset,
+        /// then the proper behaviour is to return an empty enumerable.
+        /// <see langword="null"/> mods should not be present in the returned enumerable.
+        /// </remarks>
+        [ItemNotNull]
         public abstract IEnumerable<Mod> GetModsFor(ModType type);
 
         /// <summary>