From 4d65bf4852c4f8c0adc9d58981ecf16ebba9619d Mon Sep 17 00:00:00 2001 From: Paul Hebble Date: Fri, 13 Sep 2024 10:05:11 -0500 Subject: [PATCH] Tests for safety of string format calls --- Cmdline/Action/Available.cs | 5 +- Cmdline/Action/Cache.cs | 4 +- Cmdline/Action/Compare.cs | 4 +- Cmdline/Action/Compat.cs | 2 +- Cmdline/Action/Filter.cs | 2 +- Cmdline/Action/GameInstance.cs | 2 +- Cmdline/Action/Import.cs | 2 +- Cmdline/Action/Install.cs | 4 +- Cmdline/Action/Mark.cs | 2 +- Cmdline/Action/Remove.cs | 2 +- Cmdline/Action/Replace.cs | 2 +- Cmdline/Action/Repo.cs | 2 +- Cmdline/Action/Search.cs | 4 +- Cmdline/Action/Show.cs | 2 +- Cmdline/Action/Update.cs | 4 +- Cmdline/Action/Upgrade.cs | 6 +- Cmdline/Main.cs | 2 +- ConsoleUI/CKAN-ConsoleUI.csproj | 1 + ConsoleUI/GameInstanceListScreen.cs | 2 +- ConsoleUI/InstallScreen.cs | 14 +-- ConsoleUI/ModListScreen.cs | 8 +- ConsoleUI/Toolkit/ConsoleScreen.cs | 7 +- Core/CKAN-core.csproj | 1 + Core/Net/NetAsyncDownloader.cs | 2 +- Core/Net/NetAsyncModulesDownloader.cs | 4 +- Core/User.cs | 8 +- GUI/Controls/EditModpack.cs | 2 +- GUI/Controls/UnmanagedFiles.cs | 2 +- GUI/Dialogs/CloneGameInstanceDialog.cs | 12 +-- GUI/Dialogs/EditLabelsDialog.cs | 4 +- GUI/Dialogs/ManageGameInstancesDialog.cs | 2 +- GUI/Main/Main.cs | 8 +- GUI/Main/MainImport.cs | 2 +- GUI/Main/MainInstall.cs | 24 ++--- GUI/Main/MainRepo.cs | 6 +- GUI/Main/MainWait.cs | 4 +- Tests/GUI/ThreadSafetyTests.cs | 89 ++-------------- Tests/MonoCecilAnalysisTestsBase.cs | 111 +++++++++++++++++++ Tests/StringFormatSafetyTests.cs | 129 +++++++++++++++++++++++ Tests/Tests.csproj | 2 + 40 files changed, 337 insertions(+), 158 deletions(-) create mode 100644 Tests/MonoCecilAnalysisTestsBase.cs create mode 100644 Tests/StringFormatSafetyTests.cs diff --git a/Cmdline/Action/Available.cs b/Cmdline/Action/Available.cs index 85a365998a..8e8923b899 100644 --- a/Cmdline/Action/Available.cs +++ b/Cmdline/Action/Available.cs @@ -22,8 +22,9 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) .Where(m => !m.IsDLC) .OrderBy(m => m.identifier); - user.RaiseMessage(string.Format(Properties.Resources.AvailableHeader, - instance.game.ShortName, instance.Version())); + user.RaiseMessage(Properties.Resources.AvailableHeader, + instance.game.ShortName, + instance.Version()?.ToString() ?? ""); user.RaiseMessage(""); if (opts.detail) diff --git a/Cmdline/Action/Cache.cs b/Cmdline/Action/Cache.cs index 2db6343dcb..5d446772f4 100644 --- a/Cmdline/Action/Cache.cs +++ b/Cmdline/Action/Cache.cs @@ -224,7 +224,7 @@ private int ShowCacheSizeLimit() IConfiguration cfg = ServiceLocator.Container.Resolve(); if (cfg.CacheSizeLimit.HasValue) { - user?.RaiseMessage(CkanModule.FmtSize(cfg.CacheSizeLimit.Value)); + user?.RaiseMessage("{0}", CkanModule.FmtSize(cfg.CacheSizeLimit.Value)); } else { @@ -258,7 +258,7 @@ private void PrintUsage(string verb) { foreach (var h in CacheSubOptions.GetHelp(verb)) { - user?.RaiseError(h); + user?.RaiseError("{0}", h); } } diff --git a/Cmdline/Action/Compare.cs b/Cmdline/Action/Compare.cs index 495b010693..c464646745 100644 --- a/Cmdline/Action/Compare.cs +++ b/Cmdline/Action/Compare.cs @@ -27,7 +27,7 @@ public int RunCommand(object rawOptions) if (options.machine_readable) { - user.RaiseMessage(compareResult.ToString()); + user.RaiseMessage("{0}", compareResult.ToString()); } else if (compareResult == 0) { @@ -51,7 +51,7 @@ public int RunCommand(object rawOptions) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("compare")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } diff --git a/Cmdline/Action/Compat.cs b/Cmdline/Action/Compat.cs index a89673f140..7b9b675de1 100644 --- a/Cmdline/Action/Compat.cs +++ b/Cmdline/Action/Compat.cs @@ -295,7 +295,7 @@ private void PrintUsage(string verb) { foreach (var h in CompatSubOptions.GetHelp(verb)) { - user?.RaiseError(h); + user?.RaiseError("{0}", h); } } diff --git a/Cmdline/Action/Filter.cs b/Cmdline/Action/Filter.cs index 565700ecd6..c303e07dff 100644 --- a/Cmdline/Action/Filter.cs +++ b/Cmdline/Action/Filter.cs @@ -224,7 +224,7 @@ private void PrintUsage(string verb) { foreach (var h in FilterSubOptions.GetHelp(verb)) { - user?.RaiseError(h); + user?.RaiseError("{0}", h); } } diff --git a/Cmdline/Action/GameInstance.cs b/Cmdline/Action/GameInstance.cs index 59ce45f276..510ff78d65 100644 --- a/Cmdline/Action/GameInstance.cs +++ b/Cmdline/Action/GameInstance.cs @@ -690,7 +690,7 @@ private void PrintUsage(string verb) { foreach (var h in InstanceSubOptions.GetHelp(verb)) { - user?.RaiseError(h); + user?.RaiseError("{0}", h); } } diff --git a/Cmdline/Action/Import.cs b/Cmdline/Action/Import.cs index aa0ec7c98f..ca1dac0f4e 100644 --- a/Cmdline/Action/Import.cs +++ b/Cmdline/Action/Import.cs @@ -43,7 +43,7 @@ public int RunCommand(CKAN.GameInstance instance, object options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("import")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.ERROR; } diff --git a/Cmdline/Action/Install.cs b/Cmdline/Action/Install.cs index 8f91aa47a0..142222dc26 100644 --- a/Cmdline/Action/Install.cs +++ b/Cmdline/Action/Install.cs @@ -38,7 +38,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("install")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } @@ -164,7 +164,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) } catch (Kraken e) { - user.RaiseMessage(e.Message); + user.RaiseMessage("{0}", e.Message); return Exit.ERROR; } diff --git a/Cmdline/Action/Mark.cs b/Cmdline/Action/Mark.cs index 69d4798489..4ff6a039cb 100644 --- a/Cmdline/Action/Mark.cs +++ b/Cmdline/Action/Mark.cs @@ -147,7 +147,7 @@ private static void PrintUsage(IUser user, string verb) { foreach (var h in MarkSubOptions.GetHelp(verb)) { - user.RaiseError(h); + user.RaiseError("{0}", h); } } diff --git a/Cmdline/Action/Remove.cs b/Cmdline/Action/Remove.cs index feef3939ea..918eea41c5 100644 --- a/Cmdline/Action/Remove.cs +++ b/Cmdline/Action/Remove.cs @@ -113,7 +113,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("remove")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } diff --git a/Cmdline/Action/Replace.cs b/Cmdline/Action/Replace.cs index a6b37474a3..314b395edd 100644 --- a/Cmdline/Action/Replace.cs +++ b/Cmdline/Action/Replace.cs @@ -30,7 +30,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("replace")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } diff --git a/Cmdline/Action/Repo.cs b/Cmdline/Action/Repo.cs index ecb8cacdcd..4de1295245 100644 --- a/Cmdline/Action/Repo.cs +++ b/Cmdline/Action/Repo.cs @@ -451,7 +451,7 @@ private void PrintUsage(string verb) { foreach (var h in RepoSubOptions.GetHelp(verb)) { - user?.RaiseError(h); + user?.RaiseError("{0}", h); } } diff --git a/Cmdline/Action/Search.cs b/Cmdline/Action/Search.cs index 357e274f4a..d6d62d6b98 100644 --- a/Cmdline/Action/Search.cs +++ b/Cmdline/Action/Search.cs @@ -27,7 +27,7 @@ public int RunCommand(CKAN.GameInstance ksp, object raw_options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("search")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } @@ -117,7 +117,7 @@ public int RunCommand(CKAN.GameInstance ksp, object raw_options) foreach (CkanModule mod in matching) { - user.RaiseMessage(mod.identifier); + user.RaiseMessage("{0}", mod.identifier); } } diff --git a/Cmdline/Action/Show.cs b/Cmdline/Action/Show.cs index ea85c22d19..64601f0836 100644 --- a/Cmdline/Action/Show.cs +++ b/Cmdline/Action/Show.cs @@ -25,7 +25,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("show")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } diff --git a/Cmdline/Action/Update.cs b/Cmdline/Action/Update.cs index c2efa728b4..bc406837f8 100644 --- a/Cmdline/Action/Update.cs +++ b/Cmdline/Action/Update.cs @@ -96,7 +96,7 @@ public int RunCommand(object raw_options) catch (MissingCertificateKraken kraken) { // Handling the kraken means we have prettier output. - user.RaiseMessage(kraken.ToString()); + user.RaiseMessage("{0}", kraken.ToString()); return Exit.ERROR; } @@ -162,7 +162,7 @@ private void PrintModules(string message, IEnumerable modules) throw new BadCommandKraken("List of modules cannot be null."); } - user.RaiseMessage(message); + user.RaiseMessage("{0}", message); foreach (CkanModule module in modules) { diff --git a/Cmdline/Action/Upgrade.cs b/Cmdline/Action/Upgrade.cs index d09492f63e..1bd1462cf6 100644 --- a/Cmdline/Action/Upgrade.cs +++ b/Cmdline/Action/Upgrade.cs @@ -50,7 +50,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) user.RaiseError(Properties.Resources.ArgumentMissing); foreach (var h in Actions.GetHelp("upgrade")) { - user.RaiseError(h); + user.RaiseError("{0}", h); } return Exit.BADOPT; } @@ -93,7 +93,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) latestVersion?.ToString() ?? ""); if (update.ReleaseNotes != null) { - user.RaiseMessage(update.ReleaseNotes); + user.RaiseMessage("{0}", update.ReleaseNotes); } user.RaiseMessage(""); user.RaiseMessage(""); @@ -157,7 +157,7 @@ public int RunCommand(CKAN.GameInstance instance, object raw_options) } catch (InconsistentKraken kraken) { - user.RaiseMessage(kraken.ToString()); + user.RaiseMessage("{0}", kraken.ToString()); return Exit.ERROR; } catch (ModuleIsDLCKraken kraken) diff --git a/Cmdline/Main.cs b/Cmdline/Main.cs index 1a8508760c..6d33fe1c43 100644 --- a/Cmdline/Main.cs +++ b/Cmdline/Main.cs @@ -317,7 +317,7 @@ private static int ConsoleUi(GameInstanceManager manager, ConsoleUIOptions opts) private static int Version(IUser user) { - user.RaiseMessage(Meta.GetVersion(VersionFormat.Full)); + user.RaiseMessage("{0}", Meta.GetVersion(VersionFormat.Full)); return Exit.OK; } diff --git a/ConsoleUI/CKAN-ConsoleUI.csproj b/ConsoleUI/CKAN-ConsoleUI.csproj index c872c24ab7..56918eb4fc 100644 --- a/ConsoleUI/CKAN-ConsoleUI.csproj +++ b/ConsoleUI/CKAN-ConsoleUI.csproj @@ -38,6 +38,7 @@ + diff --git a/ConsoleUI/GameInstanceListScreen.cs b/ConsoleUI/GameInstanceListScreen.cs index fecd26c50f..e90bfa4594 100644 --- a/ConsoleUI/GameInstanceListScreen.cs +++ b/ConsoleUI/GameInstanceListScreen.cs @@ -94,7 +94,7 @@ public GameInstanceListScreen(ConsoleTheme theme, } catch (Exception ex) { // This can throw if the previous current instance had an error, // since it gets destructed when it's replaced. - RaiseError(ex.Message); + RaiseError("{0}", ex.Message); } return false; } else { diff --git a/ConsoleUI/InstallScreen.cs b/ConsoleUI/InstallScreen.cs index 9e2b35a041..014f28521d 100644 --- a/ConsoleUI/InstallScreen.cs +++ b/ConsoleUI/InstallScreen.cs @@ -123,9 +123,9 @@ public override void Run(Action? process = null) // Don't need to tell the user they just cancelled out. } catch (FileNotFoundKraken ex) { // Possible file corruption - RaiseError(ex.Message); + RaiseError("{0}", ex.Message); } catch (DirectoryNotFoundKraken ex) { - RaiseError(ex.Message); + RaiseError("{0}", ex.Message); } catch (FileExistsKraken ex) { if (ex.owningModule != null) { RaiseMessage(Properties.Resources.InstallOwnedFileConflict, ex.installingModule?.ToString() ?? "", ex.filename, ex.owningModule); @@ -134,9 +134,9 @@ public override void Run(Action? process = null) } RaiseError(Properties.Resources.InstallFilesReverted); } catch (DownloadErrorsKraken ex) { - RaiseError(ex.ToString()); + RaiseError("{0}", ex.ToString()); } catch (ModuleDownloadErrorsKraken ex) { - RaiseError(ex.ToString()); + RaiseError("{0}", ex.ToString()); } catch (DownloadThrottledKraken ex) { if (RaiseYesNoDialog(string.Format(Properties.Resources.InstallAuthTokenPrompt, ex.ToString()))) { if (ex.infoUrl != null) { @@ -145,9 +145,9 @@ public override void Run(Action? process = null) LaunchSubScreen(new AuthTokenScreen(theme)); } } catch (MissingCertificateKraken ex) { - RaiseError(ex.ToString()); + RaiseError("{0}", ex.ToString()); } catch (InconsistentKraken ex) { - RaiseError(ex.Message); + RaiseError("{0}", ex.Message); } catch (TooManyModsProvideKraken ex) { var ch = new ConsoleChoiceDialog( @@ -174,7 +174,7 @@ public override void Run(Action? process = null) } catch (ModNotInstalledKraken ex) { RaiseError(Properties.Resources.InstallNotInstalled, ex.mod); } catch (DllLocationMismatchKraken ex) { - RaiseError(ex.Message); + RaiseError("{0}", ex.Message); } } while (retry); } diff --git a/ConsoleUI/ModListScreen.cs b/ConsoleUI/ModListScreen.cs index 842a426f93..6713e1580f 100644 --- a/ConsoleUI/ModListScreen.cs +++ b/ConsoleUI/ModListScreen.cs @@ -455,7 +455,7 @@ private bool ViewSuggestions() RaiseError(Properties.Resources.ModListAuditNotFound); } } catch (ModuleNotFoundKraken k) { - RaiseError($"{k.module} {k.version}: {k.Message}"); + RaiseError("{0} {1}: {2}", k.module, k.version ?? "", k.Message); } return true; } @@ -496,7 +496,7 @@ private bool UpdateRegistry(bool showNewModsPrompt = true) userAgent); } catch (Exception ex) { // There can be errors while you re-install mods with changed metadata - ps.RaiseError(ex.Message + ex.StackTrace); + ps.RaiseError("{0}", ex.Message + ex.StackTrace); } // Update recent with mods that were updated in this pass foreach (CkanModule mod in registry.CompatibleModules( @@ -517,11 +517,9 @@ private bool UpdateRegistry(bool showNewModsPrompt = true) } private static string newModPrompt(int howMany) - { - return howMany == 1 + => howMany == 1 ? string.Format(Properties.Resources.ModListNewMod, howMany) : string.Format(Properties.Resources.ModListNewMods, howMany); - } private bool ScanForMods() { diff --git a/ConsoleUI/Toolkit/ConsoleScreen.cs b/ConsoleUI/Toolkit/ConsoleScreen.cs index 5f4e9c75c3..4b0591a118 100644 --- a/ConsoleUI/Toolkit/ConsoleScreen.cs +++ b/ConsoleUI/Toolkit/ConsoleScreen.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Diagnostics.CodeAnalysis; namespace CKAN.ConsoleUI.Toolkit { @@ -141,7 +142,8 @@ public int RaiseSelectionDialog(string message, params object[] args) /// /// Format string for the message /// Values to be interpolated into the format string - public void RaiseError(string message, params object[] args) + public void RaiseError([StringSyntax(StringSyntaxAttribute.CompositeFormat)] + string message, params object[] args) { var d = new ConsoleMessageDialog( theme, @@ -159,7 +161,8 @@ public void RaiseError(string message, params object[] args) /// /// Format string for the message /// Values to be interpolated into the format string - public void RaiseMessage(string message, params object[] args) + public void RaiseMessage([StringSyntax(StringSyntaxAttribute.CompositeFormat)] + string message, params object[] args) { Message(message, args); Draw(); diff --git a/Core/CKAN-core.csproj b/Core/CKAN-core.csproj index 40850bbc4e..1b706592a0 100644 --- a/Core/CKAN-core.csproj +++ b/Core/CKAN-core.csproj @@ -59,6 +59,7 @@ + diff --git a/Core/Net/NetAsyncDownloader.cs b/Core/Net/NetAsyncDownloader.cs index 06357e9084..3a22b605f8 100644 --- a/Core/Net/NetAsyncDownloader.cs +++ b/Core/Net/NetAsyncDownloader.cs @@ -62,7 +62,7 @@ public static void DownloadWithProgress(IList downloadTargets, { if (error != null) { - user?.RaiseError(error.ToString()); + user?.RaiseError("{0}", error.ToString()); } }; downloader.DownloadAndWait(downloadTargets); diff --git a/Core/Net/NetAsyncModulesDownloader.cs b/Core/Net/NetAsyncModulesDownloader.cs index aafd8dd5a5..de7e2cd01f 100644 --- a/Core/Net/NetAsyncModulesDownloader.cs +++ b/Core/Net/NetAsyncModulesDownloader.cs @@ -161,7 +161,7 @@ private void ModuleDownloadComplete(NetAsyncDownloader.DownloadTarget target, } catch (InvalidModuleFileKraken kraken) { - User.RaiseError(kraken.ToString()); + User.RaiseError("{0}", kraken.ToString()); if (module != null) { // Finish out the progress bar @@ -176,7 +176,7 @@ private void ModuleDownloadComplete(NetAsyncDownloader.DownloadTarget target, catch (OperationCanceledException exc) { log.WarnFormat("Cancellation token threw, validation incomplete: {0}", filename); - User.RaiseMessage(exc.Message); + User.RaiseMessage("{0}", exc.Message); if (module != null) { // Finish out the progress bar diff --git a/Core/User.cs b/Core/User.cs index 275a188556..27130c38f9 100644 --- a/Core/User.cs +++ b/Core/User.cs @@ -1,3 +1,5 @@ +using System.Diagnostics.CodeAnalysis; + namespace CKAN { /// @@ -18,11 +20,13 @@ public interface IUser /// /// The index of the item selected from the array or -1 if cancelled int RaiseSelectionDialog(string message, params object[] args); - void RaiseError(string message, params object[] args); + void RaiseError([StringSyntax(StringSyntaxAttribute.CompositeFormat)] + string message, params object[] args); void RaiseProgress(string message, int percent); void RaiseProgress(int percent, long bytesPerSecond, long bytesLeft); - void RaiseMessage(string message, params object[] args); + void RaiseMessage([StringSyntax(StringSyntaxAttribute.CompositeFormat)] + string message, params object[] args); } /// diff --git a/GUI/Controls/EditModpack.cs b/GUI/Controls/EditModpack.cs index 4b4388ec3b..767764c995 100644 --- a/GUI/Controls/EditModpack.cs +++ b/GUI/Controls/EditModpack.cs @@ -333,7 +333,7 @@ private void ExportModpackButton_Click(object? sender, EventArgs? e) if (!TryFieldsToModule(out string? error, out Control? badField)) { badField.Focus(); - user?.RaiseError(error); + user?.RaiseError("{0}", error); } else if (module != null && TrySavePrompt(modpackExportOptions, out string? filename)) { diff --git a/GUI/Controls/UnmanagedFiles.cs b/GUI/Controls/UnmanagedFiles.cs index c7b141936f..9ab3655c0d 100644 --- a/GUI/Controls/UnmanagedFiles.cs +++ b/GUI/Controls/UnmanagedFiles.cs @@ -210,7 +210,7 @@ private void DeleteButton_Click(object? sender, EventArgs? e) } catch (Exception exc) { - user?.RaiseError(exc.Message); + user?.RaiseError("{0}", exc.Message); } } } diff --git a/GUI/Dialogs/CloneGameInstanceDialog.cs b/GUI/Dialogs/CloneGameInstanceDialog.cs index 51d2558e50..796bab75f9 100644 --- a/GUI/Dialogs/CloneGameInstanceDialog.cs +++ b/GUI/Dialogs/CloneGameInstanceDialog.cs @@ -173,24 +173,24 @@ await Task.Run(() => } catch (NotKSPDirKraken kraken) { - user.RaiseError(string.Format(Properties.Resources.CloneFakeKspDialogInstanceNotValid, - Platform.FormatPath(kraken.path))); + user.RaiseError(Properties.Resources.CloneFakeKspDialogInstanceNotValid, + Platform.FormatPath(kraken.path)); reactivateDialog(); } catch (PathErrorKraken kraken) { - user.RaiseError(string.Format(Properties.Resources.CloneFakeKspDialogDestinationNotEmpty, - Platform.FormatPath(kraken?.path ?? ""))); + user.RaiseError(Properties.Resources.CloneFakeKspDialogDestinationNotEmpty, + Platform.FormatPath(kraken?.path ?? "")); reactivateDialog(); } catch (IOException ex) { - user.RaiseError(string.Format(Properties.Resources.CloneFakeKspDialogCloneFailed, ex.Message)); + user.RaiseError(Properties.Resources.CloneFakeKspDialogCloneFailed, ex.Message); reactivateDialog(); } catch (Exception ex) { - user.RaiseError(string.Format(Properties.Resources.CloneFakeKspDialogCloneFailed, ex.Message)); + user.RaiseError(Properties.Resources.CloneFakeKspDialogCloneFailed, ex.Message); reactivateDialog(); } } diff --git a/GUI/Dialogs/EditLabelsDialog.cs b/GUI/Dialogs/EditLabelsDialog.cs index 46d75de1a7..f791a148be 100644 --- a/GUI/Dialogs/EditLabelsDialog.cs +++ b/GUI/Dialogs/EditLabelsDialog.cs @@ -154,7 +154,7 @@ private void SaveButton_Click(object? sender, EventArgs? e) } else { - user.RaiseError(errMsg); + user.RaiseError("{0}", errMsg); } } @@ -281,7 +281,7 @@ private bool TryCloseEdit() { if (!TrySave(out string errMsg)) { - user.RaiseError(errMsg); + user.RaiseError("{0}", errMsg); return false; } } diff --git a/GUI/Dialogs/ManageGameInstancesDialog.cs b/GUI/Dialogs/ManageGameInstancesDialog.cs index f99f969e0d..edd1c7e00d 100644 --- a/GUI/Dialogs/ManageGameInstancesDialog.cs +++ b/GUI/Dialogs/ManageGameInstancesDialog.cs @@ -182,7 +182,7 @@ private void AddToCKANMenuItem_Click(object? sender, EventArgs? e) } catch (Exception exc) { - user.RaiseError(exc.Message); + user.RaiseError("{0}", exc.Message); } } diff --git a/GUI/Main/Main.cs b/GUI/Main/Main.cs index 2cfaab01f8..2211e12aaf 100644 --- a/GUI/Main/Main.cs +++ b/GUI/Main/Main.cs @@ -777,7 +777,7 @@ private void InstallFromCkanFiles(string[] files) } catch (Kraken kraken) { - currentUser.RaiseError(kraken.InnerException == null + currentUser.RaiseError("{0}", kraken.InnerException == null ? kraken.Message : $"{kraken.Message}: {kraken.InnerException.Message}"); @@ -785,7 +785,7 @@ private void InstallFromCkanFiles(string[] files) } catch (Exception ex) { - currentUser.RaiseError(ex.Message); + currentUser.RaiseError("{0}", ex.Message); continue; } @@ -960,12 +960,12 @@ private void ManageMods_OnRegistryChanged() private void ManageMods_RaiseMessage(string message) { - currentUser.RaiseMessage(message); + currentUser.RaiseMessage("{0}", message); } private void ManageMods_RaiseError(string error) { - currentUser.RaiseError(error); + currentUser.RaiseError("{0}", error); } private void ManageMods_SetStatusBar(string message) diff --git a/GUI/Main/MainImport.cs b/GUI/Main/MainImport.cs index 95c2cd8ab3..b76b0a2ab3 100644 --- a/GUI/Main/MainImport.cs +++ b/GUI/Main/MainImport.cs @@ -74,7 +74,7 @@ private void ImportModules() if (e?.Error is Exception exc) { log.Error(exc.Message, exc); - currentUser.RaiseMessage(exc.Message); + currentUser.RaiseMessage("{0}", exc.Message); } Wait.Finish(); } diff --git a/GUI/Main/MainInstall.cs b/GUI/Main/MainInstall.cs index 491e7b808f..5cacda8669 100644 --- a/GUI/Main/MainInstall.cs +++ b/GUI/Main/MainInstall.cs @@ -374,8 +374,8 @@ private void HandlePossibleConfigOnlyDirs(Registry registry, HashSet? po private void OnModInstalled(CkanModule mod) { - currentUser.RaiseMessage(string.Format(Properties.Resources.MainInstallModSuccess, - mod)); + currentUser.RaiseMessage(Properties.Resources.MainInstallModSuccess, + mod); LabelsAfterInstall(mod); } @@ -399,7 +399,7 @@ private void PostInstallMods(object? sender, RunWorkerCompletedEventArgs? e) break; case NotEnoughSpaceKraken exc: - currentUser.RaiseMessage(exc.Message); + currentUser.RaiseMessage("{0}", exc.Message); break; case FileExistsKraken exc: @@ -420,7 +420,7 @@ private void PostInstallMods(object? sender, RunWorkerCompletedEventArgs? e) break; case InconsistentKraken exc: - currentUser.RaiseMessage(exc.Message); + currentUser.RaiseMessage("{0}", exc.Message); break; case CancelledActionKraken exc: @@ -430,12 +430,12 @@ private void PostInstallMods(object? sender, RunWorkerCompletedEventArgs? e) break; case MissingCertificateKraken exc: - currentUser.RaiseMessage(exc.ToString()); + currentUser.RaiseMessage("{0}", exc.ToString()); break; case DownloadThrottledKraken exc: string msg = exc.ToString(); - currentUser.RaiseMessage(msg); + currentUser.RaiseMessage("{0}", msg); if (configuration != null && CurrentInstance != null && YesNoDialog(string.Format(Properties.Resources.MainInstallOpenSettingsPrompt, msg), Properties.Resources.MainInstallOpenSettings, @@ -465,15 +465,15 @@ private void PostInstallMods(object? sender, RunWorkerCompletedEventArgs? e) case ModuleIsDLCKraken exc: string dlcMsg = string.Format(Properties.Resources.MainInstallCantInstallDLC, exc.module.name); - currentUser.RaiseMessage(dlcMsg); - currentUser.RaiseError(dlcMsg); + currentUser.RaiseMessage("{0}", dlcMsg); + currentUser.RaiseError("{0}", dlcMsg); break; case TransactionalKraken exc: // Thrown when the Registry tries to enlist with multiple different transactions // Want to see the stack trace for this one - currentUser.RaiseMessage(exc.ToString()); - currentUser.RaiseError(exc.ToString()); + currentUser.RaiseMessage("{0}", exc.ToString()); + currentUser.RaiseError("{0}", exc.ToString()); break; case TransactionException texc: @@ -483,12 +483,12 @@ private void PostInstallMods(object? sender, RunWorkerCompletedEventArgs? e) .Reverse()) { log.Error(exc.Message, exc); - currentUser.RaiseMessage(exc.Message); + currentUser.RaiseMessage("{0}", exc.Message); } break; default: - currentUser.RaiseMessage(e.Error.Message); + currentUser.RaiseMessage("{0}", e.Error.Message); break; } diff --git a/GUI/Main/MainRepo.cs b/GUI/Main/MainRepo.cs index 0d40650562..0853c09680 100644 --- a/GUI/Main/MainRepo.cs +++ b/GUI/Main/MainRepo.cs @@ -187,7 +187,7 @@ private void PostUpdateRepo(object? sender, RunWorkerCompletedEventArgs? e) .Reverse()) { log.Error(exc.Message, exc); - currentUser.RaiseMessage(exc.Message); + currentUser.RaiseMessage("{0}", exc.Message); } currentUser.RaiseMessage(Properties.Resources.MainRepoFailed); Wait.Finish(); @@ -201,7 +201,7 @@ private void PostUpdateRepo(object? sender, RunWorkerCompletedEventArgs? e) .Reverse())) { log.Error(inner.Message, inner); - currentUser.RaiseMessage(inner.Message); + currentUser.RaiseMessage("{0}", inner.Message); } currentUser.RaiseMessage(Properties.Resources.MainRepoFailed); Wait.Finish(); @@ -210,7 +210,7 @@ private void PostUpdateRepo(object? sender, RunWorkerCompletedEventArgs? e) case Exception exc: log.Error(exc.Message, exc); - currentUser.RaiseMessage(exc.Message); + currentUser.RaiseMessage("{0}", exc.Message); currentUser.RaiseMessage(Properties.Resources.MainRepoFailed); Wait.Finish(); EnableMainWindow(); diff --git a/GUI/Main/MainWait.cs b/GUI/Main/MainWait.cs index bcffa8e1db..31537bdadb 100644 --- a/GUI/Main/MainWait.cs +++ b/GUI/Main/MainWait.cs @@ -54,14 +54,14 @@ public void FailWaitDialog(string statusMsg, string logMsg, string description) Util.Invoke(statusStrip1, () => { StatusProgress.Visible = false; - currentUser.RaiseMessage(statusMsg); + currentUser.RaiseMessage("{0}", statusMsg); }); Util.Invoke(WaitTabPage, () => { RecreateDialogs(); Wait.Finish(); }); - currentUser.RaiseMessage(logMsg); + currentUser.RaiseMessage("{0}", logMsg); Wait.SetDescription(description); } diff --git a/Tests/GUI/ThreadSafetyTests.cs b/Tests/GUI/ThreadSafetyTests.cs index d159a65123..353643cc77 100644 --- a/Tests/GUI/ThreadSafetyTests.cs +++ b/Tests/GUI/ThreadSafetyTests.cs @@ -16,18 +16,20 @@ namespace Tests.GUI using CallsDict = Dictionary>; [TestFixture] - public class ThreadSafetyTests + public class ThreadSafetyTests : MonoCecilAnalysisBase { [Test] public void GUIAssemblyModule_MethodsWithForbidGUICalls_DontCallGUI() { - // Arrange / Act + // Arrange var mainModule = ModuleDefinition.ReadModule(Assembly.GetAssembly(typeof(CKAN.GUI.Main))! .Location); var allMethods = mainModule.Types .SelectMany(GetAllNestedTypes) .SelectMany(t => t.Methods) .ToArray(); + + // Act var calls = allMethods.Where(hasForbidGUIAttribute) .Select(meth => new MethodCall() { meth }) .Concat(allMethods.SelectMany(FindStartedTasks)) @@ -44,29 +46,6 @@ public void GUIAssemblyModule_MethodsWithForbidGUICalls_DontCallGUI() }); } - private static IEnumerable GetAllNestedTypes(TypeDefinition td) - => Enumerable.Repeat(td, 1) - .Concat(td.NestedTypes.SelectMany(GetAllNestedTypes)); - - private static IEnumerable FindStartedTasks(MethodDefinition md) - => StartNewCalls(md).Select(FindStartNewArgument) - .OfType() - .Select(taskArg => new MethodCall() { md, taskArg }); - - private static IEnumerable StartNewCalls(MethodDefinition md) - => md.Body?.Instructions.Where(instr => callOpCodes.Contains(instr.OpCode.Name) - && instr.Operand is MethodReference mr - && isStartNew(mr)) - ?? Enumerable.Empty(); - - private static bool isStartNew(MethodReference mr) - => (mr.DeclaringType.Namespace == "System.Threading.Tasks" - && mr.DeclaringType.Name == "TaskFactory" - && mr.Name == "StartNew") - || (mr.DeclaringType.Namespace == "System.Threading.Tasks" - && mr.DeclaringType.Name == "Task" - && mr.Name == "Run"); - // The sequence to start a task seems to be: // 1. ldftn the function to start // 2. newobj a System.Action to hold it @@ -77,49 +56,11 @@ private static bool isStartNew(MethodReference mr) : FindStartNewArgument(instr.Previous); private static IEnumerable GetAllCallsWithoutForbidGUI(MethodCall initialStack) - => VisitMethodDefinition(initialStack, initialStack.Last(), new CallsDict(), hasForbidGUIAttribute, unsafeCall); - - private string SimpleName(MethodDefinition md) => $"{md.DeclaringType.Name}.{md.Name}"; - - // https://gist.github.com/lnicola/b48db1a6ff3617bdac2a - private static IEnumerable VisitMethodDefinition(MethodCall fullStack, - MethodDefinition methDef, - CallsDict calls, - Func skip, - Func stopAfter) - { - var called = calls[methDef] = methodsCalledBy(methDef).Distinct().ToList(); - foreach (var calledMeth in called) - { - if (!calls.ContainsKey(calledMeth) && !skip(calledMeth)) - { - var newStack = fullStack.Append(calledMeth).ToList(); - yield return newStack; - if (!stopAfter(calledMeth)) - { - // yield from, please! - foreach (var subcall in VisitMethodDefinition(newStack, calledMeth, calls, skip, stopAfter)) - { - yield return subcall; - } - } - } - } - } - - private static IEnumerable methodsCalledBy(MethodDefinition methDef) - => methDef.Body - .Instructions - .Where(instr => callOpCodes.Contains(instr.OpCode.Name)) - .Select(instr => instr.Operand as MethodDefinition - ?? GetSetterDef(instr.Operand as MethodReference)) - .OfType() - .Where(calledMeth => calledMeth?.Body != null); - - // Property setters are virtual and have references instead of definitions - private static MethodDefinition? GetSetterDef(MethodReference? mr) - => (mr?.Name.StartsWith("set_") ?? false) ? mr.Resolve() - : null; + => VisitMethodDefinition(initialStack, + initialStack.Last(), + new CallsDict(), + hasForbidGUIAttribute, + unsafeCall); private static bool hasForbidGUIAttribute(MethodDefinition md) => md.CustomAttributes.Any(attr => attr.AttributeType.Namespace == forbidAttrib.Namespace @@ -142,17 +83,5 @@ private static bool unsafeType(TypeDefinition t) private static readonly Type forbidAttrib = typeof(ForbidGUICallsAttribute); private static readonly string winformsNamespace = typeof(Control).Namespace!; - - private static readonly HashSet callOpCodes = new HashSet - { - // Constructors - "newobj", - - // Normal function calls - "call", - - // Virtual function calls (includes property setters) - "callvirt", - }; } } diff --git a/Tests/MonoCecilAnalysisTestsBase.cs b/Tests/MonoCecilAnalysisTestsBase.cs new file mode 100644 index 0000000000..37408ed29d --- /dev/null +++ b/Tests/MonoCecilAnalysisTestsBase.cs @@ -0,0 +1,111 @@ +using System; +using System.Linq; +using System.Collections.Generic; + +using Mono.Cecil; +using Mono.Cecil.Cil; +using CKAN.Extensions; + +namespace Tests +{ + using MethodCall = List; + using CallsDict = Dictionary>; + + public abstract class MonoCecilAnalysisBase + { + protected static string FullyQualifiedName(MethodReference md) + => $"{FullyQualifiedName(md.DeclaringType)}.{md.Name}"; + + protected static string FullyQualifiedName(TypeReference td) + => string.Join(".", td.TraverseNodes(td => td.DeclaringType) + .Reverse() + .Select(td => td.DeclaringType == null + && td.Namespace != null + ? $"{td.Namespace}.{td.Name}" + : td.Name)); + + protected static string SimpleName(MethodDefinition md) + => $"{md.DeclaringType.Name}.{md.Name}"; + + // https://gist.github.com/lnicola/b48db1a6ff3617bdac2a + protected static IEnumerable VisitMethodDefinition(MethodCall fullStack, + MethodDefinition methDef, + CallsDict calls, + Func skip, + Func stopAfter) + { + var called = calls[methDef] = methodsCalledBy(methDef).Distinct().ToList(); + foreach (var calledMeth in called) + { + if (!calls.ContainsKey(calledMeth) && !skip(calledMeth)) + { + var newStack = fullStack.Append(calledMeth).ToList(); + yield return newStack; + if (!stopAfter(calledMeth)) + { + // yield from, please! + foreach (var subcall in VisitMethodDefinition(newStack, calledMeth, calls, skip, stopAfter)) + { + yield return subcall; + } + } + } + } + } + + private static IEnumerable methodsCalledBy(MethodDefinition methDef) + => GetCallsBy(methDef).Select(instr => instr.Operand as MethodDefinition + ?? GetSetterDef(instr.Operand as MethodReference)) + .OfType(); + + protected static IEnumerable GetCallsBy(MethodDefinition methDef) + => methDef.Body + ?.Instructions + .Where(instr => callOpCodes.Contains(instr.OpCode.Name)) + ?? Enumerable.Empty(); + + // Property setters are virtual and have references instead of definitions + private static MethodDefinition? GetSetterDef(MethodReference? mr) + => (mr?.Name.StartsWith("set_") ?? false) ? mr.Resolve() + : null; + + protected static IEnumerable GetAllNestedTypes(TypeDefinition td) + => Enumerable.Repeat(td, 1) + .Concat(td.NestedTypes.SelectMany(GetAllNestedTypes)); + + protected static IEnumerable FindStartedTasks(MethodDefinition md) + => StartNewCalls(md).Select(FindStartNewArgument) + .OfType() + .Select(taskArg => new MethodCall() { md, taskArg }); + + private static IEnumerable StartNewCalls(MethodDefinition md) + => md.Body?.Instructions.Where(instr => callOpCodes.Contains(instr.OpCode.Name) + && instr.Operand is MethodReference mr + && isStartNew(mr)) + ?? Enumerable.Empty(); + + private static bool isStartNew(MethodReference mr) + => (mr.DeclaringType.Namespace == "System.Threading.Tasks" + && mr.DeclaringType.Name == "TaskFactory" + && mr.Name == "StartNew") + || (mr.DeclaringType.Namespace == "System.Threading.Tasks" + && mr.DeclaringType.Name == "Task" + && mr.Name == "Run"); + + private static MethodDefinition? FindStartNewArgument(Instruction instr) + => instr.OpCode.Name == "ldftn" ? instr.Operand as MethodDefinition + : FindStartNewArgument(instr.Previous); + + private static readonly HashSet callOpCodes = new HashSet + { + // Constructors + "newobj", + + // Normal function calls + "call", + + // Virtual function calls (includes property setters) + "callvirt", + }; + } +} diff --git a/Tests/StringFormatSafetyTests.cs b/Tests/StringFormatSafetyTests.cs new file mode 100644 index 0000000000..3a4fd78c06 --- /dev/null +++ b/Tests/StringFormatSafetyTests.cs @@ -0,0 +1,129 @@ +using System; +using System.Linq; +using System.Text.RegularExpressions; +using System.Collections.Generic; +using System.Reflection; +using System.Diagnostics.CodeAnalysis; + +using Mono.Cecil; +using Mono.Cecil.Cil; +using NUnit.Framework; + +using CKAN.Extensions; + +namespace Tests +{ + [TestFixture] + public class StringFormatSafetyTests : MonoCecilAnalysisBase + { + [TestCase(new object[] + { + typeof(CKAN.IUser), + typeof(CKAN.CmdLine.ConsoleUser), + typeof(CKAN.ConsoleUI.Toolkit.ConsoleScreen), + #if NETFRAMEWORK || WINDOWS + typeof(CKAN.GUI.GUIUser), + #endif + typeof(CKAN.NetKAN.ConsoleUser), + })] + public void AssemblyModule_StringSyntaxCompositeFormat_SameOrLiteralsOnly(object[] types) + { + // Arrange + var methodsByName = types.OfType() + .Select(t => Assembly.GetAssembly(t)) + .OfType() + .Select(a => ModuleDefinition.ReadModule(a.Location)) + .SelectMany(m => m.Types + .SelectMany(GetAllNestedTypes) + .SelectMany(t => t.Methods) + .SelectMany(GetMethodAndTasks)) + .GroupBy(FullyQualifiedName) + .Where(grp => grp.Count() == 1) + .ToDictionary(grp => grp.Key, + grp => grp.Single()); + var specialNames = methodsByName.Values.Where(AnyParamHasStringSyntaxAttribute) + .Select(FullyQualifiedName) + .ToHashSet(); + + // Act / Assert + Assert.Multiple(() => + { + foreach ((string name, MethodDefinition meth) in methodsByName) + { + foreach (var call in GetCallsBy(meth)) + { + if (call.Operand is MethodReference calledRef + && FullyQualifiedName(calledRef) is string calledRefName + && specialNames.Contains(calledRefName)) + { + Assert.IsFalse(IsEmptyArray(call.Previous) + && !IsStringLiteral(call.Previous.Previous) + && !IsI18nResource(call.Previous.Previous), + $"Unsafe format string in {name} --> {InstrDescription(call, meth)}({InstrDescription(call.Previous.Previous, meth)})"); + } + } + } + }); + } + + private static string? InstrDescription(Instruction instr, MethodDefinition parent) + => instr switch + { + { + Operand: MethodReference mRef, + } => FullyQualifiedName(mRef), + { + OpCode: {Name: "ldstr"}, + Operand: string s, + } => $"literal \"{s}\"", + { + OpCode: {Name: "ldfld"}, + Operand: FieldReference fRef, + } => $"field \"{fRef.Name}\"", + { + OpCode: {Name: string a}, + } => ldargPattern.TryMatch(a, out Match? argM) + && int.TryParse(argM.Groups["argNum"].Value, out int argNum) + && argNum > 0 && argNum <= parent.Parameters.Count + ? $"argument \"{parent.Parameters[argNum - 1].Name}\"" + : ldlocPattern.TryMatch(a, out Match? locM) + ? $"local {locM.Groups["locNum"].Value}" + : a, + _ => null, + }; + + private static readonly Regex ldargPattern = + new Regex(@"ldarg\.(?\d+$)", + RegexOptions.Compiled); + + private static readonly Regex ldlocPattern = + new Regex(@"ldloc\.(?.+$)", + RegexOptions.Compiled); + + private static bool IsEmptyArray(Instruction instr) + => instr.OpCode.Name == "call" + && instr.Operand is MethodReference mRef + && mRef.Name == "Empty"; + + private static bool IsI18nResource(Instruction instr) + => instr.Operand is MethodReference mRef + && FullyQualifiedName(mRef).Contains(".Properties.Resources."); + + private static bool IsStringLiteral(Instruction instr) + => instr.OpCode.Name == "ldstr" && instr.Operand is string; + + private static IEnumerable GetMethodAndTasks(MethodDefinition meth) + => Enumerable.Repeat(meth, 1) + .Concat(FindStartedTasks(meth).Select(stack => stack.Last())); + + private static readonly Type strSynAttrib = typeof(StringSyntaxAttribute); + + private static bool AnyParamHasStringSyntaxAttribute(MethodDefinition md) + => AnyParamHasAttribute(md, strSynAttrib); + + private static bool AnyParamHasAttribute(MethodDefinition md, Type attrib) + => md.Parameters.SelectMany(p => p.CustomAttributes) + .Any(attr => attr.AttributeType.Namespace == attrib.Namespace + && attr.AttributeType.Name == attrib.Name); + } +} diff --git a/Tests/Tests.csproj b/Tests/Tests.csproj index 6b3d06593b..8ed9286771 100644 --- a/Tests/Tests.csproj +++ b/Tests/Tests.csproj @@ -20,6 +20,7 @@ 9 enable true + CS0436 IDE1006,NU1701 net48;net8.0;net8.0-windows $(TargetFramework.Replace("-windows", "")) @@ -42,6 +43,7 @@ +