[Stiwa-PohG]

No description provided.

[Copilot]

Copilot review overview

🟡 Changes recommended

Per-list locks still allow concurrent RunSSL executions against shared OpenSSL state.

Review effort: Balanced
Findings: 1

Open (1)

• Serialize RunSSL processing to protect shared SSL state · New

What changed in this PR

Adds synchronization around pending OpenSSL read/write buffer lists.

Changes:

• Introduces separate read/write list locks.
• Protects queue counts and mutations.

File	Description
OpenSSLConnector.cs	Synchronizes pending buffer-list access.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Stiwa-PohG]

A possible solution in the class OpenSSLConnector

Add one connector-wide gate object, e.g. m_sslLock, and make every entry point that drives a protocol step serialize on it while keeping the per-list locks for producer AppendBuffer:

private readonly object m_sslGate = new object();

public void Write(byte[] pData, int dataLen)
{
    lock (m_sslGate)
    {
        AppendBuffer(m_pendingWriteList, new DataBuffer(pData, dataLen));
        RunSSL();
    }
}

public void ReadCompleted(byte[] pData, int dataLen)
{
    lock (m_sslGate)
    {
        AppendBuffer(m_pendingReadList, new DataBuffer(pData, dataLen));
        RunSSL();
    }
}

Because Monitor is re-entrant, same-thread nesting of RunSSL (a real possibility given the callback-driven data flow) is fine; the gate just prevents a second thread from entering the whole processing path. Keep AppendBuffer's internal list lock only for any producer that appends without driving RunSSL.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[jogibear9988]

My assessment after reviewing the diff, discussion, and caller paths at 626fe21:

The added m_sslGate addresses Copilot's original finding for the normal Write / ReadCompleted paths. Previously, locking individual LinkedList operations still allowed two threads to process buffers against the same SSL/BIO objects simultaneously. The latest commit holds one gate across enqueueing and the complete RunSSL() call, so those two paths are now serialized. The earlier review describes the version before this addition. OpenSSL requires the application to prevent concurrent use of the same SSL object (documentation).

I would still address these points before describing the connector as thread-safe:

1. Other SSL access and teardown bypass the gate. getOMSExporterSecret() (lines 505–509) calls SSL_export_keying_material without it; this is called from the legitimation/request path while the receive thread can still be processing TLS. Dispose(bool) (lines 99–109) can likewise free the SSL object while Write is active, and entry points do not reject calls after disposal. Use the same synchronization for these operations and check disposal before native access. ExpectConnect() should follow the same rule if callable after initialization. The existing client cleanup waits for the receive thread, but does not wait for an application-thread Write.

2. Monitor reentrancy does not prove nested RunSSL() is safe. It means the same thread can acquire the lock again (Microsoft documentation). A callback can therefore re-enter processing while the outer invocation still owns a dequeued buffer. For example, a custom OnDataAvailable callback that calls Write before consuming plaintext via Receive can let SendPendingData overwrite the shared m_buffer. Nested reads can also process later queued input before the outer invocation puts an unfinished buffer back. The current S7Client.OnDataAvailable consumes plaintext first, so that specific overwrite example is not its normal behavior; nevertheless, the public callback interface does not enforce this restriction. Either explicitly restrict callbacks, or use a processing flag so nested calls only enqueue work and the outer loop drains it. Disposal requested from a callback also needs to be deferred until processing has unwound.

3. Keep the locking invariant in the processing method. RunSSL() remains protected and can be called by a subclass without taking the gate. Making it private if it is not an extension point, or enforcing synchronization inside it, would prevent that bypass. With all queue access under one gate, the separate list locks are redundant in the current implementation because the queue helpers are private; keeping them is harmless, but they are not a substitute for the connector gate.

I agree with the direction of the fix. The original two-thread processing issue is addressed in the latest commit; the remaining items concern complete SSL lifetime/access protection and callback reentrancy. I recommend targeted regression tests for simultaneous write/read processing, teardown during a write, and callback re-entry. This assessment is based on code inspection; I have not run a native OpenSSL concurrency test or PLC integration test.

[jogibear9988]

Merged in dependency order with the subsequent PRs. Follow-up commit 7915bcb protects exporter access and disposal with the connector gate, prevents recursive SSL processing from callbacks, defers native cleanup until processing returns, and rejects operations after disposal. Five native OpenSSL regression tests cover callback re-entry, callback disposal, concurrent exporter/disposal calls, and use after disposal. The final integrated offline suite passes 290 tests on .NET 8 and 285 on .NET Framework 4.8; no live PLC test was run.