From 7d30009dc87953b38cc61b7fedb3f6bc22c0c764 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:23:35 -0400 Subject: [PATCH] fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TryParse now rejects three classes of malformed PCCC address: - Sub-element + bit-index together (e.g. T4:0.ACC/2) — never valid in PCCC - File number on I/O/S system files (e.g. I3:0, S2:1) — single-letter only - Sub-element on non-T/C/R files (e.g. B3:0.DN, N7:0.FOO) — only Timer, Counter, and Control files carry structured elements New helper predicates IsNoFileNumberLetter / IsSubElementFileLetter keep the parser's intent clear. Regression tests added in AbLegacyAddressTests. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../AbLegacyAddress.cs | 27 ++++++++++++++ .../AbLegacyAddressTests.cs | 37 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs index 2cce003..d65eafe 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs @@ -102,6 +102,19 @@ public sealed record AbLegacyAddress( if (maxBit < 0 || b > maxBit) return null; } + // I/O/S are single-letter system files — they carry no file number in the PCCC spec. + // Accepting I3:0 or S2:1 would pass a malformed address straight to libplctag; reject early. + if (fileNumber is not null && IsNoFileNumberLetter(letter)) return null; + + // A PCCC address cannot have both a sub-element and a bit index: the word is either + // structured (T4:0.ACC) or bit-addressed (N7:0/3), never both. + if (subElement is not null && bitIndex is not null) return null; + + // Sub-elements are only meaningful on Timer (T), Counter (C), and Control (R) files — + // those are the only structured-element file types in the PCCC spec. Accepting B3:0.DN + // or N7:0.FOO would produce an address libplctag silently misinterprets. + if (subElement is not null && !IsSubElementFileLetter(letter)) return null; + return new AbLegacyAddress(letter, fileNumber, word, bitIndex, subElement); } @@ -122,4 +135,18 @@ public sealed record AbLegacyAddress( "N" or "F" or "B" or "L" or "ST" or "T" or "C" or "R" or "I" or "O" or "S" or "A" => true, _ => false, }; + + /// + /// Returns for file letters that carry no explicit file number in the + /// PCCC spec. I (input), O (output), and S (status) are single-letter + /// system files; a digit after the letter (e.g. I3) is a malformed address. + /// + private static bool IsNoFileNumberLetter(string letter) => letter is "I" or "O" or "S"; + + /// + /// Returns for file letters that may carry a sub-element suffix + /// (.ACC, .PRE, etc.). Only Timer (T), Counter (C), and + /// Control (R) files have structured elements in the PCCC spec. + /// + private static bool IsSubElementFileLetter(string letter) => letter is "T" or "C" or "R"; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyAddressTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyAddressTests.cs index d032357..9609e67 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyAddressTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyAddressTests.cs @@ -65,4 +65,41 @@ public sealed class AbLegacyAddressTests a.ShouldNotBeNull(); a.ToLibplctagName().ShouldBe(input); } + + // ---- Driver.AbLegacy-003: Parser tightening ---- + + [Theory] + [InlineData("T4:0.ACC/2")] // sub-element + bit index — never valid in PCCC + [InlineData("C5:0.PRE/3")] + public void TryParse_rejects_subelement_plus_bitindex(string input) => + AbLegacyAddress.TryParse(input).ShouldBeNull(); + + [Theory] + [InlineData("I3:0")] // I is a system file — no file number allowed + [InlineData("O2:1")] + [InlineData("S2:1")] + public void TryParse_rejects_file_number_on_IOS_files(string input) => + AbLegacyAddress.TryParse(input).ShouldBeNull(); + + [Theory] + [InlineData("B3:0.DN")] // B (bit) file has no structured elements + [InlineData("N7:0.FOO")] // N (integer) file has no structured elements + [InlineData("F8:0.ACC")] // F (float) file has no structured elements + [InlineData("L9:0.PRE")] // L (long) file has no structured elements + public void TryParse_rejects_subelement_on_non_structured_file(string input) => + AbLegacyAddress.TryParse(input).ShouldBeNull(); + + [Theory] + [InlineData("T4:0.ACC")] // T, C, R are the only structured-element files + [InlineData("C5:0.PRE")] + [InlineData("R6:0.LEN")] + public void TryParse_accepts_subelement_only_on_TCR_files(string input) => + AbLegacyAddress.TryParse(input).ShouldNotBeNull(); + + [Theory] + [InlineData("I:0/0")] // I/O/S without file number are valid + [InlineData("O:1/2")] + [InlineData("S:1")] + public void TryParse_accepts_IOS_without_file_number(string input) => + AbLegacyAddress.TryParse(input).ShouldNotBeNull(); }