From 4e3ec1c69145c7fb1bfb9eb44fc999dd5153b6d2 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Tue, 1 Sep 2026 23:24:52 +0200 Subject: [PATCH 01/15] move SyslogNewLineHandling out of RemoteSyslogAppender #315 LocalSyslogAppender needs the same option, so nesting it in one of the two appenders no longer fits. Breaking for code naming RemoteSyslogAppender.SyslogNewLineHandling. Configuration binds the value by name and is unaffected. --- .../315-syslog-newline-handling-type.xml | 13 +++++ .../Appender/RemoteSyslogAppenderTest.cs | 12 ++--- src/log4net/Appender/RemoteSyslogAppender.cs | 21 -------- src/log4net/Appender/SyslogNewLineHandling.cs | 48 +++++++++++++++++++ 4 files changed, 67 insertions(+), 27 deletions(-) create mode 100644 src/changelog/3.5.0/315-syslog-newline-handling-type.xml create mode 100644 src/log4net/Appender/SyslogNewLineHandling.cs diff --git a/src/changelog/3.5.0/315-syslog-newline-handling-type.xml b/src/changelog/3.5.0/315-syslog-newline-handling-type.xml new file mode 100644 index 00000000..39ac9502 --- /dev/null +++ b/src/changelog/3.5.0/315-syslog-newline-handling-type.xml @@ -0,0 +1,13 @@ + + + + + move `SyslogNewLineHandling` out of `RemoteSyslogAppender` to `log4net.Appender`, now that + `LocalSyslogAppender` uses it too. Configuration files are unaffected, they bind the value by + name, but code naming `RemoteSyslogAppender.SyslogNewLineHandling` has to drop the prefix + (implemented by @FreeAndNil) + + diff --git a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs index 5b187822..acf1cbe7 100644 --- a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs @@ -154,7 +154,7 @@ public void RemoteSyslogTest() /// /// Test for the - /// with + /// with /// /// /// https://github.com/apache/logging-log4net/issues/274 @@ -171,7 +171,7 @@ public void RemoteSyslogNewLineHandlingEscapeTest() /// /// Test for the - /// with + /// with /// /// /// https://github.com/apache/logging-log4net/issues/274 @@ -180,7 +180,7 @@ public void RemoteSyslogNewLineHandlingEscapeTest() public void RemoteSyslogNewLineHandlingKeepTest() { List sentBytes = ExecuteAppend("Test\r\nmessage", - RemoteSyslogAppender.SyslogNewLineHandling.Keep); + SyslogNewLineHandling.Keep); // ReSharper disable once StringLiteralTypo const string expectedData = "<14>TestDomain: INFO - Test\r\nmessage"; Assert.That(sentBytes, Has.Count.EqualTo(1)); @@ -189,7 +189,7 @@ public void RemoteSyslogNewLineHandlingKeepTest() /// /// Test for the - /// with + /// with /// /// /// https://github.com/apache/logging-log4net/issues/274 @@ -198,7 +198,7 @@ public void RemoteSyslogNewLineHandlingKeepTest() public void RemoteSyslogNewLineHandlingSplitTest() { List sentBytes = ExecuteAppend("Test\r\nmessage", - RemoteSyslogAppender.SyslogNewLineHandling.Split); + SyslogNewLineHandling.Split); // ReSharper disable once StringLiteralTypo Assert.That(sentBytes, Has.Count.EqualTo(2)); const string expectedData0 = "<14>TestDomain: INFO - Test"; @@ -266,7 +266,7 @@ public void IdentityWithoutControlCharactersIsUnchangedAndNotReported() } private static List ExecuteAppend(string message, - RemoteSyslogAppender.SyslogNewLineHandling newLineHandling = default, + SyslogNewLineHandling newLineHandling = default, string? identity = null) { System.Net.IPAddress ipAddress = new([127, 0, 0, 1]); diff --git a/src/log4net/Appender/RemoteSyslogAppender.cs b/src/log4net/Appender/RemoteSyslogAppender.cs index e2d6cd30..ec9ea4a1 100644 --- a/src/log4net/Appender/RemoteSyslogAppender.cs +++ b/src/log4net/Appender/RemoteSyslogAppender.cs @@ -256,27 +256,6 @@ public enum SyslogFacility Local7 = 23 } - /// - /// Options for handling newlines (\r or \n) in - /// - public enum SyslogNewLineHandling - { - /// - /// escape the newlines (\\r for \r and \\n for \n) - /// - Escape, - - /// - /// split the message at new lines - /// - Split, - - /// - /// keep newlines as is (many syslog servers can handle newlines in the message part) - /// - Keep - } - private const int CloseTimeoutMillis = 5_000; private IUdpConnection? _connection; private BackgroundSender? _sender; diff --git a/src/log4net/Appender/SyslogNewLineHandling.cs b/src/log4net/Appender/SyslogNewLineHandling.cs new file mode 100644 index 00000000..975af25a --- /dev/null +++ b/src/log4net/Appender/SyslogNewLineHandling.cs @@ -0,0 +1,48 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +namespace log4net.Appender; + +/// +/// Options for handling the newlines (\r or \n) in logged content, used by +/// and . +/// +/// +/// +/// A newline ends the record for a syslog daemon that writes the message through to a line +/// oriented log, so content could otherwise forge a second, authentic looking entry. +/// +/// +public enum SyslogNewLineHandling +{ + /// + /// escape the newlines (\\r for \r and \\n for \n) + /// + Escape, + + /// + /// split the message at new lines + /// + Split, + + /// + /// keep newlines as is (many syslog servers can handle newlines in the message part) + /// + Keep +} From 877269352ac133a733fb4cf52862dfb4fa35f8d2 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Tue, 1 Sep 2026 23:25:43 +0200 Subject: [PATCH 02/15] escape newlines in LocalSyslogAppender content #315 A newline in logged content ends the record for a syslog daemon that writes the message through to a line oriented log, so content could forge a second entry that looks authentic. The code claimed syslog(3) escapes control characters itself. It does not: glibc formats the buffer and hands it over, and the escaping seen on a mainstream Linux is the daemon's. Measured with LOG_PERROR, an embedded newline comes out as two lines. NewLineHandling mirrors the option RemoteSyslogAppender has had all along, which already escaped by default. Keep restores the previous behaviour. The remote appender's habit of dropping non-ASCII is deliberately not copied. --- .../3.5.0/315-local-syslog-newlines.xml | 15 ++++++ .../Appender/LocalSyslogAppenderTest.cs | 45 +++++++++++++++- src/log4net/Appender/LocalSyslogAppender.cs | 53 +++++++++++++++++-- .../appenders/localsyslogappender.adoc | 7 +++ 4 files changed, 114 insertions(+), 6 deletions(-) create mode 100644 src/changelog/3.5.0/315-local-syslog-newlines.xml diff --git a/src/changelog/3.5.0/315-local-syslog-newlines.xml b/src/changelog/3.5.0/315-local-syslog-newlines.xml new file mode 100644 index 00000000..4c7a6a47 --- /dev/null +++ b/src/changelog/3.5.0/315-local-syslog-newlines.xml @@ -0,0 +1,15 @@ + + + + + escape the newlines in logged content in `LocalSyslogAppender`, which passed them to + `syslog(3)` unchanged. A daemon that writes the message through to a line oriented log then + records everything after the newline as its own entry, so content could forge an authentic + looking record (CWE-117). `NewLineHandling` mirrors the option of the same name on + `RemoteSyslogAppender`, which already escaped by default; set it to `Keep` for the previous + behaviour (audit da18b6fd-f008) + + diff --git a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs index be09b7f9..1a3627d2 100644 --- a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs @@ -55,8 +55,7 @@ public void EveryNulCharacterIsEscaped() => Assert.That(EscapeNulCharacters("a\0b\0c"), Is.EqualTo("a\\0b\\0c")); /// - /// A message without a NUL character has to come through untouched, including the newlines an - /// exception layout produces: syslog(3) deals with those itself. + /// This escape is only about NUL. Newlines are . /// [Test] public void MessagesWithoutNulCharactersAreUnchanged() @@ -114,6 +113,48 @@ private static IntPtr CurrentIdentityHandle() .GetField("_handleToIdentity", BindingFlags.Static | BindingFlags.NonPublic)! .GetValue(null)!; + /// + /// A newline ends the record for a daemon that writes the message through to a line oriented + /// log, so content could otherwise forge a second entry. glibc does not escape it. + /// + [Test] + public void NewLinesAreEscaped() + => Assert.That(EscapeNewLines("value\r\nJan 1 00:00:00 host sshd[1]: forged"), + Is.EqualTo("value\\r\\nJan 1 00:00:00 host sshd[1]: forged")); + + /// Both characters count, on their own as well as paired. + [TestCase("a\rb", "a\\rb")] + [TestCase("a\nb", "a\\nb")] + [TestCase("a\n\nb", "a\\n\\nb")] + public void EveryNewLineIsEscaped(string message, string expected) + => Assert.That(EscapeNewLines(message), Is.EqualTo(expected)); + + /// A message without newlines takes the fast path and comes through untouched. + [Test] + public void MessagesWithoutNewLinesAreUnchanged() + => Assert.That(EscapeNewLines("field=1\tfield=2"), Is.EqualTo("field=1\tfield=2")); + + /// Escaping is the default, because a daemon that splits the record is the common case. + [Test] + public void NewLineHandlingDefaultsToEscape() + => Assert.That(new LocalSyslogAppender().NewLineHandling, + Is.EqualTo(SyslogNewLineHandling.Escape)); + + /// One record per line, and a blank line is no record at all. + [Test] + public void SplittingDropsTheEmptyLines() + => Assert.That(SplitLines("first\r\nsecond\n\nthird\r"), Is.EqualTo(new[] { "first", "second", "third" })); + + private static string EscapeNewLines(string message) + => (string)typeof(LocalSyslogAppender) + .GetMethod("EscapeNewLines", BindingFlags.Static | BindingFlags.NonPublic)! + .Invoke(null, [message])!; + + private static string[] SplitLines(string message) + => (string[])typeof(LocalSyslogAppender) + .GetMethod("SplitLines", BindingFlags.Static | BindingFlags.NonPublic)! + .Invoke(null, [message])!; + private static string EscapeNulCharacters(string message) => (string)typeof(LocalSyslogAppender) .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)! diff --git a/src/log4net/Appender/LocalSyslogAppender.cs b/src/log4net/Appender/LocalSyslogAppender.cs index 6c946a1b..1711485f 100644 --- a/src/log4net/Appender/LocalSyslogAppender.cs +++ b/src/log4net/Appender/LocalSyslogAppender.cs @@ -355,11 +355,56 @@ protected override void Append(LoggingEvent loggingEvent) int priority = GeneratePriority(Facility, GetSeverity(loggingEvent.EnsureNotNull().Level)); string message = EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); - // Call the local libc syslog method - // The second argument is a printf style format string + // The second argument is a printf style format string. + if (NewLineHandling == SyslogNewLineHandling.Split) + { + foreach (string line in SplitLines(message)) + { + NativeMethods.syslog(priority, "%s", line); + } + + return; + } + + if (NewLineHandling == SyslogNewLineHandling.Escape) + { + message = EscapeNewLines(message); + } + NativeMethods.syslog(priority, "%s", message); } + /// + /// What to do with the newlines in logged content. Defaults to + /// . + /// + /// + /// A newline in content ends the record for daemons that write the message through to a line + /// oriented log, letting content forge a second, authentic looking entry. + /// + public SyslogNewLineHandling NewLineHandling { get; set; } + = SyslogNewLineHandling.Escape; + + /// + /// Replaces the newlines with a visible \r or \n escape. + /// + /// The rendered message. + /// The message with every newline escaped. + private static string EscapeNewLines(string message) + => message.IndexOf('\r') < 0 && message.IndexOf('\n') < 0 + ? message + : message.Replace("\r", "\\r").Replace("\n", "\\n"); + + /// + /// Splits the message into the lines to send as separate records, dropping the empty ones. + /// + /// The rendered message. + /// One entry per non-empty line. + private static string[] SplitLines(string message) + => message.Split(_newLines, StringSplitOptions.RemoveEmptyEntries); + + private static readonly string[] _newLines = ["\r\n", "\n", "\r"]; + /// /// Replaces NUL characters with a visible \0 escape. /// @@ -373,8 +418,8 @@ protected override void Append(LoggingEvent loggingEvent) /// contain a NUL, so the character is escaped rather than passed through. /// /// - /// Other control characters are left alone: syslog(3) encodes them itself, and newlines - /// are needed for the multi-line output an exception layout produces. + /// Newlines are handled separately, see . Other control characters + /// are passed through. /// /// private static string EscapeNulCharacters(string message) diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/localsyslogappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/localsyslogappender.adoc index 07459e96..5c5e9eea 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/localsyslogappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/localsyslogappender.adoc @@ -38,6 +38,13 @@ You can also specify: * Facility (default: user) * Identity (default: application name) +* NewLineHandling (default: Escape), one of `Escape`, `Split` or `Keep` + +A newline in logged content ends the record for a syslog daemon that writes the message through to +a line oriented log, so content can otherwise forge a second, authentic looking entry. `Escape` +writes them as `\r` and `\n`, `Split` sends one record per line, and `Keep` passes them through +for a daemon that handles multiline messages itself. Note that `syslog(3)` does no escaping of its +own: whatever escaping you see on a mainstream Linux comes from the daemon, not from libc. [source,xml] ---- From 6046fe9d7f353b49fe995361c071ec2eea8f7ef6 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Tue, 1 Sep 2026 23:52:07 +0200 Subject: [PATCH 03/15] escape NUL characters in OutputDebugStringAppender content #315 OutputDebugStringW takes a null terminated string, so a NUL in logged content ended the record there and dropped whatever the layout rendered after it. The escape LocalSyslogAppender already had is now shared by both, since EventLogAppender is the same shape and will want it too. Its tests moved onto the shared helper with it. The appender level test only runs on Windows: Append refuses to run elsewhere. --- .../3.5.0/315-outputdebugstring-nul.xml | 13 ++++++ .../Appender/LocalSyslogAppenderTest.cs | 3 +- .../Appender/OutputDebugAppenderTest.cs | 23 +++++++++++ .../Appender/Internal/NativeStringEscape.cs | 41 +++++++++++++++++++ src/log4net/Appender/LocalSyslogAppender.cs | 23 +---------- .../Appender/OutputDebugStringAppender.cs | 3 +- 6 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 src/changelog/3.5.0/315-outputdebugstring-nul.xml create mode 100644 src/log4net/Appender/Internal/NativeStringEscape.cs diff --git a/src/changelog/3.5.0/315-outputdebugstring-nul.xml b/src/changelog/3.5.0/315-outputdebugstring-nul.xml new file mode 100644 index 00000000..b2adef68 --- /dev/null +++ b/src/changelog/3.5.0/315-outputdebugstring-nul.xml @@ -0,0 +1,13 @@ + + + + + escape NUL characters in `OutputDebugStringAppender` content. `OutputDebugStringW` takes a null + terminated string, so a NUL in logged content ended the record there and silently dropped whatever + the layout rendered after it, exception text and trailing fields included (CWE-158). The escape + `LocalSyslogAppender` already applied is now shared between the two (audit da18b6fd-f009) + + diff --git a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs index 1a3627d2..bf9aee71 100644 --- a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs @@ -156,7 +156,8 @@ private static string[] SplitLines(string message) .Invoke(null, [message])!; private static string EscapeNulCharacters(string message) - => (string)typeof(LocalSyslogAppender) + => (string)typeof(LocalSyslogAppender).Assembly + .GetType("log4net.Appender.Internal.NativeStringEscape")! .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)! .Invoke(null, [message])!; } diff --git a/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs b/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs index 882b1d53..19b84a8a 100644 --- a/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs +++ b/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs @@ -58,6 +58,29 @@ public void AppendShouldNotCauseAnyErrors() log.Debug(DebugMessage); Assert.That(lastDebugString, Is.Not.Null.And.Contains(DebugMessage)); } + + /// + /// OutputDebugStringW takes a null terminated string, so a NUL in content would end the record + /// there and drop whatever the layout rendered after it. + /// + [Test] + public void NulCharactersAreEscapedBeforeTheNativeCall() + { + ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString()); + string? lastDebugString = null; + OutputAppender appender = new(value => lastDebugString = value) + { + Layout = new SimpleLayout(), + ErrorHandler = new FailOnError() + }; + appender.ActivateOptions(); + BasicConfigurator.Configure(rep, appender); + + LogManager.GetLogger(rep.Name, GetType()).Debug("before\0after"); + + Assert.That(lastDebugString, Does.Contain("before\\0after")); + Assert.That(lastDebugString, Does.Not.Contain("\0")); + } } file sealed class OutputAppender(Action outputDebugString) diff --git a/src/log4net/Appender/Internal/NativeStringEscape.cs b/src/log4net/Appender/Internal/NativeStringEscape.cs new file mode 100644 index 00000000..a4ac1a62 --- /dev/null +++ b/src/log4net/Appender/Internal/NativeStringEscape.cs @@ -0,0 +1,41 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +namespace log4net.Appender.Internal; + +/// +/// Prepares rendered content for a sink that takes a null terminated string. +/// +internal static class NativeStringEscape +{ + /// + /// Replaces NUL characters with a visible \0 escape. + /// + /// The rendered message. + /// The message with every NUL character escaped. + /// + /// + /// A NUL ends the string for the native sink, dropping everything the layout rendered after it, + /// trailing fields and exception text included. Logged content is not trusted and may well + /// contain a NUL, so the character is escaped rather than passed through. + /// + /// + internal static string EscapeNulCharacters(string message) + => message.IndexOf('\0') < 0 ? message : message.Replace("\0", "\\0"); +} diff --git a/src/log4net/Appender/LocalSyslogAppender.cs b/src/log4net/Appender/LocalSyslogAppender.cs index 1711485f..0e6f7fa9 100644 --- a/src/log4net/Appender/LocalSyslogAppender.cs +++ b/src/log4net/Appender/LocalSyslogAppender.cs @@ -20,6 +20,7 @@ using System; using System.Runtime.InteropServices; +using log4net.Appender.Internal; using log4net.Core; using log4net.Util; @@ -353,7 +354,7 @@ public override void ActivateOptions() protected override void Append(LoggingEvent loggingEvent) { int priority = GeneratePriority(Facility, GetSeverity(loggingEvent.EnsureNotNull().Level)); - string message = EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); + string message = NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); // The second argument is a printf style format string. if (NewLineHandling == SyslogNewLineHandling.Split) @@ -405,26 +406,6 @@ private static string[] SplitLines(string message) private static readonly string[] _newLines = ["\r\n", "\n", "\r"]; - /// - /// Replaces NUL characters with a visible \0 escape. - /// - /// The rendered message. - /// The message with every NUL character escaped. - /// - /// - /// The message is marshaled to libc as a null-terminated string, so a NUL character anywhere in - /// it would end the record there and silently drop everything the layout rendered after it, - /// including trailing fields and exception text. Logged content is not trusted and may well - /// contain a NUL, so the character is escaped rather than passed through. - /// - /// - /// Newlines are handled separately, see . Other control characters - /// are passed through. - /// - /// - private static string EscapeNulCharacters(string message) - => message.IndexOf('\0') < 0 ? message : message.Replace("\0", "\\0"); - /// /// Close the syslog when the appender is closed /// diff --git a/src/log4net/Appender/OutputDebugStringAppender.cs b/src/log4net/Appender/OutputDebugStringAppender.cs index 44442e9d..9c5b01b9 100644 --- a/src/log4net/Appender/OutputDebugStringAppender.cs +++ b/src/log4net/Appender/OutputDebugStringAppender.cs @@ -18,6 +18,7 @@ #endregion using System; +using log4net.Appender.Internal; using log4net.Core; using log4net.Util; @@ -59,7 +60,7 @@ protected override void Append(LoggingEvent loggingEvent) } #endif - _outputDebugString(RenderLoggingEvent(loggingEvent)); + _outputDebugString(NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent))); } /// From dc5855a0720c91590fd7a81d729ea01fdd69e000 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Tue, 1 Sep 2026 23:57:40 +0200 Subject: [PATCH 04/15] escape NUL characters in EventLogAppender content #315 ReportEventW takes a null terminated string, so a NUL in logged content ended the stored record there and dropped whatever the layout rendered after it. WriteEntry raises nothing, so the record simply stored short and no ErrorHandler call fired. Measured on Windows 11 build 26200: a 45 character message with a NUL at 23 stored as its 23 character prefix. Escaping happens before the size limit is applied, since it doubles each NUL, and PrepareEventText exists so that ordering can be tested without an event log. --- src/changelog/3.5.0/315-eventlog-nul.xml | 15 ++++++++ .../Appender/EventLogAppenderTest.cs | 37 ++++++++++++++++++- src/log4net/Appender/EventLogAppender.cs | 28 ++++++++++---- 3 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 src/changelog/3.5.0/315-eventlog-nul.xml diff --git a/src/changelog/3.5.0/315-eventlog-nul.xml b/src/changelog/3.5.0/315-eventlog-nul.xml new file mode 100644 index 00000000..2ac7a56f --- /dev/null +++ b/src/changelog/3.5.0/315-eventlog-nul.xml @@ -0,0 +1,15 @@ + + + + + escape NUL characters in `EventLogAppender` content. `ReportEventW` takes a null terminated + string, so a NUL in logged content ended the stored record there and silently dropped whatever + the layout rendered after it, exception text and trailing fields included (CWE-158). `WriteEntry` + raises nothing, so the record simply stored short. Measured on Windows 11 build 26200: of a 45 + character message with a NUL at 23, the 23 character prefix was stored and the rest was gone + (audit da18b6fd-f007) + + diff --git a/src/log4net.Tests/Appender/EventLogAppenderTest.cs b/src/log4net.Tests/Appender/EventLogAppenderTest.cs index ad0cd705..07873db5 100644 --- a/src/log4net.Tests/Appender/EventLogAppenderTest.cs +++ b/src/log4net.Tests/Appender/EventLogAppenderTest.cs @@ -21,6 +21,7 @@ #if NET462_OR_GREATER using System.Diagnostics; +using System.Reflection; using log4net.Appender; using log4net.Core; @@ -81,6 +82,40 @@ public void ActivateOptionsDisablesAppenderIfSourceDoesntExist() eventAppender.ActivateOptions(); Assert.That(eventAppender.Threshold, Is.EqualTo(Level.Off)); } + + /// + /// ReportEventW takes a null terminated string, so a NUL in content ends the stored record + /// there and silently drops whatever the layout rendered after it. Measured on Windows 11 + /// 26200: WriteEntry does not throw, and only the prefix is stored. + /// + [Test] + public void NulCharactersAreEscaped() + => Assert.That(PrepareEventText("before\0after", 100), Is.EqualTo("before\\0after")); + + /// + /// The escape doubles each NUL, so it has to happen before the limit is applied. Escaping + /// afterwards would push a message near the limit back over it. + /// + [Test] + public void EscapingHappensBeforeTheLimitIsApplied() + { + const int maxSize = 4; + + string prepared = PrepareEventText("\0\0\0", maxSize); + + Assert.That(prepared, Has.Length.EqualTo(maxSize)); + Assert.That(prepared, Does.Not.Contain("\0")); + } + + /// A message within the limit and without a NUL comes through untouched. + [Test] + public void MessagesWithinTheLimitAreUnchanged() + => Assert.That(PrepareEventText("field=1\tfield=2", 100), Is.EqualTo("field=1\tfield=2")); + + private static string PrepareEventText(string rendered, int maxSize) + => (string)typeof(EventLogAppender) + .GetMethod("PrepareEventText", BindingFlags.Static | BindingFlags.NonPublic)! + .Invoke(null, [rendered, maxSize])!; } -#endif // NET462_OR_GREATER \ No newline at end of file +#endif // NET462_OR_GREATER diff --git a/src/log4net/Appender/EventLogAppender.cs b/src/log4net/Appender/EventLogAppender.cs index 48844cd3..ea5f8894 100644 --- a/src/log4net/Appender/EventLogAppender.cs +++ b/src/log4net/Appender/EventLogAppender.cs @@ -23,6 +23,7 @@ using System.Diagnostics; using log4net.Util; +using log4net.Appender.Internal; using log4net.Core; namespace log4net.Appender; @@ -377,13 +378,7 @@ protected override void Append(LoggingEvent loggingEvent) // Write to the event log try { - string eventTxt = RenderLoggingEvent(loggingEvent); - - // There is a limit of about 32K characters for an event log message - if (eventTxt.Length > _maxEventlogMessageSize) - { - eventTxt = eventTxt.Substring(0, _maxEventlogMessageSize); - } + string eventTxt = PrepareEventText(RenderLoggingEvent(loggingEvent), _maxEventlogMessageSize); EventLogEntryType entryType = GetEntryType(loggingEvent.Level); @@ -398,6 +393,25 @@ protected override void Append(LoggingEvent loggingEvent) } } + /// + /// Escapes the NUL characters and then applies the message size limit. + /// + /// The rendered event. + /// The largest message the event log accepts. + /// The text to write. + /// + /// + /// The order matters. ReportEventW takes a null terminated string, so a NUL in content + /// ends the stored record there, and escaping doubles each NUL, so escaping after the limit was + /// applied could push the message back over it. + /// + /// + private static string PrepareEventText(string rendered, int maxSize) + { + string escaped = NativeStringEscape.EscapeNulCharacters(rendered); + return escaped.Length > maxSize ? escaped.Substring(0, maxSize) : escaped; + } + /// /// This appender requires a to be set. /// From 77717061b20d4346b6c0ce6b54643d85fb348bc7 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Wed, 2 Sep 2026 00:05:52 +0200 Subject: [PATCH 05/15] escape what RemoteSyslogAppender cannot send instead of deleting it #315 RFC 3164 allows only visible ASCII and space in the message, and everything else fell through the loop unwritten. "Schoenwetter " reached the collector as "Schnwetter ", and a tab vanished from between its neighbours, with no marker and no error. Such characters are now written as a \uXXXX escape, which stays inside the allowed range. Encoding still cannot make the message body non-ASCII, which the appender page now says. --- src/changelog/3.5.0/315-syslog-non-ascii.xml | 14 +++++++++++ .../Appender/RemoteSyslogAppenderTest.cs | 24 +++++++++++++++++++ src/log4net/Appender/RemoteSyslogAppender.cs | 7 ++++++ .../appenders/remotesyslogappender.adoc | 4 ++++ 4 files changed, 49 insertions(+) create mode 100644 src/changelog/3.5.0/315-syslog-non-ascii.xml diff --git a/src/changelog/3.5.0/315-syslog-non-ascii.xml b/src/changelog/3.5.0/315-syslog-non-ascii.xml new file mode 100644 index 00000000..e8020168 --- /dev/null +++ b/src/changelog/3.5.0/315-syslog-non-ascii.xml @@ -0,0 +1,14 @@ + + + + + escape the characters `RemoteSyslogAppender` cannot send instead of deleting them. RFC 3164 + allows only the visible ASCII characters and space, and everything else was dropped silently, so + `Schönwetter 你好` reached the collector as `Schnwetter ` and a tab disappeared + from between its neighbours. Such characters are now written as a `\uXXXX` escape, which keeps + the record inside the allowed range and readable (audit da18b6fd-f035) + + diff --git a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs index acf1cbe7..149d191b 100644 --- a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs @@ -69,6 +69,30 @@ private sealed class RecordingErrorHandler : IErrorHandler private const int FlushTimeoutMillis = 30_000; + /// + /// Content outside the RFC 3164 range used to be deleted with no marker, so a message written + /// in a non-Latin script reached the audit trail empty. + /// + [Test] + public void NonAsciiContentIsEscapedAndNotDeleted() + { + List sentBytes = ExecuteAppend("Sch\u00f6nwetter \u4f60\u597d"); + + Assert.That(sentBytes, Has.Count.EqualTo(1)); + Assert.That(Encoding.ASCII.GetString(sentBytes[0]), + Is.EqualTo(@"<14>TestDomain: INFO - Sch\u00f6nwetter \u4f60\u597d")); + } + + /// A control character other than CR or LF was dropped as well. + [Test] + public void OtherControlCharactersAreEscaped() + { + List sentBytes = ExecuteAppend("a\tb"); + + Assert.That(sentBytes, Has.Count.EqualTo(1)); + Assert.That(Encoding.ASCII.GetString(sentBytes[0]), Is.EqualTo(@"<14>TestDomain: INFO - a\u0009b")); + } + /// Bounded, so an unreachable server cannot grow the queue without limit. [Test] public void SendQueueSizeDefaultsTo500() diff --git a/src/log4net/Appender/RemoteSyslogAppender.cs b/src/log4net/Appender/RemoteSyslogAppender.cs index ec9ea4a1..00a55dab 100644 --- a/src/log4net/Appender/RemoteSyslogAppender.cs +++ b/src/log4net/Appender/RemoteSyslogAppender.cs @@ -18,6 +18,7 @@ #endregion using System; +using System.Globalization; using System.Text; using System.Threading; using log4net.Appender.Internal; @@ -484,6 +485,12 @@ protected virtual void AppendMessage(string message, ref int characterIndex, Str break; } } + else + { + // Escaped, not dropped: content is masked visibly rather than deleted. RFC 3164 allows + // only 0x20 to 0x7E here, so the escape itself stays inside that range. + builder.Append("\\u").Append(((int)c).ToString("x4", CultureInfo.InvariantCulture)); + } } } diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc index 54e712f9..9fef8942 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc @@ -45,6 +45,10 @@ You can also specify: * SendQueueSize (default: 500), how many datagrams may wait to be sent * EnqueueTimeoutMillis (default: 5000), how long a logging call waits for room in a full queue +RFC 3164 allows only the visible ASCII characters and space in the message, so anything else is +written as a `\uXXXX` escape rather than dropped: a message in a non-Latin script reaches the +collector readable instead of empty. `Encoding` therefore does not make the message body non-ASCII. + Datagrams are handed to a background thread, so a slow or unreachable syslog server does not hold up logging. Once the queue is full, a logging call waits `EnqueueTimeoutMillis` for room and the datagram is then discarded and counted, rather than growing the queue without limit. `Flush` waits From 522064b89e4ec54f4b20d6b5b7fec0cfe400a681 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Wed, 2 Sep 2026 06:23:27 +0200 Subject: [PATCH 06/15] compare ordinally when asserting no NUL survives #315 Does.Not.Contain is culture sensitive, and a culture sensitive comparison treats NUL as ignorable: it reports a match in a string that contains none. Both new escape tests therefore failed on Windows against correctly escaped output. ContainsConstraint has no comparison knob at all, so these use Contains.Substring(x).Using(StringComparison.Ordinal), negated with the ! operator Constraint defines. The EventLog test asserts the whole value instead, which is ordinal and pins the length too. This is the shape f018 reports in StringMatchFilter, which is still open. --- src/log4net.Tests/Appender/EventLogAppenderTest.cs | 5 +++-- src/log4net.Tests/Appender/OutputDebugAppenderTest.cs | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/log4net.Tests/Appender/EventLogAppenderTest.cs b/src/log4net.Tests/Appender/EventLogAppenderTest.cs index 07873db5..e5dfc293 100644 --- a/src/log4net.Tests/Appender/EventLogAppenderTest.cs +++ b/src/log4net.Tests/Appender/EventLogAppenderTest.cs @@ -103,8 +103,9 @@ public void EscapingHappensBeforeTheLimitIsApplied() string prepared = PrepareEventText("\0\0\0", maxSize); - Assert.That(prepared, Has.Length.EqualTo(maxSize)); - Assert.That(prepared, Does.Not.Contain("\0")); + // Equality is ordinal, and pins the length and the absence of a NUL in one go. Escaping the + // three NULs gives six characters, so the limit has to cut it back to four. + Assert.That(prepared, Is.EqualTo(@"\0\0")); } /// A message within the limit and without a NUL comes through untouched. diff --git a/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs b/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs index 19b84a8a..5d03f02e 100644 --- a/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs +++ b/src/log4net.Tests/Appender/OutputDebugAppenderTest.cs @@ -78,8 +78,10 @@ public void NulCharactersAreEscapedBeforeTheNativeCall() LogManager.GetLogger(rep.Name, GetType()).Debug("before\0after"); - Assert.That(lastDebugString, Does.Contain("before\\0after")); - Assert.That(lastDebugString, Does.Not.Contain("\0")); + // Ordinal throughout: a culture sensitive comparison treats NUL as ignorable, so it reports a + // match in a string that has none. + Assert.That(lastDebugString, Contains.Substring("before\\0after").Using(StringComparison.Ordinal)); + Assert.That(lastDebugString, !Contains.Substring("\0").Using(StringComparison.Ordinal)); } } From 28fbfb25678c48a8cc5bc9b94ead0dddfc39ffed Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Wed, 2 Sep 2026 22:41:04 +0200 Subject: [PATCH 07/15] compute the EventLog size limit instead of guessing it #315 The limit is a whole record budget, and the log name, the source and the machine name are spent from it one character for one. The fixed 31837 sat above the real ceiling, so log4net truncated to a size the service then discarded: the event was lost whole rather than shortened, with no exception and no record. Measured on Windows 11 build 26200 over five source name lengths and two log names with no residual: stored while message + logName + applicationName stays within 31736. One character more and nothing is stored. ApplicationName defaults to the app domain name, so the consumer's assembly name came out of the budget invisibly. A 1024 margin is held back because crossing the line is not one lost message: the write still consumes log space, and a log given about thirty of them was later found reporting a negative record count. Truncation is now reported through the error handler. There is no channel where the service records a dropped write, so that is the only signal available. --- .../3.5.0/315-eventlog-size-budget.xml | 17 +++++++ .../Appender/EventLogAppenderTest.cs | 38 ++++++++++++++++ src/log4net/Appender/EventLogAppender.cs | 45 ++++++++++++++++--- 3 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 src/changelog/3.5.0/315-eventlog-size-budget.xml diff --git a/src/changelog/3.5.0/315-eventlog-size-budget.xml b/src/changelog/3.5.0/315-eventlog-size-budget.xml new file mode 100644 index 00000000..faa41f34 --- /dev/null +++ b/src/changelog/3.5.0/315-eventlog-size-budget.xml @@ -0,0 +1,17 @@ + + + + + stop `EventLogAppender` truncating to a size the event log then discards. The limit is a whole + record budget that the log name, the source and the machine name are spent from, so the fixed + 31837 was above the real ceiling: measured on Windows 11 build 26200, a record is stored while + `message + logName + applicationName` stays within 31736 characters, and one character beyond + that the service stores nothing and reports nothing. The whole event was lost rather than + shortened, and `applicationName` defaults to the app domain name, so a consumer with a long + assembly name lost more. The limit is now computed, and a truncation is reported through the + error handler, which is the only signal available (audit da18b6fd-f030) + + diff --git a/src/log4net.Tests/Appender/EventLogAppenderTest.cs b/src/log4net.Tests/Appender/EventLogAppenderTest.cs index e5dfc293..9abfc4b7 100644 --- a/src/log4net.Tests/Appender/EventLogAppenderTest.cs +++ b/src/log4net.Tests/Appender/EventLogAppenderTest.cs @@ -117,6 +117,44 @@ private static string PrepareEventText(string rendered, int maxSize) => (string)typeof(EventLogAppender) .GetMethod("PrepareEventText", BindingFlags.Static | BindingFlags.NonPublic)! .Invoke(null, [rendered, maxSize])!; + + /// + /// The limit is a whole record budget: the source is spent from it one character for one, so a + /// longer ApplicationName has to leave less room for the message. + /// + [Test] + public void TheSourceNameIsSpentFromTheMessageBudget() + { + const int difference = 44; + int shortSource = GetMaxMessageSize(new() { LogName = "Application", ApplicationName = "abc" }); + int longSource = GetMaxMessageSize(new() { LogName = "Application", ApplicationName = new('a', 3 + difference) }); + + Assert.That(shortSource - longSource, Is.EqualTo(difference)); + } + + /// + /// And so is the log name, which is the half of the budget that was not expected. + /// + [Test] + public void TheLogNameIsSpentFromTheMessageBudgetToo() + { + const int difference = 7; + int shortLog = GetMaxMessageSize(new() { LogName = "Application", ApplicationName = "abc" }); + int longLog = GetMaxMessageSize(new() { LogName = new('L', 11 + difference), ApplicationName = "abc" }); + + Assert.That(shortLog - longLog, Is.EqualTo(difference)); + } + + /// Names long enough to exhaust the budget must not produce a negative length. + [Test] + public void TheLimitNeverGoesBelowZero() + => Assert.That(GetMaxMessageSize(new() { LogName = new('L', 40000), ApplicationName = "abc" }), + Is.EqualTo(0)); + + private static int GetMaxMessageSize(EventLogAppender appender) + => (int)typeof(EventLogAppender) + .GetMethod("GetMaxMessageSize", BindingFlags.Instance | BindingFlags.NonPublic)! + .Invoke(appender, [])!; } #endif // NET462_OR_GREATER diff --git a/src/log4net/Appender/EventLogAppender.cs b/src/log4net/Appender/EventLogAppender.cs index ea5f8894..ed54c855 100644 --- a/src/log4net/Appender/EventLogAppender.cs +++ b/src/log4net/Appender/EventLogAppender.cs @@ -378,7 +378,16 @@ protected override void Append(LoggingEvent loggingEvent) // Write to the event log try { - string eventTxt = PrepareEventText(RenderLoggingEvent(loggingEvent), _maxEventlogMessageSize); + string escaped = NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); + int maxSize = GetMaxMessageSize(); + string eventTxt = PrepareEventText(escaped, maxSize); + if (eventTxt.Length < escaped.Length) + { + // The only signal there is: the service reports neither a truncated nor a dropped record. + ErrorHandler.Error( + $"Truncated a logging event from {escaped.Length} to {maxSize} characters for log [{LogName}] " + + $"using source [{ApplicationName}]. What the layout rendered after that is not in the record."); + } EventLogEntryType entryType = GetEntryType(loggingEvent.Level); @@ -412,6 +421,26 @@ private static string PrepareEventText(string rendered, int maxSize) return escaped.Length > maxSize ? escaped.Substring(0, maxSize) : escaped; } + /// + /// The largest message this appender may hand to the event log. + /// + /// What is left of the record budget once the names are spent from it. + /// + /// Computed, not a constant: defaults to the app domain name, so + /// the consumer's assembly name comes out of the budget. The machine name is subtracted on the + /// assumption that it counts, which cannot be tested without renaming a machine. + /// + private int GetMaxMessageSize() + { + string machineName = MachineName == "." ? Environment.MachineName : MachineName; + int budget = _maxEventlogMessageSize + - LogName.Length + - ApplicationName.Length + - machineName.Length + - MaxEventlogMessageSizeMargin; + return Math.Max(budget, 0); + } + /// /// This appender requires a to be set. /// @@ -527,12 +556,16 @@ public class Level2EventLogEntryType : LevelMappingEntry /// Going over this size may succeed a few times but the buffer will overrun and /// eventually corrupt the log (based on testing). /// - /// The maxEventMsgSize size is based on the max buffer size of the lpStrings parameter of the ReportEvent API. - /// The documented max size for EventLog.WriteEntry for Windows Vista and higher is 31839, but I'm leaving room for a - /// terminator of #0#0, as we cannot see the source of ReportEvent (though we could use an API monitor to examine the - /// buffer, given enough time). + /// Measured on Windows 11 build 26200: a record is stored while message plus log name plus + /// source stays within 31736 characters, and one character more stores nothing at all. /// - private const int MaxEventlogMessageSizeVistaOrNewer = 31839 - 2; + private const int MaxEventlogMessageSizeVistaOrNewer = 31736; + + /// + /// Held back from the computed limit. Crossing it discards the record silently, consumes the + /// log's space anyway, and has been seen to leave the log unreadable. + /// + private const int MaxEventlogMessageSizeMargin = 1024; /// /// The maximum size that the operating system supports for From 2998f34044dcc0100fa65035e013b2a58dcf44d8 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Thu, 3 Sep 2026 00:11:12 +0200 Subject: [PATCH 08/15] share the content escapes in ContentEscape #315 The helper held only the NUL escape. It now also escapes unpaired surrogates, which f013 needs and f011 will, so the name no longer fitted. --- .../Appender/ContentEscapeTest.cs | 73 +++++++++++++ .../Appender/LocalSyslogAppenderTest.cs | 2 +- src/log4net/Appender/EventLogAppender.cs | 4 +- .../Appender/Internal/ContentEscape.cs | 103 ++++++++++++++++++ .../Appender/Internal/NativeStringEscape.cs | 41 ------- src/log4net/Appender/LocalSyslogAppender.cs | 2 +- .../Appender/OutputDebugStringAppender.cs | 2 +- 7 files changed, 181 insertions(+), 46 deletions(-) create mode 100644 src/log4net.Tests/Appender/ContentEscapeTest.cs create mode 100644 src/log4net/Appender/Internal/ContentEscape.cs delete mode 100644 src/log4net/Appender/Internal/NativeStringEscape.cs diff --git a/src/log4net.Tests/Appender/ContentEscapeTest.cs b/src/log4net.Tests/Appender/ContentEscapeTest.cs new file mode 100644 index 00000000..1935dfc5 --- /dev/null +++ b/src/log4net.Tests/Appender/ContentEscapeTest.cs @@ -0,0 +1,73 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System.Reflection; + +using log4net.Appender; + +using NUnit.Framework; + +namespace log4net.Tests.Appender; + +/// +/// Tests for the internal ContentEscape helper. +/// +[TestFixture] +public class ContentEscapeTest +{ + /// + /// An unpaired surrogate cannot be encoded, and an encoder that throws costs the event. The + /// input is built here rather than in the attribute: an attribute argument lives in metadata as + /// UTF-8, so the compiler would replace the surrogate with U+FFFD before the test ran. + /// + [TestCase(0xd800)] + [TestCase(0xdbff)] + [TestCase(0xdc00)] + [TestCase(0xdfff)] + public void UnpairedSurrogatesAreEscaped(int surrogate) + { + string input = "before" + (char)surrogate + "after"; + + Assert.That(EscapeUnpairedSurrogates(input), Is.EqualTo($@"before\u{surrogate:x4}after")); + } + + /// Every one of them, not just the first. + [Test] + public void EveryUnpairedSurrogateIsEscaped() + => Assert.That(EscapeUnpairedSurrogates("a" + (char)0xd800 + "b" + (char)0xdc00 + "c"), + Is.EqualTo(@"a\ud800b\udc00c")); + + /// A valid pair is one character and must survive untouched. + [Test] + public void ValidSurrogatePairsAreLeftAlone() + => Assert.That(EscapeUnpairedSurrogates("emoji \U0001F600 here"), Is.EqualTo("emoji \U0001F600 here")); + + /// The common case takes a fast path that must not alter anything. + [TestCase("")] + [TestCase("plain ascii")] + [TestCase("Schönwetter 你好")] + public void MessagesWithoutSurrogatesAreUnchanged(string message) + => Assert.That(EscapeUnpairedSurrogates(message), Is.EqualTo(message)); + + private static string EscapeUnpairedSurrogates(string message) + => (string)typeof(TelnetAppender).Assembly + .GetType("log4net.Appender.Internal.ContentEscape")! + .GetMethod("EscapeUnpairedSurrogates", BindingFlags.Static | BindingFlags.NonPublic)! + .Invoke(null, [message])!; +} diff --git a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs index bf9aee71..cd921030 100644 --- a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs @@ -157,7 +157,7 @@ private static string[] SplitLines(string message) private static string EscapeNulCharacters(string message) => (string)typeof(LocalSyslogAppender).Assembly - .GetType("log4net.Appender.Internal.NativeStringEscape")! + .GetType("log4net.Appender.Internal.ContentEscape")! .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)! .Invoke(null, [message])!; } diff --git a/src/log4net/Appender/EventLogAppender.cs b/src/log4net/Appender/EventLogAppender.cs index ed54c855..fa0a23d5 100644 --- a/src/log4net/Appender/EventLogAppender.cs +++ b/src/log4net/Appender/EventLogAppender.cs @@ -378,7 +378,7 @@ protected override void Append(LoggingEvent loggingEvent) // Write to the event log try { - string escaped = NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); + string escaped = ContentEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); int maxSize = GetMaxMessageSize(); string eventTxt = PrepareEventText(escaped, maxSize); if (eventTxt.Length < escaped.Length) @@ -417,7 +417,7 @@ protected override void Append(LoggingEvent loggingEvent) /// private static string PrepareEventText(string rendered, int maxSize) { - string escaped = NativeStringEscape.EscapeNulCharacters(rendered); + string escaped = ContentEscape.EscapeNulCharacters(rendered); return escaped.Length > maxSize ? escaped.Substring(0, maxSize) : escaped; } diff --git a/src/log4net/Appender/Internal/ContentEscape.cs b/src/log4net/Appender/Internal/ContentEscape.cs new file mode 100644 index 00000000..05021f8d --- /dev/null +++ b/src/log4net/Appender/Internal/ContentEscape.cs @@ -0,0 +1,103 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System.Globalization; +using System.Text; + +namespace log4net.Appender.Internal; + +/// +/// Makes rendered content safe for a sink, without discarding any of it. +/// +internal static class ContentEscape +{ + /// + /// Replaces NUL characters with a visible \0 escape. + /// + /// The rendered message. + /// The message with every NUL character escaped. + /// + /// + /// A NUL ends the string for the native sink, dropping everything the layout rendered after it, + /// trailing fields and exception text included. Logged content is not trusted and may well + /// contain a NUL, so the character is escaped rather than passed through. + /// + /// + internal static string EscapeNulCharacters(string message) + => message.IndexOf('\0') < 0 ? message : message.Replace("\0", "\\0"); + + /// + /// Replaces the surrogates that are not part of a pair with a visible \uXXXX escape. + /// + /// The rendered message. + /// The message with every unpaired surrogate escaped. + /// + /// An unpaired surrogate is a legal but cannot be encoded, and an encoder + /// that throws costs the whole event, so it is escaped rather than left to fail. + /// + internal static string EscapeUnpairedSurrogates(string message) + { + if (!ContainsUnpairedSurrogate(message)) + { + return message; + } + + StringBuilder builder = new(message.Length); + for (int i = 0; i < message.Length; i++) + { + char c = message[i]; + if (char.IsHighSurrogate(c) && i + 1 < message.Length && char.IsLowSurrogate(message[i + 1])) + { + builder.Append(c).Append(message[i + 1]); + i++; + } + else if (char.IsSurrogate(c)) + { + builder.Append("\\u").Append(((int)c).ToString("x4", CultureInfo.InvariantCulture)); + } + else + { + builder.Append(c); + } + } + + return builder.ToString(); + } + + private static bool ContainsUnpairedSurrogate(string message) + { + for (int i = 0; i < message.Length; i++) + { + if (!char.IsSurrogate(message[i])) + { + continue; + } + + if (char.IsHighSurrogate(message[i]) && i + 1 < message.Length && char.IsLowSurrogate(message[i + 1])) + { + i++; + continue; + } + + return true; + } + + return false; + } +} diff --git a/src/log4net/Appender/Internal/NativeStringEscape.cs b/src/log4net/Appender/Internal/NativeStringEscape.cs deleted file mode 100644 index a4ac1a62..00000000 --- a/src/log4net/Appender/Internal/NativeStringEscape.cs +++ /dev/null @@ -1,41 +0,0 @@ -#region Apache License -// -// Licensed to the Apache Software Foundation (ASF) under one or more -// contributor license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright ownership. -// The ASF licenses this file to you under the Apache License, Version 2.0 -// (the "License"); you may not use this file except in compliance with -// the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -#endregion - -namespace log4net.Appender.Internal; - -/// -/// Prepares rendered content for a sink that takes a null terminated string. -/// -internal static class NativeStringEscape -{ - /// - /// Replaces NUL characters with a visible \0 escape. - /// - /// The rendered message. - /// The message with every NUL character escaped. - /// - /// - /// A NUL ends the string for the native sink, dropping everything the layout rendered after it, - /// trailing fields and exception text included. Logged content is not trusted and may well - /// contain a NUL, so the character is escaped rather than passed through. - /// - /// - internal static string EscapeNulCharacters(string message) - => message.IndexOf('\0') < 0 ? message : message.Replace("\0", "\\0"); -} diff --git a/src/log4net/Appender/LocalSyslogAppender.cs b/src/log4net/Appender/LocalSyslogAppender.cs index 0e6f7fa9..dae7f505 100644 --- a/src/log4net/Appender/LocalSyslogAppender.cs +++ b/src/log4net/Appender/LocalSyslogAppender.cs @@ -354,7 +354,7 @@ public override void ActivateOptions() protected override void Append(LoggingEvent loggingEvent) { int priority = GeneratePriority(Facility, GetSeverity(loggingEvent.EnsureNotNull().Level)); - string message = NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); + string message = ContentEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)); // The second argument is a printf style format string. if (NewLineHandling == SyslogNewLineHandling.Split) diff --git a/src/log4net/Appender/OutputDebugStringAppender.cs b/src/log4net/Appender/OutputDebugStringAppender.cs index 9c5b01b9..7a6b0617 100644 --- a/src/log4net/Appender/OutputDebugStringAppender.cs +++ b/src/log4net/Appender/OutputDebugStringAppender.cs @@ -60,7 +60,7 @@ protected override void Append(LoggingEvent loggingEvent) } #endif - _outputDebugString(NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent))); + _outputDebugString(ContentEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent))); } /// From e3edfab8bc199870d5861d3e2da1a9e17dbf1dd5 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Thu, 3 Sep 2026 00:11:23 +0200 Subject: [PATCH 09/15] escape what the Telnet writer cannot encode #315 The default writer encoding threw on an unpaired surrogate, and Send reads a throw as a client that hung up, so one event reached nobody and disconnected everybody. Escaped as \uXXXX now; the non-throwing encoding stays as belt and braces. --- .../3.5.0/315-telnet-unencodable-content.xml | 13 +++ .../Appender/TelnetAppenderTest.cs | 81 +++++++++++++++++++ src/log4net/Appender/TelnetAppender.cs | 8 +- 3 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 src/changelog/3.5.0/315-telnet-unencodable-content.xml diff --git a/src/changelog/3.5.0/315-telnet-unencodable-content.xml b/src/changelog/3.5.0/315-telnet-unencodable-content.xml new file mode 100644 index 00000000..ad96651f --- /dev/null +++ b/src/changelog/3.5.0/315-telnet-unencodable-content.xml @@ -0,0 +1,13 @@ + + + + + stop one logging event disconnecting every `TelnetAppender` client. The default writer encoding + throws on content it cannot encode, such as an unpaired surrogate, and `Send` reads any failure as + a client that hung up. Unpaired surrogates are now written as a `\uXXXX` escape, as elsewhere + (audit da18b6fd-f013) + + diff --git a/src/log4net.Tests/Appender/TelnetAppenderTest.cs b/src/log4net.Tests/Appender/TelnetAppenderTest.cs index 5d8f37cd..c0df92b4 100644 --- a/src/log4net.Tests/Appender/TelnetAppenderTest.cs +++ b/src/log4net.Tests/Appender/TelnetAppenderTest.cs @@ -47,6 +47,87 @@ public sealed class TelnetAppenderTest /// https://github.com/apache/logging-log4net/issues/194 /// https://stackoverflow.com/questions/79053363/log4net-telnetappender-doesnt-work-after-migrate-to-log4net-3 /// + /// + /// An unpaired surrogate used to throw while encoding, and Send reads any throw as a client + /// that hung up, so one such event reached nobody and disconnected everybody. + /// + [Test] + public void ContentThatCannotBeEncodedDoesNotDisconnectTheClient() + { + StringBuilder received = new(); + object receivedSyncRoot = new(); + + int port = FindFreeTcpPort(); + XmlDocument log4NetConfig = new(); + log4NetConfig.LoadXml( + $""" + + + + + + + + + + + + + """); + string marker = Guid.NewGuid().ToString(); + ILoggerRepository repository = LogManager.CreateRepository(marker); + XmlConfigurator.Configure(repository, log4NetConfig["log4net"]!); + try + { + using (SimpleTelnetClient telnetClient = new(Received, port)) + { + telnetClient.Run(TestContext.Out.WriteLine); + WaitFor("welcome message", WelcomeMessage); + + ILogger logger = repository.GetLogger("Telnet"); + logger.Log(typeof(TelnetAppenderTest), Level.Info, "poison\ud800event", null); + // The event after it only arrives if the client survived the one before. + logger.Log(typeof(TelnetAppenderTest), Level.Info, marker, null); + WaitFor("the event after the unencodable one", marker); + } + } + finally + { + repository.Shutdown(); + } + + Assert.That(ReceivedText(), Does.Contain(@"poison\ud800event")); + + void Received(string message) + { + lock (receivedSyncRoot) + { + received.Append(message); + } + } + + string ReceivedText() + { + lock (receivedSyncRoot) + { + return received.ToString(); + } + } + + void WaitFor(string what, string expected) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (ReceivedText().IndexOf(expected, StringComparison.Ordinal) < 0) + { + if (stopwatch.Elapsed > _receiveTimeout) + { + Assert.Fail($"Timeout waiting for {what} - received so far: '{ReceivedText()}'"); + } + Thread.Sleep(20); + } + } + } + /// /// Maximum time to wait for a message to arrive at the client. /// diff --git a/src/log4net/Appender/TelnetAppender.cs b/src/log4net/Appender/TelnetAppender.cs index a0b04e13..903cd0d5 100644 --- a/src/log4net/Appender/TelnetAppender.cs +++ b/src/log4net/Appender/TelnetAppender.cs @@ -21,8 +21,10 @@ using System.Collections.Generic; using System.Net; using System.Net.Sockets; +using System.Text; using System.IO; using System.Linq; +using log4net.Appender.Internal; using log4net.Core; using log4net.Util; @@ -245,7 +247,9 @@ public SocketClient(Socket socket) _socket = socket; try { - _writer = new(new NetworkStream(socket)); + // Belt and braces. Send escapes what cannot be encoded; this keeps a future gap costing + // one character rather than every client, since Send reads a throw as a hung up client. + _writer = new(new NetworkStream(socket), new UTF8Encoding(false)); } catch (Exception e) when (!e.IsFatal()) { @@ -260,7 +264,7 @@ public SocketClient(Socket socket) /// string to send public void Send(string message) { - _writer.Write(message); + _writer.Write(ContentEscape.EscapeUnpairedSurrogates(message.EnsureNotNull())); _writer.Flush(); } From 71743377e975a9bcf27c6214c80afabf93213244 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Thu, 3 Sep 2026 00:11:23 +0200 Subject: [PATCH 10/15] record the escaping rule in CLAUDE.md Four appenders have needed the same two escapes. New ones belong in ContentEscape, and escaping comes before any length limit, not after. --- CLAUDE.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 09b3b901..bba164a2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -229,9 +229,15 @@ What that leaves for this file is where the answers live in the code: at the site with a link to the model rather than changing the code. `XmlConfigurator` and `XmlHierarchyConfigurator` carry these for the configuration-is-trusted paths, and `SystemStringFormat` for the format string. -- `LocalSyslogAppender.EscapeNulCharacters` and `RemoteSyslogAppender.ValidateIdentity` are the two +- `log4net.Appender.Internal.ContentEscape` and `RemoteSyslogAppender.ValidateIdentity` are the two sides of the content and structural-identifier rule: content is escaped and never rejected, a malformed identifier is reported rather than quietly repaired. +- **A sink that cannot carry a character escapes it visibly, and never drops the character, the + rest of the record, or the event.** The escapes already in use are `\0` for NUL, `\r` and `\n` + for newlines, and `\uXXXX` for anything else, in `ContentEscape` and in + `RemoteSyslogAppender.AppendMessage`. Put new ones in `ContentEscape` rather than in the + appender: four appenders have needed the same two so far. Escaping before a length limit, not + after, since an escape is longer than what it replaces. - Deliberate secure-default choices belong in the changelog with their opt-out named, so that an upgrade surprise is searchable. See the entries for `SendTimeoutMillis`, `MatchTimeoutMillis` and `LockTimeoutMillis`. From 4d2e10f0908199604b4326f9df6d0b43b871e333 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Thu, 3 Sep 2026 00:19:58 +0200 Subject: [PATCH 11/15] escape what the pickup mail writer cannot encode #315 File.CreateText throws on an unpaired surrogate, which abandoned the whole buffered batch and left a truncated mail for the pickup service to send. Reverting the fix leaves the test with a file that exists and is empty. Writing under the final name stays as it was, with a note why. --- .../315-pickup-dir-unencodable-content.xml | 13 ++++++++++ .../Appender/SmtpPickupDirAppenderTest.cs | 23 +++++++++++++++++ src/log4net/Appender/SmtpPickupDirAppender.cs | 25 ++++++++++++------- 3 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml diff --git a/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml b/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml new file mode 100644 index 00000000..8f952bf1 --- /dev/null +++ b/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml @@ -0,0 +1,13 @@ + + + + + stop one logging event destroying a whole `SmtpPickupDirAppender` batch. `File.CreateText` + throws on content it cannot encode, such as an unpaired surrogate, which abandoned every buffered + event and left a truncated mail in the pickup directory for the service to send. Such content is + now written as a `\uXXXX` escape (audit da18b6fd-f011) + + diff --git a/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs b/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs index 509b42a9..5ba88f03 100644 --- a/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs +++ b/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs @@ -153,6 +153,29 @@ private static void DestroyLogger() LoggerManager.RepositorySelector = new DefaultRepositorySelector(typeof(log4net.Repository.Hierarchy.Hierarchy)); } + /// + /// An unpaired surrogate used to abort the write, losing every buffered event with it and + /// leaving a truncated mail behind for the pickup service to send. + /// + [Test] + public void ContentThatCannotBeEncodedDoesNotDestroyTheBatch() + { + SilentErrorHandler sh = new(); + SmtpPickupDirAppender appender = CreateSmtpPickupDirAppender(sh); + ILogger log = CreateLogger(appender); + + log.Log(GetType(), Level.Info, "poison" + (char)0xd800 + "event", null); + log.Log(GetType(), Level.Info, "the event after it", null); + DestroyLogger(); + + Assert.That(Directory.GetFiles(_testPickupDir), Has.Length.EqualTo(1)); + string content = File.ReadAllText(Directory.GetFiles(_testPickupDir)[0]); + + Assert.That(content, Does.Contain(@"poison\ud800event")); + Assert.That(content, Does.Contain("the event after it")); + Assert.That(sh.Message, Is.EqualTo(string.Empty), "Unexpected error message"); + } + /// /// Tests if the sent message contained the date header. /// diff --git a/src/log4net/Appender/SmtpPickupDirAppender.cs b/src/log4net/Appender/SmtpPickupDirAppender.cs index 6cb88c80..86c17c79 100644 --- a/src/log4net/Appender/SmtpPickupDirAppender.cs +++ b/src/log4net/Appender/SmtpPickupDirAppender.cs @@ -20,6 +20,9 @@ using System; using System.IO; +using System.Globalization; +using System.Text; +using log4net.Appender.Internal; using log4net.Core; using log4net.Util; @@ -128,10 +131,15 @@ protected override void SendBuffer(LoggingEvent[] events) StreamWriter writer; // Impersonate to open the file + // Written under its final name, so a failure mid write leaves a partial mail for the pickup + // service. Accepted: no temporary name is safe for every service, and FileExtension is the + // operator's to choose. string filePath = Path.Combine(PickupDir.EnsureNotNull(), Guid.NewGuid().ToString("N") + _fileExtension); using (SecurityContext?.Impersonate(this)) { - writer = File.CreateText(filePath); + // Not File.CreateText: its encoding throws on content it cannot encode, which would + // abandon the whole batch and leave a truncated mail for the pickup service to send. + writer = new StreamWriter(filePath, false, new UTF8Encoding(false)); } using (writer) @@ -142,22 +150,21 @@ protected override void SendBuffer(LoggingEvent[] events) writer.WriteLine("Date: " + DateTime.UtcNow.ToString("r")); writer.WriteLine(); - string? t = Layout?.Header; - if (t is not null) + if (Layout?.Header is string header) { - writer.Write(t); + writer.Write(header); } for (int i = 0; i < events.Length; i++) { - // Render the event and append the text to the buffer - RenderLoggingEvent(writer, events[i]); + using StringWriter rendered = new(CultureInfo.InvariantCulture); + RenderLoggingEvent(rendered, events[i]); + writer.Write(ContentEscape.EscapeUnpairedSurrogates(rendered.ToString())); } - t = Layout?.Footer; - if (t is not null) + if (Layout?.Footer is string footer) { - writer.Write(t); + writer.Write(footer); } writer.WriteLine(); From da4b4af5def0c78ffd5f07279a678bce01431174 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Thu, 3 Sep 2026 22:31:48 +0200 Subject: [PATCH 12/15] stop the ANSI terminal appender dropping empty messages #316 - Logging an empty message threw IndexOutOfRangeException: the branch meant for a one-character message read message[0] without checking there was one. - AppenderSkeleton caught it, so the event simply disappeared. - The reset codes now go at one computed offset, leaving no short-message branch to get wrong. - All ten line-break cases are pinned by test. They carry explicit names because dotnet test --filter cannot see them otherwise; CLAUDE.md records why. audit da18b6fd-f029 --- CLAUDE.md | 6 + src/changelog/3.5.0/316-ansi-empty-render.xml | 13 +++ .../Appender/AnsiColorTerminalAppenderTest.cs | 104 ++++++++++++++++++ .../Appender/AnsiColorTerminalAppender.cs | 51 ++++----- 4 files changed, 144 insertions(+), 30 deletions(-) create mode 100644 src/changelog/3.5.0/316-ansi-empty-render.xml create mode 100644 src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs diff --git a/CLAUDE.md b/CLAUDE.md index bba164a2..0e7bcaa2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -149,6 +149,12 @@ almost always be doing. the assertion is about control characters, use `Contains.Substring(x).Using(StringComparison.Ordinal)`, negated with the `!` operator that `Constraint` defines, or assert the whole value with `Is.EqualTo`, which is ordinal. +- **Give a `[TestCase]` an explicit `TestName` when an argument holds a control character.** + Otherwise the whole fixture can become invisible to `dotnet test --filter`, silently: it is + listed by `--list-tests` and runs in a full pass, but every filter reports "No test matches". + Reproduced with `[TestCase("one", "\x1b[0m")]`; a single argument holding the same escape is + fine, so it takes two arguments and an escape character. `AnsiColorTerminalAppenderTest` names + all ten of its cases for that reason, and a filtered run there is 54 ms against 9 s for the suite. - Mark a test `[NonParallelizable]` when it mutates static state (`LogLog.InternalDebugging`, a static field on a test double, a process-wide native registration). - Wrap expected internal logging in `LogLog.ExecuteWithoutEmittingInternalMessages(...)` and capture diff --git a/src/changelog/3.5.0/316-ansi-empty-render.xml b/src/changelog/3.5.0/316-ansi-empty-render.xml new file mode 100644 index 00000000..bf01d9ca --- /dev/null +++ b/src/changelog/3.5.0/316-ansi-empty-render.xml @@ -0,0 +1,13 @@ + + + + + stop `AnsiColorTerminalAppender` dropping an event that renders to nothing. The branch meant + for a single character read the first one without checking there was one, so an empty render + threw and the event was lost. The reset codes are now placed by one computed offset, which has no + special case to get wrong (audit da18b6fd-f029) + + diff --git a/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs b/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs new file mode 100644 index 00000000..222dce26 --- /dev/null +++ b/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs @@ -0,0 +1,104 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.IO; + +using log4net.Appender; +using log4net.Core; +using log4net.Layout; + +using NUnit.Framework; + +namespace log4net.Tests.Appender; + +/// +/// Tests for , which places the terminal reset codes +/// before any trailing line break so the colour ends with the text. +/// +[TestFixture] +[NonParallelizable] +public class AnsiColorTerminalAppenderTest +{ + /// Matches the appender's private PostEventCodes. + private const string Reset = "\x1b[0m"; + + /// The reset codes belong before the line break, whichever one it is. + // Explicit names: two arguments where one holds an escape character make the whole fixture + // invisible to "dotnet test --filter", reproduced with [TestCase("one", "\x1b[0m")]. + [TestCase("", Reset, TestName = "AnEmptyRender")] + [TestCase("x", "x" + Reset, TestName = "ASingleCharacter")] + [TestCase("\n", Reset + "\n", TestName = "NothingButALineFeed")] + [TestCase("text", "text" + Reset, TestName = "NoTrailingLineBreak")] + [TestCase("\r", Reset + "\r", TestName = "NothingButACarriageReturn")] + [TestCase("text\n", "text" + Reset + "\n", TestName = "TrailingLineFeed")] + [TestCase("text\r", "text" + Reset + "\r", TestName = "TrailingCarriageReturn")] + [TestCase("text\r\n", "text" + Reset + "\r\n", TestName = "TrailingCarriageReturnLineFeed")] + [TestCase("text\n\r", "text" + Reset + "\n\r", TestName = "TrailingLineFeedCarriageReturn")] + [TestCase("text\n\n", "text\n" + Reset + "\n", TestName = "TrailingDoubledLineFeedCountsAsOne")] + public void TheResetCodesGoBeforeATrailingLineBreak(string message, string expected) + { + RecordingErrorHandler errorHandler = new(); + // Level.Info has no colour mapping configured, so nothing is prepended and the rendered + // message is exactly what was logged, down to the empty string. + AnsiColorTerminalAppender appender = new() + { + Layout = new PatternLayout("%message"), + ErrorHandler = errorHandler + }; + appender.ActivateOptions(); + + TextWriter previous = Console.Out; + using StringWriter captured = new(); + try + { + Console.SetOut(captured); + // DoAppend is overloaded on LoggingEvent and LoggingEvent[], so this new cannot be short. + appender.DoAppend(new LoggingEvent(new() + { + Level = Level.Info, + Message = message, + LoggerName = nameof(AnsiColorTerminalAppenderTest) + })); + } + finally + { + Console.SetOut(previous); + } + + Assert.That(errorHandler.Message, Is.Empty, "the event must not be dropped"); + Assert.That(captured.ToString(), Is.EqualTo(expected)); + } + + /// Collects what the appender reports, so a dropped event is visible. + private sealed class RecordingErrorHandler : IErrorHandler + { + /// Everything reported so far. + internal string Message { get; private set; } = string.Empty; + + /// + public void Error(string message) => Message += message + '\n'; + + /// + public void Error(string message, Exception e) => Message += message + '\n'; + + /// + public void Error(string message, Exception? e, ErrorCode errorCode) => Message += message + '\n'; + } +} diff --git a/src/log4net/Appender/AnsiColorTerminalAppender.cs b/src/log4net/Appender/AnsiColorTerminalAppender.cs index b685c456..1c65d027 100644 --- a/src/log4net/Appender/AnsiColorTerminalAppender.cs +++ b/src/log4net/Appender/AnsiColorTerminalAppender.cs @@ -251,36 +251,10 @@ protected override void Append(LoggingEvent loggingEvent) loggingMessage = levelColors.CombinedColor + loggingMessage; } - // on most terminals there are weird effects if we don't clear the background color - // before the new line. This checks to see if it ends with a newline, and if - // so, inserts the clear codes before the newline, otherwise the clear codes - // are inserted afterward. - if (loggingMessage.Length > 1) - { - if (loggingMessage.EndsWith("\r\n") || loggingMessage.EndsWith("\n\r")) - { - loggingMessage = loggingMessage.Insert(loggingMessage.Length - 2, PostEventCodes); - } - else if (loggingMessage.EndsWith("\n") || loggingMessage.EndsWith("\r")) - { - loggingMessage = loggingMessage.Insert(loggingMessage.Length - 1, PostEventCodes); - } - else - { - loggingMessage += PostEventCodes; - } - } - else - { - if (loggingMessage[0] is '\n' or '\r') - { - loggingMessage = PostEventCodes + loggingMessage; - } - else - { - loggingMessage += PostEventCodes; - } - } + // On most terminals there are weird effects if the background colour is not cleared before + // the line break, so the reset codes go before it rather than after. + loggingMessage = loggingMessage.Insert( + loggingMessage.Length - TrailingLineBreakLength(loggingMessage), PostEventCodes); if (_writeToErrorStream) { @@ -295,6 +269,23 @@ protected override void Append(LoggingEvent loggingEvent) } + /// + /// How many characters of line break the message ends with, 0, 1 or 2. + /// + private static int TrailingLineBreakLength(string message) + { + int last = message.Length - 1; + if (last < 0 || message[last] is not '\n' and not '\r') + { + return 0; + } + + int previous = last - 1; + return previous >= 0 && message[previous] is '\n' or '\r' && message[previous] != message[last] + ? 2 + : 1; + } + /// /// This appender requires a to be set. /// From 8b44780ad6f0b1bad9bc1a763e29ce404716ab6d Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Thu, 3 Sep 2026 23:21:08 +0200 Subject: [PATCH 13/15] stop the aspnet-request converter dropping events with rejected content #316 - Reading HttpRequest.Params validates the query string, form and cookies on first access, so a request carrying