From 14c31941c77fd24066a50152e80c51a4fb1690b6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 17:16:09 -0400 Subject: [PATCH] fix(runtime): parallel-safe decompression limit; tighten ParsedDeploymentArtifact surface --- .../Drivers/DeploymentArtifact.cs | 41 ++++++++++++---- .../ZB.MOM.WW.OtOpcUa.Runtime.csproj | 7 ++- .../Drivers/ParsedDeploymentArtifactTests.cs | 49 ++++++++++--------- 3 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs index 698dd077..9800e657 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs @@ -114,7 +114,8 @@ public static class DeploymentArtifact /// The blob is not valid JSON, is not decompressible, or inflates past /// . Unlike the static wrappers — which own the lenient "malformed ⇒ /// empty result" contract — opening is explicit about unusable bytes. - public static ParsedDeploymentArtifact Open(byte[] blob) => new(JsonDocument.Parse(Inflate(blob))); + internal static ParsedDeploymentArtifact Open(byte[] blob) => + new(JsonDocument.Parse(Inflate(blob, MaxDecompressedBytes))); /// /// Hard ceiling on the decompressed size of an artifact blob (512 MiB). A corrupt or hostile blob @@ -123,10 +124,13 @@ public static class DeploymentArtifact /// ones are single-digit MB before compression. /// /// - /// Mutable and internal purely so tests can lower it: proving the guard fires with the production - /// value would mean crafting and inflating half a gigabyte per test run. Production never writes it. + /// A deliberately: it used to be a mutable static that the size-guard test + /// lowered and restored. xUnit runs test CLASSES in parallel, so that window was visible to every + /// sibling class parsing a >4 KB artifact concurrently — a spurious "refusing to inflate" failure + /// with no relation to the test that caused it. The limit is a parameter + /// instead, so the guard is provable at any ceiling without any process-wide mutation. /// - internal static int MaxDecompressedBytes = 512 * 1024 * 1024; + internal const int MaxDecompressedBytes = 512 * 1024 * 1024; /// /// Return the artifact's JSON bytes, decompressing when the blob carries the gzip magic @@ -150,13 +154,16 @@ public static class DeploymentArtifact /// /// /// The stored/wire artifact bytes. + /// Ceiling on the inflated size. Production always passes + /// via ; the parameter exists so the guard can be + /// proved at a small ceiling without mutating process-wide state that parallel test classes share. /// The JSON bytes to parse. - /// The gzip stream is corrupt or inflates past . - internal static byte[] Inflate(byte[] blob) + /// The gzip stream is corrupt or inflates past . + internal static byte[] Inflate(byte[] blob, int maxBytes) { if (blob.Length < 2 || blob[0] != 0x1F || blob[1] != 0x8B) return blob; - var limit = MaxDecompressedBytes; + var limit = maxBytes; try { using var input = new MemoryStream(blob, writable: false); @@ -340,8 +347,15 @@ public static class DeploymentArtifact /// Not thread-safe: the per-node memo is an unsynchronised /// dictionary. An apply resolves its consumers sequentially, which is the only intended use. /// +/// +/// Internal on purpose. Every consumer is inside this assembly (the apply paths in +/// DriverHostActor and OpcUaPublishActor, plus the tests via InternalsVisibleTo), +/// and the lenient static wrappers on are the cross-assembly door. +/// Keeping the disposable, non-thread-safe, document-owning type unexported is what makes the +/// "results survive disposal" contract enforceable rather than merely documented. +/// /// -public sealed class ParsedDeploymentArtifact : IDisposable +internal sealed class ParsedDeploymentArtifact : IDisposable { private readonly JsonDocument _document; @@ -357,8 +371,15 @@ public sealed class ParsedDeploymentArtifact : IDisposable /// The parsed artifact document, disposed with this instance. internal ParsedDeploymentArtifact(JsonDocument document) => _document = document; - /// The artifact's root JSON object. - public JsonElement Root => _document.RootElement; + /// + /// The artifact's root JSON object. Valid only until — do not retain it, + /// and do not hand it outside this type. Unlike everything the parse methods return (fully + /// materialised records, lists and strings that hold no reference back into the document), a + /// is a live view over the disposed-with-us : + /// reading one after disposal throws, or worse, reads a recycled pooled buffer. Internal so that + /// escape hatch cannot be taken from outside the assembly. + /// + internal JsonElement Root => _document.RootElement; /// Releases the underlying . Idempotent. public void Dispose() diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ZB.MOM.WW.OtOpcUa.Runtime.csproj b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ZB.MOM.WW.OtOpcUa.Runtime.csproj index b967ec9f..d45ea40d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ZB.MOM.WW.OtOpcUa.Runtime.csproj +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ZB.MOM.WW.OtOpcUa.Runtime.csproj @@ -6,8 +6,11 @@ - + diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/ParsedDeploymentArtifactTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/ParsedDeploymentArtifactTests.cs index 8fda1c01..4c43610b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/ParsedDeploymentArtifactTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/ParsedDeploymentArtifactTests.cs @@ -410,33 +410,36 @@ public sealed class ParsedDeploymentArtifactTests // A hostile/corrupt blob must not be able to OOM the node: gzip of a highly repetitive payload // reaches ~1000:1, so a few-hundred-KB artifact row could inflate to hundreds of MB. The production // limit is 512 MB, which no honest artifact approaches — crafting a real 512 MB expansion in a unit - // test would burn seconds and gigabytes, so the limit is an internal seam this test lowers instead. - var original = DeploymentArtifact.MaxDecompressedBytes; - try - { - var payload = new byte[64 * 1024]; // all zeroes: compresses to a few dozen bytes - var bomb = Gzip(payload); - bomb.Length.ShouldBeLessThan(1024); // the small-in / huge-out shape the guard exists for + // test would burn seconds and gigabytes, so the ceiling is a parameter on the internal Inflate seam + // and this test passes a small one. + // + // It used to lower a mutable DeploymentArtifact.MaxDecompressedBytes static and restore it in a + // finally. xUnit runs test CLASSES in parallel, so that window was visible to every sibling class + // parsing a >4 KB artifact concurrently — they would fail with "refusing to inflate" for reasons + // having nothing to do with them. Do not reintroduce a process-wide mutation here. + var payload = new byte[64 * 1024]; // all zeroes: compresses to a few dozen bytes + var bomb = Gzip(payload); + bomb.Length.ShouldBeLessThan(1024); // the small-in / huge-out shape the guard exists for - DeploymentArtifact.MaxDecompressedBytes = 4096; + var ex = Should.Throw(() => DeploymentArtifact.Inflate(bomb, maxBytes: 4096)); + ex.Message.ShouldContain("4096"); + ex.Message.ShouldContain("refusing to inflate"); - var ex = Should.Throw(() => DeploymentArtifact.Open(bomb)); - ex.Message.ShouldContain("4096"); + // Falsifiability control: the SAME bytes at the production ceiling inflate to the full payload — so + // the throw above is the guard firing, not the gzip stream being unreadable. + DeploymentArtifact.Inflate(bomb, DeploymentArtifact.MaxDecompressedBytes).Length.ShouldBe(payload.Length); - // And the lenient statics swallow it exactly like malformed JSON. - DeploymentArtifact.ParseComposition(bomb).EquipmentNodes.ShouldBeEmpty(); - DeploymentArtifact.ParseDriverInstances(bomb).ShouldBeEmpty(); + // …and the guard is actually wired into the door every consumer comes through: Open() at the + // production ceiling gets past decompression and fails as JSON (64 KB of NULs is not JSON), never + // as a bomb. + Should.Throw(() => DeploymentArtifact.Open(bomb)) + .Message.ShouldNotContain("refusing to inflate"); - // Falsifiability control: under the production limit these same bytes decompress fine (and then - // fail as JSON, not as a bomb) — so the assertion above is the guard firing, not gzip failing. - DeploymentArtifact.MaxDecompressedBytes = original; - Should.Throw(() => DeploymentArtifact.Open(bomb)) - .Message.ShouldNotContain("refusing to inflate"); - } - finally - { - DeploymentArtifact.MaxDecompressedBytes = original; - } + // The over-limit JsonException lands in the same lenient catch the static wrappers already own — + // that leniency is proved on the corrupt-gzip path in Static_wrappers_stay_lenient_on_corrupt_gzip, + // which reaches it through the identical `catch (JsonException)` in every wrapper. Asserting it here + // with these bytes would be vacuous: 64 KB of NULs comes back empty from a lenient wrapper whether + // the guard fired or the JSON parse did. } private static byte[] Gzip(byte[] plain)