Fixes after AVSS scanning #298

Fixes for 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
**Labels:** bug, security, priority:low
**Description:**

### Summary
The `AddFilter` and `ClearFilters` methods in `AppenderSkeleton.cs` lack proper synchronization, creating a race condition with `FilterEvent` during active logging operations. This can lead to inconsistent filter chain state, potentially causing filters to be skipped, `NullReferenceException`, or lost filter entries.

### Details
**CWE:** CWE-362 (Concurrent Execution using Shared Resource with Improper Synchronization)  
**ASVS:** 15.4.1 (L3)

**Data Flow:**
- `AddFilter` (no lock) → modifies `FilterHead`/`_tailFilter`/`filter.Next`
- `FilterEvent` (under `LockObj` in `DoAppend`) reads `FilterHead` and traverses `f.Next`

**Attack Vector:**
In-process code within the trust boundary calling `AddFilter`/`ClearFilters` concurrently with active logging—for example, during dynamic reconfiguration while the appender is receiving log events.

**Impact:**
Inconsistent filter chain state during traversal in `FilterEvent`, resulting in:
- Filters being skipped during evaluation
- `NullReferenceException` during chain traversal
- Lost filter entries

### Remediation
Add `lock(LockObj)` to both `AddFilter` and `ClearFilters` methods to synchronize with the `DoAppend` hot path and ensure thread-safe filter chain modifications.

### Acceptance Criteria
- [x] Fixed: `AddFilter` method wrapped with `lock(LockObj)`
- [x] Fixed: `ClearFilters` method wrapped with `lock(LockObj)`
- [x] Test added: Concurrent filter modification during active logging

### References
- File: `src/log4net/Appender/AppenderSkeleton.cs`
- Source Report: 15.4.1.md

### Priority
**Low** - Requires in-process code with concurrent reconfiguration during active logging. Limited to availability/integrity impact within the logging subsystem.

---
## Issue: FINDING-002 - InterProcessLock Mutex Not Released When Underlying File Stream Is Null
**Labels:** bug, security, priority:low
**Description:**

### Summary
When `InterProcessLock.AcquireLock()` is called and the underlying `_stream` is null (due to a prior file open failure), the named Mutex is acquired but never released. This causes a resource leak that blocks other processes attempting to use InterProcessLock on the same file, potentially leading to deadlock or resource exhaustion.

### Details
**CWE:** CWE-772 (Missing Release of Resource after Effective Lifetime)  
**ASVS:** 1.4.3 (L2)

**Data Flow:**
1. `InterProcessLock.AcquireLock()` called with `_stream == null`
2. `_mutex.WaitOne()` acquires the named Mutex
3. `_recursiveWatch` is incremented
4. Method returns null without releasing the mutex
5. Caller (`FileAppender.Append`) does not enter try/finally block
6. `ReleaseLock()` is never called
7. Named system Mutex remains held indefinitely

**Attack Vector:**
Not directly exploitable by external attackers. Requires environmental file open failure (e.g., permissions, disk full, file locked by another process).

**Impact:**
- Named Mutex remains held indefinitely
- Other processes using InterProcessLock on the same file are blocked
- Potential deadlock across processes
- Resource exhaustion if multiple locks are leaked

### Remediation
Release the named Mutex immediately when `AcquireLock()` detects that `_stream` is null:
1. Decrement `_recursiveWatch`
2. Call `_mutex.ReleaseMutex()`
3. Return null

Ensure all code paths that acquire the mutex properly release it, even in error conditions.

### Acceptance Criteria
- [x] Fixed: Mutex released when `_stream` is null in `AcquireLock()`
- [x] Fixed: `_recursiveWatch` properly decremented in error path
- [x] Test added: Verify mutex released when file stream is null

### References
- File: `src/log4net/Appender/FileAppender.cs`
- Source Report: 1.4.3.md

### Priority
**Low** - Requires environmental file system failure. Impact limited to inter-process synchronization and resource exhaustion within logging subsystem.

---
## Issue: FINDING-003 - Finalizer path lacks exception protection, risking process termination
**Labels:** bug, security, priority:low
**Description:**

### Summary
The `~AppenderSkeleton()` finalizer calls `Close()` which in turn calls `OnClose()` without exception protection. An unhandled exception on the finalizer thread will terminate the entire process in .NET Framework 2.0+ and .NET Core/5+.

### Details
**CWE:** CWE-755 (Improper Handling of Exceptional Conditions)  
**ASVS:** 16.5.4 (L3)

**Data Flow:**
GC finalizer thread → `~AppenderSkeleton()` → `Close()` → `OnClose()` (subclass implementation) → unhandled exception → **process termination**

**Attack Vector:**
If a subclass implementation of `OnClose()` throws an unhandled exception during finalization (e.g., due to resource cleanup failure, network timeout, or malformed state), the finalizer thread will propagate the exception and terminate the entire application process.

**Impact:**
- Complete application/service termination
- Denial of service
- Loss of in-flight data
- Ungraceful shutdown without proper cleanup

### Remediation
1. Wrap the finalizer's call to `Close()` in a try-catch block:
   ```csharp
   catch (Exception ex) when (!ex.IsFatal())
   {
       // Log if possible, otherwise suppress
   }
   ```
2. Consider protecting `Close()` itself with exception handling
3. Ensure `_isClosed` is set in a finally block to prevent repeated finalization attempts

### Acceptance Criteria
- [x] Fixed: Finalizer wrapped with try-catch for non-fatal exceptions
- [x] Fixed: `_isClosed` flag set in finally block
- [x] Code review: Verify fatal exceptions (OutOfMemoryException, StackOverflowException) are not caught

### References
- File: `src/log4net/Appender/AppenderSkeleton.cs`
- Source Report: 16.5.4.md

### Priority
**Low** - Requires specific failure conditions during finalization. However, impact is severe (process termination) when triggered. Recommend prioritizing fix despite low likelihood.

Author: FreeAndNil

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