Fixes after AVSS scanning

src/changelog/3.3.2/298-fix-appender-filter-chain-synchronization.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="f57d7b3-001" link="https://github.com/apache/tooling-agents/blob/main/ASVS/reports/logging-log4net/f57d7b3/issues.md#issue-finding-001---filter-chain-modification-methods-lack-synchronization-creating-potential-race-with-filterevent-under-active-logging"/>
+  <issue id="298" link="https://github.com/apache/logging-log4net/pull/298"/>
+  <description format="asciidoc">
+    fix race condition in `AppenderSkeleton.AddFilter` and `ClearFilters` under concurrent logging (CWE-362)
+  </description>
+</entry>

src/changelog/3.3.2/298-fix-appender-skeleton-finalizer-exception.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="f57d7b3-003" link="https://github.com/apache/tooling-agents/blob/main/ASVS/reports/logging-log4net/f57d7b3/issues.md#issue-finding-003---finalizer-path-lacks-exception-protection-risking-process-termination"/>
+  <issue id="298" link="https://github.com/apache/logging-log4net/pull/298"/>
+  <description format="asciidoc">
+    fix unhandled exception in `AppenderSkeleton` finalizer that could terminate the process when `OnClose()` throws (CWE-755)
+  </description>
+</entry>

src/changelog/3.3.2/298-fix-interprocesslock-mutex-leak.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="f57d7b3-002" link="https://github.com/apache/tooling-agents/blob/main/ASVS/reports/logging-log4net/f57d7b3/issues.md#issue-finding-002---interprocesslock-mutex-not-released-when-underlying-file-stream-is-null"/>
+  <issue id="298" link="https://github.com/apache/logging-log4net/pull/298"/>
+  <description format="asciidoc">
+    fix mutex leak in `InterProcessLock.AcquireLock` when the underlying file stream is null (CWE-772)
+  </description>
+</entry>

src/log4net.Tests/Appender/AppenderSkeletonTest.cs

@@ -0,0 +1,88 @@
+#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 log4net.Filter;
+using log4net.Appender;
+using NUnit.Framework;
+
+namespace log4net.Tests.Appender;
+
+/// <summary>
+/// Unit tests for <see cref="AppenderSkeleton"/> filter chain management.
+/// </summary>
+[TestFixture]
+public sealed class AppenderSkeletonTest
+{
+  private CountingAppender _appender = null!;
+
+  [SetUp]
+  public void SetUp() => _appender = new CountingAppender();
+
+  [TearDown]
+  public void TearDown() => _appender.Close();
+
+  /// <summary>
+  /// Verifies that <see cref="AppenderSkeleton.AddFilter"/> sets <see cref="AppenderSkeleton.FilterHead"/> when the chain is empty.
+  /// </summary>
+  [Test]
+  public void AddFilter_FirstFilter_SetsFilterHead()
+  {
+    DenyAllFilter filter = new();
+    _appender.AddFilter(filter);
+    Assert.That(_appender.FilterHead, Is.SameAs(filter));
+  }
+
+  /// <summary>
+  /// Verifies that a second <see cref="AppenderSkeleton.AddFilter"/> call appends the filter via <see cref="Filter.IFilter.Next"/>.
+  /// </summary>
+  [Test]
+  public void AddFilter_SecondFilter_LinksToChain()
+  {
+    DenyAllFilter first = new();
+    DenyAllFilter second = new();
+    _appender.AddFilter(first);
+    _appender.AddFilter(second);
+    Assert.That(_appender.FilterHead, Is.SameAs(first));
+    Assert.That(_appender.FilterHead!.Next, Is.SameAs(second));
+  }
+
+  /// <summary>
+  /// Verifies that <see cref="AppenderSkeleton.ClearFilters"/> resets <see cref="AppenderSkeleton.FilterHead"/> to null.
+  /// </summary>
+  [Test]
+  public void ClearFilters_ResetsFilterHead()
+  {
+    _appender.AddFilter(new DenyAllFilter());
+    _appender.ClearFilters();
+    Assert.That(_appender.FilterHead, Is.Null);
+  }
+
+  /// <summary>
+  /// Verifies that filters can be added again after <see cref="AppenderSkeleton.ClearFilters"/>.
+  /// </summary>
+  [Test]
+  public void ClearFilters_AllowsAddingFiltersAgain()
+  {
+    _appender.AddFilter(new DenyAllFilter());
+    _appender.ClearFilters();
+    DenyAllFilter filter = new();
+    _appender.AddFilter(filter);
+    Assert.That(_appender.FilterHead, Is.SameAs(filter));
+  }
+}

src/log4net.Tests/Appender/FileAppenderTest.cs

@@ -29,6 +29,8 @@
 using log4net.Core;
 using System.IO;
 using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
 
 namespace log4net.Tests.Appender;
 
@@ -151,4 +153,34 @@ public void FilenameWithGlobalContextPatternStringTest()
     logs.Refresh();
     Assert.That(logs.GetFiles().Any(file => file.Name.StartsWith("file_custom_log_issue_193")));
   }
