diff --git a/code-reviews/Server/findings.md b/code-reviews/Server/findings.md index a79fd6b..daf5860 100644 --- a/code-reviews/Server/findings.md +++ b/code-reviews/Server/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 15 | +| Open findings | 14 | ## Checklist coverage @@ -32,13 +32,13 @@ | Severity | Critical | | Category | Correctness & logic bugs | | Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:1791` | -| Status | Open | +| Status | Resolved | **Description:** `WriteNodeIdUnknown` calls itself unconditionally as its first statement, then sets `errors[i]`. Unbounded recursion with no base case overflows the stack. Called from all four `HistoryRead*` overrides whenever a HistoryRead targets a node whose `NodeId` cannot be resolved to a driver full reference. Any client issuing such a HistoryRead triggers an uncatchable `StackOverflowException` that terminates the process — a remotely-triggerable DoS. **Recommendation:** Replace the self-call with the result-slot assignment mirroring `WriteUnsupported`/`WriteInternalError`: `results[i] = new OpcHistoryReadResult { StatusCode = StatusCodes.BadNodeIdUnknown };` then `errors[i] = StatusCodes.BadNodeIdUnknown;`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — replaced the unconditional self-call in `WriteNodeIdUnknown` with the result-slot assignment (`results[i] = new OpcHistoryReadResult { StatusCode = StatusCodes.BadNodeIdUnknown }`), mirroring `WriteUnsupported`/`WriteInternalError`; the helper is now `internal` for testability. Regression test `DriverNodeManagerHistoryMappingTests.WriteNodeIdUnknown_returns_BadNodeIdUnknown_without_unbounded_recursion` runs the helper on a small-stack worker thread and asserts it returns promptly with `BadNodeIdUnknown`. ### Server-002 | Field | Value | diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs index 801e080..22189c5 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs @@ -1788,9 +1788,10 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder errors[i] = StatusCodes.BadUserAccessDenied; } - private static void WriteNodeIdUnknown(IList results, IList errors, int i) + // Internal so the test suite can pin the StatusCode without booting a server fixture. + internal static void WriteNodeIdUnknown(IList results, IList errors, int i) { - WriteNodeIdUnknown(results, errors, i); + results[i] = new OpcHistoryReadResult { StatusCode = StatusCodes.BadNodeIdUnknown }; errors[i] = StatusCodes.BadNodeIdUnknown; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/DriverNodeManagerHistoryMappingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/DriverNodeManagerHistoryMappingTests.cs index fc76f33..643fa12 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/DriverNodeManagerHistoryMappingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/DriverNodeManagerHistoryMappingTests.cs @@ -1,4 +1,6 @@ using System.Linq; +using System.Threading; +using System.Threading.Tasks; using Opc.Ua; using Shouldly; using Xunit; @@ -143,6 +145,39 @@ public sealed class DriverNodeManagerHistoryMappingTests dv.ServerTimestamp.ShouldBe(new DateTime(2024, 5, 1, 10, 0, 1, DateTimeKind.Utc)); } + [Fact] + public async Task WriteNodeIdUnknown_returns_BadNodeIdUnknown_without_unbounded_recursion() + { + // Regression for Server-001: WriteNodeIdUnknown previously called itself unconditionally + // as its first statement — unbounded recursion with no base case → StackOverflowException, + // an uncatchable crash of the whole server process. A HistoryRead targeting an + // unresolvable NodeId reaches this helper (HistoryReadRawModified / HistoryReadProcessed / + // HistoryReadAtTime all call it when ResolveFullRef yields null), so the bug was a + // remotely-triggerable DoS. The helper must instead just populate the result + error + // slots with BadNodeIdUnknown, mirroring WriteUnsupported / WriteInternalError. + // + // The call runs on a worker thread with a deliberately small (256 KiB) stack: if the + // self-recursion ever returns, the StackOverflowException tears down only that thread's + // worker rather than crashing the test host, and the join below times out instead. + var results = new HistoryReadResultCollection { new() }; + var errors = new List { ServiceResult.Good }; + + var completed = false; + var worker = new Thread(() => + { + DriverNodeManager.WriteNodeIdUnknown(results, errors, 0); + completed = true; + }, maxStackSize: 256 * 1024); + worker.IsBackground = true; + worker.Start(); + worker.Join(TimeSpan.FromSeconds(5)); + + completed.ShouldBeTrue("WriteNodeIdUnknown must return promptly, not recurse until the stack overflows"); + results[0].StatusCode.Code.ShouldBe(StatusCodes.BadNodeIdUnknown); + errors[0].StatusCode.Code.ShouldBe(StatusCodes.BadNodeIdUnknown); + await Task.CompletedTask; + } + [Fact] public void ToDataValue_leaves_SourceTimestamp_default_when_snapshot_has_no_source_time() {