From 51e7d3e1a06c4089a1e92288265eed61ddefa0a2 Mon Sep 17 00:00:00 2001 From: BRUNER Patrick Date: Wed, 29 Jul 2026 13:54:26 +0200 Subject: [PATCH] fix: degrade malformed legacy XML Session Files instead of throwing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A legacy XML .lxp whose element has no child threw NullReferenceException out of PersisterXML.ReadOptions. Persister.Load calls the XML reader from inside its JSON catch block, so the NRE replaced the original exception and propagated up the synchronous file-open path, where nothing handles it. ReadOptions now skips the options section when it is absent, leaving those settings at their PersistenceData defaults. The tab, columnizer and highlightGroup elements are siblings of , not children, so they moved out of the guarded region and are still read — an early return would have cost the user their columnizer for a file that merely lacks options. The body that does need the node moved to ReadOptionsNode. ReadPersistenceDataFromNode now honours its own documented contract and returns defaults for a node that is not an element, which is what a comment among the filterTab children produces. PersisterXML.Load is documented to return null on unparseable input, but only caught malformed markup. This is a read-only fallback for a format LogExpert no longer writes, and its readers assume a well-formed document, so the catch now also covers the exceptions malformed *content* raises: FormatException, OverflowException, ArgumentException and NullReferenceException. Each is pinned by a test case. Closes #680 --- .../Classes/Persister/PersisterXML.cs | 101 ++++++++---- .../PersisterXmlMalformedTests.cs | 144 ++++++++++++++++++ .../Encodings/PersisterXmlEncodingTests.cs | 1 - 3 files changed, 217 insertions(+), 29 deletions(-) create mode 100644 src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs diff --git a/src/LogExpert.Core/Classes/Persister/PersisterXML.cs b/src/LogExpert.Core/Classes/Persister/PersisterXML.cs index 674f7159..f27285a7 100644 --- a/src/LogExpert.Core/Classes/Persister/PersisterXML.cs +++ b/src/LogExpert.Core/Classes/Persister/PersisterXML.cs @@ -235,7 +235,13 @@ private static PersistenceData LoadInternal (string fileName) private static PersistenceData ReadPersistenceDataFromNode (XmlNode node) { PersistenceData persistenceData = new(); - var fileElement = node as XmlElement; + if (node is not XmlElement fileElement) + { + // Documented above: a node that is not an element yields defaults. ReadFilterTabs walks every + // child of filterTabs, so a comment or text node in a hand-edited .lxp arrives here. + return persistenceData; + } + persistenceData.BookmarkList = ReadBookmarks(fileElement); persistenceData.RowHeightList = ReadRowHeightList(fileElement); ReadOptions(fileElement, persistenceData); @@ -447,17 +453,59 @@ private static SortedList ReadRowHeightList (XmlElement sta } /// - /// Reads configuration options from the specified XML element and populates the provided object with the extracted settings. + /// Reads the options section of a file element, plus the tab, columnizer and + /// highlightGroup elements that sit beside it, into the provided . /// - /// This method processes various configuration options such as multi-file settings, current line - /// information, filter visibility, and more. It expects the XML structure to contain specific nodes and attributes - /// that define these settings. If certain attributes are missing or invalid, default values are applied. - /// The XML element containing the configuration options to be read. + /// + /// The options section is delegated to and is optional: a hand-edited + /// or very old .lxp may omit it, in which case every setting it carries keeps its + /// default. The three sibling elements are read either way, so a missing + /// options section does not cost the user their tab name, columnizer or highlight group. + /// + /// The file element to read from. /// The object to populate with the settings extracted from the XML element. private static void ReadOptions (XmlElement startNode, PersistenceData persistenceData) { XmlNode optionsNode = startNode.SelectSingleNode("options"); + if (optionsNode != null) + { + ReadOptionsNode(optionsNode, persistenceData); + } + + // tab, columnizer and highlightGroup are siblings of options, not children, so they are read + // even when a hand-edited or very old .lxp carries no options section at all. + XmlNode tabNode = startNode.SelectSingleNode("tab"); + if (tabNode != null) + { + persistenceData.TabName = (tabNode as XmlElement).GetAttribute("name"); + } + + XmlNode columnizerNode = startNode.SelectSingleNode("columnizer"); + if (columnizerNode != null) + { + persistenceData.ColumnizerName = (columnizerNode as XmlElement).GetAttribute("name"); + } + + XmlNode highlightGroupNode = startNode.SelectSingleNode("highlightGroup"); + if (highlightGroupNode != null) + { + persistenceData.HighlightGroupName = (highlightGroupNode as XmlElement).GetAttribute("name"); + } + } + + /// + /// Reads the settings held under the options element into the given . + /// + /// + /// Boolean flags are written unconditionally, so an absent element or attribute reads as false. + /// The remaining settings keep the value they already have. Not calling this at all — which is what + /// does for a file element without an options child — therefore + /// leaves the whole group at its default, FollowTail included. + /// + /// The options element. Must not be null. + /// The object to populate. + private static void ReadOptionsNode (XmlNode optionsNode, PersistenceData persistenceData) + { var value = GetOptionsAttribute(optionsNode, "multifile", "enabled"); persistenceData.MultiFile = value != null && value.Equals("1", StringComparison.OrdinalIgnoreCase); persistenceData.MultiFilePattern = GetOptionsAttribute(optionsNode, "multifile", "pattern"); @@ -530,24 +578,6 @@ FormatException or value = GetOptionsAttribute(optionsNode, "filterSaveList", "visible"); persistenceData.FilterSaveListVisible = value != null && value.Equals("1", StringComparison.OrdinalIgnoreCase); - - XmlNode tabNode = startNode.SelectSingleNode("tab"); - if (tabNode != null) - { - persistenceData.TabName = (tabNode as XmlElement).GetAttribute("name"); - } - - XmlNode columnizerNode = startNode.SelectSingleNode("columnizer"); - if (columnizerNode != null) - { - persistenceData.ColumnizerName = (columnizerNode as XmlElement).GetAttribute("name"); - } - - XmlNode highlightGroupNode = startNode.SelectSingleNode("highlightGroup"); - if (highlightGroupNode != null) - { - persistenceData.HighlightGroupName = (highlightGroupNode as XmlElement).GetAttribute("name"); - } } /// @@ -599,8 +629,19 @@ private static string GetOptionsAttribute (XmlNode optionsNode, string elementNa /// when the file cannot be read or parsed (XML/IO/security related issues are logged). /// /// - /// Only XML format is attempted. Any , , - /// or is caught and logged; in these cases null is returned. + /// Only XML format is attempted. Unreadable files (, + /// , ) and unparseable content are caught + /// and logged; in these cases null is returned. + /// + /// "Unparseable" covers more than malformed markup: this is a read-only fallback for a format LogExpert + /// no longer writes, so the readers below assume a well-formed document and let a hand-edited or truncated + /// one throw (unparseable numbers, invalid Base64), + /// (numbers too large for their field), + /// (missing attributes, duplicate bookmark lines, a filter tab with no filter) or + /// (attribute iteration over a comment or text node that stands where + /// an element was expected). The caller reaches this synchronously from the file-open path, so every one of + /// those has to degrade to "no persistence data" rather than escape. + /// /// public static PersistenceData Load (string fileName) { @@ -611,7 +652,11 @@ public static PersistenceData Load (string fileName) catch (Exception xmlParsingException) when (xmlParsingException is XmlException or UnauthorizedAccessException or IOException or - FileNotFoundException) + FileNotFoundException or + FormatException or + OverflowException or + ArgumentException or + NullReferenceException) { _logger.Error(xmlParsingException, $"Error loading persistence data from {fileName}, unknown format, parsing xml or json was not possible"); return null; diff --git a/src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs b/src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs new file mode 100644 index 00000000..ee6aaba9 --- /dev/null +++ b/src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs @@ -0,0 +1,144 @@ +using LogExpert.Core.Classes.Persister; + +namespace LogExpert.Persister.Tests; + +/// +/// The deprecated XML .lxp format is a read-only fallback: it is only reached when a session file fails +/// to parse as JSON, and that happens on the file-open path, synchronously, with no handler above it. +/// A .lxp that is hand-edited, truncated, or written by a very old version must therefore degrade to +/// "no persistence data" instead of throwing into the open. +/// +[TestFixture] +#pragma warning disable CS0618 // PersisterXML is the deprecated fallback format, and still loads old .lxp files. +public class PersisterXmlMalformedTests +{ + private string _lxpFile = null!; + + [SetUp] + public void Setup () + { + _lxpFile = Path.Join(Path.GetTempPath(), $"{Guid.NewGuid()}.lxp"); + } + + [TearDown] + public void Cleanup () + { + if (File.Exists(_lxpFile)) + { + File.Delete(_lxpFile); + } + } + + [Test] + public void Load_FileElementWithoutOptions_ReturnsDefaults () + { + WriteLxp(""""""); + + var data = PersisterXML.Load(_lxpFile); + + Assert.That(data, Is.Not.Null, "a without must still load"); + Assert.Multiple(() => + { + Assert.That(data.FileName, Is.EqualTo("test.log")); + Assert.That(data.LineCount, Is.EqualTo(1)); + Assert.That(data.MultiFile, Is.False); + Assert.That(data.MultiFilePattern, Is.Null); + Assert.That(data.MultiFileMaxDays, Is.Zero); + Assert.That(data.FilterVisible, Is.False); + + // A missing means "nothing was persisted", so every option keeps its + // PersistenceData default — including the ones whose default is not the zero value. + Assert.That(data.CurrentLine, Is.EqualTo(-1)); + Assert.That(data.FollowTail, Is.True); + }); + } + + [Test] + public void Load_FileElementWithoutOptions_StillReadsSiblingsOfOptions () + { + // tab, columnizer and highlightGroup are children of , not of . A missing + // must not cost the user their columnizer and highlight group. + WriteLxp(""" + + + + + + """); + + var data = PersisterXML.Load(_lxpFile); + + Assert.That(data, Is.Not.Null); + Assert.Multiple(() => + { + Assert.That(data.TabName, Is.EqualTo("My Tab")); + Assert.That(data.ColumnizerName, Is.EqualTo("CSV Columnizer")); + Assert.That(data.HighlightGroupName, Is.EqualTo("My Group")); + }); + } + + /// + /// Load documents "null when the file cannot be read or parsed", and malformed content counts, not + /// just malformed markup. One case per exception the readers let escape, so the widened catch in + /// Load is pinned rather than merely asserted in prose. + /// + [Test] + [TestCase("""""", "FormatException")] + [TestCase("""""", "OverflowException")] + [TestCase("""1234""", "ArgumentException")] + [TestCase("""""", "NullReferenceException")] + public void Load_MalformedContent_ReturnsNull (string fileElement, string escapingException) + { + WriteLxp(fileElement); + + Assert.That(PersisterXML.Load(_lxpFile), Is.Null, $"{escapingException} escaped Load"); + } + + [Test] + public void Load_FilterTabsWithNonElementChild_SkipsIt () + { + WriteLxp(""" + + + + + """); + + var data = PersisterXML.Load(_lxpFile); + + // ReadPersistenceDataFromNode documents defaults for a node that is not an element, so the + // remark costs the reader nothing beyond the tab it is standing in for. + Assert.That(data, Is.Not.Null); + Assert.Multiple(() => + { + Assert.That(data.FileName, Is.EqualTo("test.log")); + Assert.That(data.FilterTabDataList, Is.Empty); + }); + } + + [Test] + public void PersisterLoad_FileElementWithoutOptions_DoesNotThrowOnTheFileOpenPath () + { + // Persister.Load tries JSON first and only falls back to XML from inside the catch block, so + // anything PersisterXML throws replaces the original exception and escapes into AddFileTab. + WriteLxp(""""""); + + var data = Core.Classes.Persister.Persister.Load(_lxpFile); + + Assert.That(data, Is.Not.Null); + Assert.That(data.FileName, Is.EqualTo("test.log")); + } + + private void WriteLxp (string fileElement) + { + File.WriteAllText( + _lxpFile, + $""" + + + {fileElement} + + """); + } +} +#pragma warning restore CS0618 diff --git a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs index a2a1e044..a917255f 100644 --- a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs +++ b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs @@ -73,7 +73,6 @@ public void Load_UnresolvableEncodingName_FallsBackToDefault () private void WriteLxp (string encodingElement) { - // has to be present: PersisterXML.ReadOptions dereferences it unconditionally. File.WriteAllText( _lxpFile, $"""