+
+  /// <summary>
+  /// Verifies that <see cref="FileAppender.InterProcessLock"/> releases the mutex
+  /// when the underlying file stream is null.
+  /// </summary>
+  [Test]
+  public void InterProcessLock_AcquireLock_ReleasesMutexWhenStreamIsNull()
+  {
+    string tempFile = Path.GetTempFileName();
+    FileAppender appender = new() { File = "log4net_ipl_test" };
+    FileAppender.InterProcessLock lockingModel = new() { CurrentAppender = appender };
+    lockingModel.ActivateOptions();
+    lockingModel.OpenFile(tempFile, false, Encoding.UTF8);
+    lockingModel.CloseFile(); // sets _stream to null; mutex remains alive
+    try
+    {
+      Stream? stream = lockingModel.AcquireLock();
+      Assert.That(stream, Is.Null);
+
+      // if the mutex was released, a second thread can acquire the lock without blocking
+      Task task = Task.Run(lockingModel.AcquireLock);
+      Assert.That(task.Wait(TimeSpan.FromSeconds(2)), Is.True,
+        "Mutex was not released by AcquireLock when stream is null");
+    }
+    finally
+    {
+      lockingModel.OnClose();
+      File.Delete(tempFile);
+    }
+  }
 }

src/log4net/Appender/AppenderSkeleton.cs

@@ -65,13 +65,21 @@ public abstract class AppenderSkeleton : IAppender, IBulkAppender, IOptionHandle
   /// </remarks>
   ~AppenderSkeleton()
   {
-    // An appender might be closed then garbage collected. 
+    // An appender might be closed then garbage collected.
     // There is no point in closing twice.
-    if (!_isClosed)
+    if (_isClosed)
+    {
+      return;
+    }
+    LogLog.Debug(_declaringType, $"Finalizing appender named [{Name}].");
+    try
     {
-      LogLog.Debug(_declaringType, $"Finalizing appender named [{Name}].");
       Close();
     }
+    catch (Exception ex) when (!ex.IsFatal())
+    {
+      LogLog.Warn(_declaringType, $"Exception during finalization of {GetType().FullName} [{Name}].", ex);
+    }
   }
 
   /// <summary>
@@ -111,6 +119,7 @@ public virtual IErrorHandler ErrorHandler
     {
       lock (LockObj)
       {
+        // ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract
         if (value is null)
         {
           // We do not throw exception here since the cause is probably a
@@ -198,8 +207,14 @@ public void Close()
     {
       if (!_isClosed)
       {
-        OnClose();
-        _isClosed = true;
+        try
+        {
+          OnClose();
+        }
+        finally
+        {
+          _isClosed = true;
+        }
       }
     }
   }
@@ -248,7 +263,7 @@ public void Close()
   public void DoAppend(LoggingEvent loggingEvent)
   {
     // This lock is absolutely critical for correct formatting
-    // of the message in a multi-threaded environment.  Without
+    // of the message in a multithreaded environment.  Without
     // this, the message may be broken up into elements from
     // multiple thread contexts (like get the wrong thread ID).
 
@@ -332,7 +347,7 @@ public void DoAppend(LoggingEvent[] loggingEvents)
     loggingEvents.EnsureNotNull();
 
     // This lock is absolutely critical for correct formatting
-    // of the message in a multi-threaded environment.  Without
+    // of the message in a multithreaded environment.  Without
     // this, the message may be broken up into elements from
     // multiple thread contexts (like get the wrong thread ID).
     lock (LockObj)
@@ -456,14 +471,17 @@ protected virtual bool FilterEvent(LoggingEvent loggingEvent)
   public virtual void AddFilter(IFilter filter)
   {
     filter.EnsureNotNull();
-    if (FilterHead is null)
-    {
-      FilterHead = _tailFilter = filter;
-    }
-    else
+    lock (LockObj)
     {
-      _tailFilter!.Next = filter;
-      _tailFilter = filter;
+      if (FilterHead is null)
+      {
+        FilterHead = _tailFilter = filter;
+      }
+      else
+      {
+        _tailFilter!.Next = filter;
+        _tailFilter = filter;
+      }
     }
   }
 
@@ -475,7 +493,13 @@ public virtual void AddFilter(IFilter filter)
   /// Clears the filter list for this appender.
   /// </para>
   /// </remarks>
-  public virtual void ClearFilters() => FilterHead = _tailFilter = null;
+  public virtual void ClearFilters()
+  {
+    lock (LockObj)
+    {
+      FilterHead = _tailFilter = null;
+    }
+  }
 
   /// <summary>
   /// Checks if the message level is below this appender's threshold.

src/log4net/Appender/FileAppender.cs

@@ -657,6 +657,9 @@ public override void CloseFile()
         else
         {
           // this can happen when the file appender cannot open a file for writing
+          _recursiveWatch--;
+          _mutex.ReleaseMutex();
+          return null;
         }
       }
       else