Fix crash bug while parsing settings

Fix bug where, if a setting in the config file had an invalid value (for example, a numeric setting having a value of `blah`), there might be an uncaught exception that would cause Notepad++ to crash. This bug appears to be most likely to occur when the localization is *not* set to `en-us`.
molsonkiko · Jun 23, 2024 · 64b5599 · 64b5599
1 parent 2bf4263
commit 64b5599
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 1. Fixed issue where clicking buttons on floating docking dialogs could sometimes cause Notepad++ to hang forever (see [CsvLint issue 83](https://github.com/BdR76/CSVLint/issues/83) for a detailed explanation).
 2. Fix `SCNotification` byte alignment issue in 64-bit Notepad++ by making `annotationLinesAdded` field an `IntPtr`, which has been the correct type for that field since [between Notepad++ 7.6.6 and 7.7](https://github.com/notepad-plus-plus/notepad-plus-plus/blob/37c4b894cc247d1ee6976bc1a1b66cfed4b7774e/scintilla/include/Scintilla.h#L1227). Note that *this is a potentially breaking change for 64-bit Notepad++ 7.6.6 or older*, but there's a ton of other bit rot for such old Notepad++ anyway.
 3. Fix error due to assuming that "." (the current directory according to the filesystem) will always point to the path to the Notepad++ executable; this is *almost always true*, but can be broken due to at least one known weird interaction (the one molsonkiko is familiar with concerns the `New script` functionality of the PythonScript plugin).
+4. Fix bug where, if a setting in the config file had an invalid value (for example, a numeric setting having a value of `blah`), there might be an uncaught exception that would cause Notepad++ to crash. This bug appears to be most likely to occur when the localization is *not* set to `en-us`.
 
 ## [0.0.3] - 2024-02-26
 

diff --git a/NppCSharpPluginPack/PluginInfrastructure/SettingsBase.cs b/NppCSharpPluginPack/PluginInfrastructure/SettingsBase.cs
@@ -14,6 +14,7 @@
 using NppDemo.Utils;
 using Kbg.NppPluginNET;
 using Kbg.NppPluginNET.PluginInfrastructure;
+using System.Reflection;
 
 namespace CsvQuery.PluginInfrastructure
 {
@@ -76,18 +77,21 @@ public SettingsBase(bool loadFromFile = true)
                     propertyInfo.SetValue(this, def.Value, null);
                 }
             }
-            if (loadFromFile)
-                ReadFromIniFile();
+            if (loadFromFile && !ReadFromIniFile())
+                SaveToIniFile();
         }
 
         /// <summary>
         /// Reads all (existing) settings from an ini-file
         /// </summary>
         /// <param name="filename">File to write to (default is N++ plugin config)</param>
-        public void ReadFromIniFile(string filename = null)
+        /// <returns>False if the file did not exist, or if not all values in the file were valid.<br></br>
+        /// True otherwise.</returns>
+        public bool ReadFromIniFile(string filename = null)
         {
             filename = filename ?? IniFilePath;
-            if (!File.Exists(filename)) return;
+            if (!File.Exists(filename))
+                return false;
 
             // Load all sections from file
             var loaded = GetType().GetProperties()
@@ -96,6 +100,7 @@ public void ReadFromIniFile(string filename = null)
                 .ToDictionary(section => section, section => GetKeys(filename, section));
 
             //var loaded = GetKeys(filename, "General");
+            bool allConvertedCorrectly = true;
             foreach (var propertyInfo in GetType().GetProperties())
             {
                 var category = ((CategoryAttribute)propertyInfo.GetCustomAttributes(typeof(CategoryAttribute), false).FirstOrDefault())?.Category ?? "General";
@@ -104,12 +109,54 @@ public void ReadFromIniFile(string filename = null)
                 {
                     var rawString = loaded[category][name];
                     var converter = TypeDescriptor.GetConverter(propertyInfo.PropertyType);
+                    bool convertedCorrectly = false;
+                    Exception ex = null;
                     if (converter.IsValid(rawString))
                     {
-                        propertyInfo.SetValue(this, converter.ConvertFromString(rawString), null);
+                        try
+                        {
+                            propertyInfo.SetValue(this, converter.ConvertFromString(rawString), null);
+                            convertedCorrectly = true;
+                        }
+                        catch (Exception ex_)
+                        {
+                            ex = ex_;
+                        }
+                    }
+                    if (!convertedCorrectly)
+                    {
+                        allConvertedCorrectly = false;
+                        // use the default value for the property, since the config file couldn't be read in this case.
+                        SetPropertyInfoToDefault(propertyInfo);
+                        string errorDescription = ex is null ? "could not be converted for an unknown reason." : $"raised the following error:\r\n{ex}";
+                        MessageBox.Show(
+                            $"While parsing {Main.PluginName} config file, expected setting \"{name}\" to be type {propertyInfo.PropertyType.Name}, but got an error.\r\n" +
+                            $"That setting was set to its default value of {propertyInfo.GetValue(this, null)}.\r\n" +
+                            $"The given value {rawString} {errorDescription}",
+                            $"Error while parsing {Main.PluginName} config file",
+                            MessageBoxButtons.OK,
+                            MessageBoxIcon.Error
+                        );
                     }
                 }
             }
+            return allConvertedCorrectly;
+        }
+
+        /// <summary>
+        /// if the PropertyInfo does not have a default value, return false.<br></br>
+        /// Otherwise, set the PropertyInfo's value for this object to the default value, and return true.
+        /// </summary>
+        /// <param name="propertyInfo"></param>
+        /// <returns></returns>
+        private bool SetPropertyInfoToDefault(PropertyInfo propertyInfo)
+        {
+            if (propertyInfo.GetCustomAttributes(typeof(DefaultValueAttribute), false).FirstOrDefault() is DefaultValueAttribute def)
+            {
+                propertyInfo.SetValue(this, def.Value, null);
+                return true;
+            }
+            return false;
         }
 
         /// <summary>
@@ -254,10 +301,7 @@ public void ShowDialog(bool debug = false)
                 // reset the settings to defaults
                 foreach (var propertyInfo in GetType().GetProperties())
                 {
-                    if (propertyInfo.GetCustomAttributes(typeof(DefaultValueAttribute), false).FirstOrDefault() is DefaultValueAttribute def)
-                    {
-                        propertyInfo.SetValue(this, def.Value, null);
-                    }
+                    SetPropertyInfoToDefault(propertyInfo);
                 }
                 OnSettingsChanged();
                 dialog.Close();

diff --git a/NppCSharpPluginPack/Properties/AssemblyInfo.cs b/NppCSharpPluginPack/Properties/AssemblyInfo.cs
@@ -28,5 +28,5 @@
 //      Build Number
 //      Revision
 //
-[assembly: AssemblyVersion("0.0.3.4")]
-[assembly: AssemblyFileVersion("0.0.3.4")]
+[assembly: AssemblyVersion("0.0.3.5")]
+[assembly: AssemblyFileVersion("0.0.3.5")]