Fixes after AVSS scanning

[FreeAndNil]

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

• Fixed: AddFilter method wrapped with lock(LockObj)
• Fixed: ClearFilters method wrapped with lock(LockObj)
• 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

• Fixed: Mutex released when _stream is null in AcquireLock()
• Fixed: _recursiveWatch properly decremented in error path
• 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:

   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

• Fixed: Finalizer wrapped with try-catch for non-fatal exceptions
• Fixed: _isClosed flag set in finally block
• 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.

[gdziadkiewicz]

LGTM