From c2c3c52274bf941bc28bbd46cb0c8bac4783e220 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 12:25:15 -0400 Subject: [PATCH] fix(abcip): unblock the controller-browse gate, and fix the decoder it exposed (#468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AbCip controller browse could never be verified end to end: libplctag's ab_server answers CIP service 0x55 (Get Instance Attribute List) on the Symbol Object class with "unsupported", so the `@tags` walk had no backend. #468's own definition of done offered a sim that serves `@tags` as an alternative to real hardware; this is that. The fixture now applies a vendored patch teaching ab_server to serve the Symbol Object listing, with partial-transfer paging. Correctness is checked against libplctag's own `list_tags_logix` rather than against our decoder — our decoder is the thing under test, so its agreement would prove nothing. The clone is also pinned to a concrete tag: the Dockerfile called `--branch release` "pinned" while tracking a branch that moves. That immediately exposed a real defect. CipSymbolObjectDecoder advanced to an even byte boundary after each symbol name, but entries are packed with no padding — libplctag walks them by sizeof(entry) + string_len, and a captured reply confirms it. Any odd-length name therefore desynchronised every entry behind it, and odd-length names are ordinary. In the live browse below, `Motor_Run` is 9 bytes and first on the wire, so the old decoder would have shown that one tag and silently dropped the other eight. Every existing multi-entry test passed throughout, because the test builder padded exactly as the decoder did: the two agreed with each other and neither agreed with a controller. The builder no longer pads, and the layout is pinned by a captured reply (CipSymbolObjectWireCaptureTests) instead of by a second statement of the assumption. Restoring the alignment step reddens exactly the three new tests and none of the 29 others. Live gate on docker-dev, all three of #468's criteria: - populated tree — all nine seeded tags render as leaves, in wire order - refresh re-captures - leaf commit — Pump_1_Speed + TestDINT saved with their CIP tag paths The gate also surfaced two things it was not asked about: browse-commit drops the discovered data type, so a browsed REAL commits as a DINT and the driver decodes it as one (filed as #536), and Test Connect fails against this fixture regardless of the device because it probes `@raw_cpu_type` (documented). TwinCAT and FOCAS still have no browsable backend; #468's AbCip half is closed, theirs is not. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- ...versal-discovery-browser-implementation.md | 39 +++- .../CipSymbolObjectDecoder.cs | 18 +- .../Docker/Dockerfile | 18 +- .../Docker/README.md | 55 ++++- .../Docker/docker-compose.yml | 11 +- ...rver-serve-cip-symbol-object-listing.patch | 204 ++++++++++++++++++ .../CipSymbolObjectDecoderTests.cs | 46 +++- .../CipSymbolObjectWireCaptureTests.cs | 118 ++++++++++ 8 files changed, 485 insertions(+), 24 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0001-ab_server-serve-cip-symbol-object-listing.patch create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs diff --git a/docs/plans/2026-07-15-universal-discovery-browser-implementation.md b/docs/plans/2026-07-15-universal-discovery-browser-implementation.md index 5f946d95..a0fd089a 100644 --- a/docs/plans/2026-07-15-universal-discovery-browser-implementation.md +++ b/docs/plans/2026-07-15-universal-discovery-browser-implementation.md @@ -1105,12 +1105,43 @@ Equipment-kind namespace and bound to an equipment; a tag was added on the Tags- - ⚠️ **Not verifiable on this fixture** — a *populated* tree render + leaf-commit. The libplctag `ab_server` test sim returns `ErrorUnsupported` for controller symbol enumeration (it implements tag read/write, not the `@tags` Symbol Object walk). This is a **fixture limitation, not a code defect** — the binding is proven alive. + **CLOSED 2026-08-01 — see the second gate below.** -### Follow-ups discovered during the gate +## Second live-gate result (Gitea #468 — docker-dev, 2026-08-01) -1. **Full tree-render verification is fixture-blocked.** `ab_server` doesn't implement the `@tags` controller - walk. To live-verify a populated tree + `TagConfig.FullName` leaf commit, point the universal browser at a - symbol-enumeration-capable AB backend (a real ControlLogix, or a sim that serves `@tags`). +The fixture gap was closed by teaching `ab_server` to serve the Symbol Object listing +(`tests/Drivers/…AbCip.IntegrationTests/Docker/patches/`), which was the option #468's own +definition of done offered alongside real hardware. All three criteria ran green against the +patched fixture on `10.100.0.35:44818`: + +- ✅ **Populated controller-tag tree** — the AbCip browse rendered all nine seeded tags as leaves + under `AbCip → Device1 → Discovered`, in wire order: `Motor_Run`, `Line1_Count`, `Pump_1_Speed`, + `TestArray`, `TestString`, `TestSINT`, `TestBOOL`, `TestREAL`, `TestDINT`. +- ✅ **Refresh re-captures** — Refresh collapsed to the root and re-expansion returned the same + nine leaves from a fresh capture. +- ✅ **Leaf commit round-trip** — selecting `Pump_1_Speed` + `TestDINT` and committing wrote both + into the raw tree with the CIP tag path in `TagConfig` (`{"tagPath":"TestDINT",…}`) and saved. + +**The gate earned its keep — it found a defect no unit test could have.** `CipSymbolObjectDecoder` +advanced to an even byte boundary after each symbol name, but Symbol Object entries are packed with +no padding. `Motor_Run` is 9 bytes, so under the old decoder the browse above would have shown +**`Motor_Run` alone** and silently dropped the other eight. Every existing multi-entry test passed, +because the test builder padded exactly as the decoder did — the pair agreed with each other and +neither agreed with a controller. Fixed, with the layout now pinned by a captured reply +(`CipSymbolObjectWireCaptureTests`) rather than by a restatement of the assumption. + +### Follow-ups discovered during the second gate + +1. **Browse-commit discards the discovered data type** — a browsed REAL commits with + `"dataType":"DInt"` in its `TagConfig`, and the driver decodes off that field rather than the + row's `DataType`. Filed as **Gitea #536**; affects AbCip and (by inspection) TwinCAT. +2. **Test Connect fails against this fixture regardless of the device** — the probe falls back to + `@raw_cpu_type` when no tags are authored, which `ab_server` does not implement. Fixture + limitation, documented in the fixture README; browse is unaffected. + +### Follow-ups discovered during the first gate + +1. ~~**Full tree-render verification is fixture-blocked.**~~ **CLOSED** by the second gate above. 2. **Driver-page address pickers pass no live config.** `AbCipDriverPage`/`TwinCATDriverPage`/`FOCASDriverPage` host the same `*AddressPickerBody` but do **not** pass `DriverType`/`GetConfigJson`, so their (now-visible) Browse button would capture against `"{}"` (no device). Out of Task 14's scope (which listed only the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs index 740b9efe..aad6a2b1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs @@ -11,18 +11,22 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// /// /// Entry layout (little-endian) per Rockwell CIP Vol 1 + Logix 5000 CIP Programming -/// Manual (1756-PM019 chapter "Symbol Object"), cross-checked against libplctag's -/// ab/cip.c handle_listed_tags_reply: +/// Manual (1756-PM019 chapter "Symbol Object"), checked against a captured controller reply — +/// see CipSymbolObjectWireCaptureTests: /// /// u32Symbol Instance ID — opaque identifier for the tag. /// u16Symbol Type — lower 12 bits = CIP type code (0xC1 BOOL, -/// 0xC2 SINT, …, 0xD0 STRING). Bit 12 = system-tag flag. Bit 13 = reserved. +/// 0xC2 SINT, …, 0xD0 STRING). Bit 12 = system-tag flag. Bits 13-14 = array dimension +/// count, so an ordinary one-dimensional DINT arrives as 0x20C4 and the low 12 +/// bits must be masked off before the type is read. /// Bit 15 = struct flag; when set, the lower 12 bits are the template instance id /// (not a primitive type code). /// u16Element length — bytes per element (e.g. 4 for DINT). /// u32 × 3Array dimensions — zero for scalar tags. /// u16Symbol name length in bytes. -/// u8 × NASCII symbol name, padded to an even byte boundary. +/// u8 × NASCII symbol name. Not padded — entries are +/// packed back to back, so an odd-length name leaves the next entry on an odd +/// offset. /// /// /// Program:-scope tags arrive with their scope prefix baked into the name @@ -66,8 +70,12 @@ public static class CipSymbolObjectDecoder if (pos + nameLength > buffer.Length) break; var name = Encoding.ASCII.GetString(buffer, pos, nameLength); + + // No alignment step: the next entry begins on the byte immediately after the last name + // byte, even when that is an odd offset. This used to advance to an even boundary, which + // read a correct first entry and then garbage for everything following an odd-length + // name — and odd-length names are ordinary (Gitea #468). pos += nameLength; - if ((pos & 1) != 0) pos++; // even-align for the next entry if (string.IsNullOrWhiteSpace(name)) continue; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/Dockerfile b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/Dockerfile index 0f259025..d6950e64 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/Dockerfile +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/Dockerfile @@ -15,13 +15,23 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ && rm -rf /var/lib/apt/lists/* -# Pinned tag matches the `ab_server` version AbServerFixture + CI treat as -# canonical. Bump deliberately alongside a driver-side change that needs -# something newer. -ARG LIBPLCTAG_TAG=release +# A concrete tag, not a branch. This said "pinned" while cloning `release`, +# which moves — so two builds of the same Dockerfile could produce different +# servers, and the patch below could not be relied on to apply. Bump +# deliberately alongside a driver-side change that needs something newer, and +# re-check the patch against the new tag. +ARG LIBPLCTAG_TAG=v2.7.0 RUN git clone --depth 1 --branch "${LIBPLCTAG_TAG}" https://github.com/libplctag/libplctag.git /src +# Upstream ab_server answers CIP service 0x55 (Get Instance Attribute List) with +# "unsupported", so a controller-tag browse against it returns nothing. The +# patch implements the Symbol Object listing that a real Logix controller +# serves, which is what the AbCip driver's `@tags` walk reads. Verified against +# libplctag's own `list_tags_logix` client — see README.md. +COPY patches/ /patches/ WORKDIR /src +RUN git apply --verbose /patches/*.patch + RUN cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \ && cmake --build build --target ab_server --parallel diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md index 7a6db837..a4478384 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md @@ -12,6 +12,47 @@ clone needs Docker Desktop and nothing else. |---|---| | [`Dockerfile`](Dockerfile) | Multi-stage: build from libplctag at pinned tag → copy binary into `debian:bookworm-slim` runtime image | | [`docker-compose.yml`](docker-compose.yml) | One service per family (`controllogix` / `compactlogix` / `micro800` / `guardlogix`); all bind `:44818` | +| [`patches/`](patches/) | Applied to the libplctag checkout before the build — see [Local patches](#local-patches) | + +## Local patches + +`patches/0001-ab_server-serve-cip-symbol-object-listing.patch` teaches `ab_server` +to answer **CIP service 0x55 (Get Instance Attribute List) on class 0x6B**, the +Symbol Object listing a client reads through the `@tags` pseudo-tag to enumerate +a controller's symbol table. Upstream answers `unsupported`, which is what left +the AbCip controller browse unverifiable (Gitea #468) — the browse binding was +proven alive, but no backend ever returned a tag. + +Two things worth knowing before touching it: + +- **The reply layout is fixed to the attribute set libplctag asks for** — + `{0x02 symbol type, 0x07 element size, 0x08 array dimensions, 0x01 symbol + name}`. A request for a different set is refused rather than answered with a + differently-shaped reply the client would misparse. +- **Entries are packed with no padding.** The next entry begins on the byte + after the last name byte, even on an odd offset. This is not a detail of the + fixture: it is what a controller does, and assuming otherwise is exactly the + decoder defect the gate found. + +Correctness is checked against **libplctag's own `list_tags_logix`** client, not +against our decoder — our decoder is the thing under test, so agreeing with it +would prove nothing. `list_tags_logix` is the reference implementation exercised +against real Logix hardware: + +```bash +git clone --depth 1 --branch v2.7.0 https://github.com/libplctag/libplctag.git +cd libplctag && cmake -S . -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build --parallel +./build/bin_dist/list_tags_logix 10.100.0.35 1,0 # expect all nine tags, "SUCCESS!" +``` + +Partial-transfer paging is exercised the same way by starting `ab_server` with a +few hundred tags: all of them should come back, with no duplicates and no gaps +at the page boundaries. + +**Bumping `LIBPLCTAG_TAG` means re-checking the patch against the new tag.** It +is a concrete tag rather than the `release` branch precisely so that this stays +a deliberate act — the branch moves, and the Dockerfile previously called it +"pinned" while tracking it. ## Run @@ -64,7 +105,7 @@ place means updating both. | Family | Seeded tags | Notes | |---|---|---| -| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server | +| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server. The last three serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | | CompactLogix | `TestDINT` `TestREAL` `TestBOOL` | Narrow ConnectionSize cap enforced driver-side; ab_server accepts any size | | Micro800 | `TestDINT` `TestREAL` | ab_server has no `micro800` mode; falls back to `controllogix` emulation | | GuardLogix | `TestDINT` `SafetyDINT_S` | ab_server has no safety subsystem; `_S` suffix triggers driver-side classification only | @@ -73,7 +114,17 @@ place means updating both. - **No UDT / CIP Template Object emulation** — `ab_server` covers atomic types only. UDT reads + task #194 whole-UDT optimization verify via - unit tests with golden byte buffers. + unit tests with golden byte buffers. The Symbol Object patch above lists + symbols, including their types; it does not add Template Object support, so a + browsed UDT still cannot be expanded into members here. +- **No `@raw_cpu_type`** — the driver's Test Connect probe falls back to that + pseudo-tag when a device has no authored tags, and `ab_server` answers + `ErrorBadParam`. So **Test Connect reports failure against this fixture even + when the device is fine**, and browse works regardless. Author one tag and the + probe uses it instead. +- **Controller-scope listing only** — a program-scope listing arrives with a + leading symbolic segment naming the program, and `ab_server` has no notion of + programs. `Program:`-scoped tags cannot be browsed here. - **Family-specific quirks trust driver-side code** — ab_server emulates a generic Logix CPU; the ConnectionSize cap, empty-path unconnected mode, and safety-partition write rejection all need lab rigs for diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml index a9b56f25..27e4ef18 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml @@ -20,6 +20,12 @@ services: restart: "no" ports: - "44818:44818" + # The last three are here for the controller-browse gate (Gitea #468) and + # are additive — the read/write suites key off the Test* tags above. + # Odd-length names are deliberate: Symbol Object entries are packed with no + # padding, so a name with an odd byte count puts every following entry on an + # odd offset. A decoder that assumes even alignment reads all six correctly + # and then loses everything after the first odd name. command: [ "ab_server", "--plc=ControlLogix", @@ -30,7 +36,10 @@ services: "--tag=TestBOOL:BOOL[1]", "--tag=TestSINT:SINT[1]", "--tag=TestString:STRING[1]", - "--tag=TestArray:DINT[16]" + "--tag=TestArray:DINT[16]", + "--tag=Pump_1_Speed:REAL[1]", + "--tag=Line1_Count:DINT[1]", + "--tag=Motor_Run:BOOL[1]" ] compactlogix: diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0001-ab_server-serve-cip-symbol-object-listing.patch b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0001-ab_server-serve-cip-symbol-object-listing.patch new file mode 100644 index 00000000..469a7263 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0001-ab_server-serve-cip-symbol-object-listing.patch @@ -0,0 +1,204 @@ +diff --git a/src/tools/ab_server/cip.c b/src/tools/ab_server/cip.c +index 68a5794..f266bf6 100644 +--- a/src/tools/ab_server/cip.c ++++ b/src/tools/ab_server/cip.c +@@ -111,6 +111,18 @@ const uint8_t CIP_OBJ_CONNECTION_MANAGER[] = {0x20, 0x06, 0x24, 0x01}; + + #define CIP_TAG_MAX_INDEXES ((uint32_t)3) /* should double check for OMRON */ + ++/* Symbol Object (class 0x6B) listing — see handle_list_tags_request(). */ ++#define CIP_SYMBOL_OBJECT_CLASS ((uint8_t)0x6b) ++#define CIP_LIST_TAGS_PATH_SIZE ((size_t)6) /* 0x20 0x6B 0x25 0x00 */ ++#define CIP_LIST_TAGS_ENTRY_HEADER_SIZE ((size_t)22) ++#define CIP_LIST_TAGS_PAYLOAD_MIN_SIZE ((size_t)2) /* two bytes for the attribute count */ ++#define CIP_LIST_TAGS_ATTR_COUNT ((uint16_t)4) ++#define CIP_SYM_ATTR_NAME ((uint16_t)0x01) ++#define CIP_SYM_ATTR_TYPE ((uint16_t)0x02) ++#define CIP_SYM_ATTR_ELEM_SIZE ((uint16_t)0x07) ++#define CIP_SYM_ATTR_DIMENSIONS ((uint16_t)0x08) ++#define CIP_SYM_TYPE_DIM_SHIFT (13) /* bits 13-14 carry the array dimension count. */ ++ + #define MAX_SUB_PACKETS ((uint16_t)1000) /* maximum number of sub-packets in a multi-service request */ + + #define CIP_MINIMAL_RESPONSE_SIZE ((size_t)6) /* four bytes for header plus 2 for optional extended status. */ +@@ -126,6 +138,8 @@ static slice_s handle_read_request(uint8_t cip_service, slice_s cip_service_path + plc_s *plc); + static slice_s handle_write_request(uint8_t cip_service, slice_s cip_service_path, slice_s cip_service_payload, slice_s output, + plc_s *plc); ++static slice_s handle_list_tags_request(uint8_t cip_service, slice_s cip_service_path, slice_s cip_service_payload, ++ slice_s output, plc_s *plc); + static slice_s handle_multi_request(uint8_t cip_service, slice_s cip_service_path, slice_s cip_service_payload, slice_s output, + plc_s *plc); + +@@ -220,6 +234,10 @@ slice_s cip_dispatch_request(slice_s input, slice_s output, plc_s *plc) { + return handle_write_request(cip_service, cip_service_path, cip_service_payload, output, plc); + break; + ++ case CIP_SRV_INSTANCES_ATTRIBS: ++ return handle_list_tags_request(cip_service, cip_service_path, cip_service_payload, output, plc); ++ break; ++ + default: return make_cip_log_error(output, cip_service, CIP_ERR_UNSUPPORTED, false, 0); break; + } + } +@@ -877,6 +895,160 @@ slice_s handle_read_request(uint8_t cip_service, slice_s cip_service_path, slice + } + + ++/* ++ * Handle the CIP "Get Instance Attribute List" service (0x55) against the Symbol ++ * Object class (0x6B). This is the request a client issues to enumerate the ++ * controller's symbol table — libplctag sends it when an application reads the ++ * "@tags" pseudo-tag. ++ * ++ * Request path (6 bytes): ++ * 0x20 0x6B 8-bit class segment, Symbol Object ++ * 0x25 0x00 16-bit instance segment + pad ++ * u16 instance id to start from ++ * ++ * Request payload: ++ * u16 attribute count ++ * u16 * count attribute ids ++ * ++ * The reply layout is determined by the requested attributes, so this server ++ * serves exactly the set libplctag asks for — symbol type, element size, array ++ * dimensions, symbol name — and refuses anything else rather than returning a ++ * differently-shaped reply the client would silently misparse. ++ * ++ * Reply payload is a packed run of entries, each: ++ * u32 instance id ++ * u16 symbol type ++ * u16 element length in bytes ++ * u32 [3] array dimensions ++ * u16 name length ++ * u8 * length name ++ * ++ * NOTE: entries are NOT padded. The next entry starts on the byte immediately ++ * after the last name byte, even when that leaves it on an odd offset — see ++ * libplctag's own reply walker in eip_cip_special.c, which advances by ++ * sizeof(entry) + string_len with no alignment step. ++ * ++ * When the entries do not all fit in the response buffer the reply carries ++ * status 0x06 (partial transfer) and the client asks again starting from the ++ * last instance id it saw, plus one. ++ */ ++slice_s handle_list_tags_request(uint8_t cip_service, slice_s cip_service_path, slice_s cip_service_payload, slice_s output, ++ plc_s *plc) { ++ uint16_t start_instance_id = 0; ++ uint16_t attribute_count = 0; ++ uint16_t instance_id = 0; ++ size_t payload_offset = 0; ++ size_t output_offset = CIP_RESPONSE_HEADER_SIZE; ++ bool more_to_come = false; ++ tag_def_s *tag = NULL; ++ ++ log_info("Processing Get Instance Attribute List (Symbol Object) request."); ++ ++ /* Only the controller-scope listing is served. A program-scope listing ++ * arrives with a leading symbolic segment naming the program, and this ++ * server has no notion of program scopes to list. */ ++ if(slice_len(cip_service_path) != CIP_LIST_TAGS_PATH_SIZE || slice_get_uint8(cip_service_path, 0) != 0x20 ++ || slice_get_uint8(cip_service_path, 1) != CIP_SYMBOL_OBJECT_CLASS || slice_get_uint8(cip_service_path, 2) != 0x25 ++ || slice_get_uint8(cip_service_path, 3) != 0x00) { ++ log_info("Unsupported path for a Symbol Object listing request!"); ++ log_info_slice(cip_service_path); ++ return make_cip_log_error(output, cip_service, CIP_ERR_PATH_SEGMENT, false, 0); ++ } ++ ++ start_instance_id = slice_get_uint16_le(cip_service_path, 4); ++ ++ log_info("Listing symbols from instance %u.", (unsigned int)start_instance_id); ++ ++ if(slice_len(cip_service_payload) < CIP_LIST_TAGS_PAYLOAD_MIN_SIZE) { ++ log_info("Insufficient data in the Symbol Object listing payload!"); ++ return make_cip_log_error(output, cip_service, CIP_ERR_INSUFFICIENT_DATA, false, 0); ++ } ++ ++ attribute_count = slice_get_uint16_le(cip_service_payload, payload_offset); ++ payload_offset += 2; ++ ++ if(slice_len(cip_service_payload) < payload_offset + ((size_t)attribute_count * 2)) { ++ log_info("Symbol Object listing payload is shorter than its attribute count claims!"); ++ return make_cip_log_error(output, cip_service, CIP_ERR_INSUFFICIENT_DATA, false, 0); ++ } ++ ++ if(attribute_count != CIP_LIST_TAGS_ATTR_COUNT ++ || slice_get_uint16_le(cip_service_payload, payload_offset + 0) != CIP_SYM_ATTR_TYPE ++ || slice_get_uint16_le(cip_service_payload, payload_offset + 2) != CIP_SYM_ATTR_ELEM_SIZE ++ || slice_get_uint16_le(cip_service_payload, payload_offset + 4) != CIP_SYM_ATTR_DIMENSIONS ++ || slice_get_uint16_le(cip_service_payload, payload_offset + 6) != CIP_SYM_ATTR_NAME) { ++ log_info("Symbol Object listing requested an attribute set this server does not serve!"); ++ log_info_slice(cip_service_payload); ++ return make_cip_log_error(output, cip_service, CIP_ERR_INVALID_PARAM, false, 0); ++ } ++ ++ if(slice_len(output) < CIP_RESPONSE_HEADER_SIZE) { ++ log_info("Insufficient space in the output buffer for the response header!"); ++ return make_cip_log_error(output, cip_service, CIP_ERR_INSUFFICIENT_DATA, false, 0); ++ } ++ ++ /* Instance ids are assigned by position in the tag list. The walk order is ++ * stable across requests, so ids ascend the same way every time and paging ++ * can simply resume at the first id the client has not seen. */ ++ for(tag = plc->tags; tag; tag = tag->next_tag) { ++ size_t name_len = 0; ++ size_t entry_len = 0; ++ uint16_t symbol_type = 0; ++ ++ instance_id++; ++ ++ if(instance_id < start_instance_id) { continue; } ++ ++ name_len = strlen(tag->name); ++ entry_len = CIP_LIST_TAGS_ENTRY_HEADER_SIZE + name_len; ++ ++ if(output_offset + entry_len > slice_len(output)) { ++ log_info("Response buffer full — %u symbol(s) sent, more to come.", (unsigned int)(instance_id - 1)); ++ more_to_come = true; ++ break; ++ } ++ ++ /* Bits 13-14 of the symbol type carry the array dimension count; the ++ * low 12 bits carry the CIP type code. */ ++ symbol_type = (uint16_t)(tag->tag_type | ((uint16_t)(tag->num_dimensions & 0x03) << CIP_SYM_TYPE_DIM_SHIFT)); ++ ++ slice_set_uint32_le(output, output_offset + 0, (uint32_t)instance_id); ++ slice_set_uint16_le(output, output_offset + 4, symbol_type); ++ slice_set_uint16_le(output, output_offset + 6, (uint16_t)tag->elem_size); ++ ++ for(size_t dim = 0; dim < 3; dim++) { ++ uint32_t dim_size = (dim < tag->num_dimensions) ? (uint32_t)tag->dimensions[dim] : 0; ++ slice_set_uint32_le(output, output_offset + 8 + (dim * 4), dim_size); ++ } ++ ++ slice_set_uint16_le(output, output_offset + 20, (uint16_t)name_len); ++ ++ if(!slice_copy_data_in(slice_from_slice(output, output_offset + CIP_LIST_TAGS_ENTRY_HEADER_SIZE, name_len), ++ (uint8_t *)tag->name, name_len)) { ++ log_info("Unable to copy the symbol name into the response!"); ++ return make_cip_log_error(output, cip_service, CIP_ERR_INVALID_PARAM, false, 0); ++ } ++ ++ output_offset += entry_len; ++ } ++ ++ /* A partial reply carrying no entries at all would leave the client asking ++ * for the same instance forever, because it advances its cursor from the ++ * last entry it received. Fail the request instead of looping it. */ ++ if(more_to_come && output_offset == CIP_RESPONSE_HEADER_SIZE) { ++ log_info("A single symbol entry does not fit in the response buffer!"); ++ return make_cip_log_error(output, cip_service, CIP_ERR_EXTENDED, true, CIP_ERR_EX_TOO_LONG); ++ } ++ ++ slice_set_uint8(output, 0, cip_service | CIP_DONE); ++ slice_set_uint8(output, 1, 0); /* reserved */ ++ slice_set_uint8(output, 2, (more_to_come ? CIP_ERR_FRAG : CIP_OK)); /* status */ ++ slice_set_uint8(output, 3, 0); /* no extended error */ ++ ++ return slice_from_slice(output, 0, output_offset); ++} ++ ++ + #define CIP_WRITE_MIN_SIZE (6) + #define CIP_WRITE_FRAG_MIN_SIZE (10) + diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectDecoderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectDecoderTests.cs index b5a1fc0d..a5b093c9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectDecoderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectDecoderTests.cs @@ -11,8 +11,14 @@ public sealed class CipSymbolObjectDecoderTests { /// /// Build one Symbol Object entry in the byte layout - /// instance_id(u32) symbol_type(u16) element_length(u16) array_dims(u32×3) name_len(u16) name[len] pad. + /// instance_id(u32) symbol_type(u16) element_length(u16) array_dims(u32×3) name_len(u16) name[len]. /// + /// + /// There is no trailing pad. This builder used to round the entry up to an even length, + /// which is what let the padding defect survive: the builder and the decoder shared one + /// wrong assumption, so every multi-entry test agreed with itself and none of them agreed + /// with a controller. pins the real layout. + /// private static byte[] BuildEntry( uint instanceId, ushort symbolType, @@ -23,7 +29,6 @@ public sealed class CipSymbolObjectDecoderTests var nameBytes = Encoding.ASCII.GetBytes(name); var nameLen = nameBytes.Length; var totalLen = 22 + nameLen; - if ((totalLen & 1) != 0) totalLen++; // pad to even var buf = new byte[totalLen]; BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(0), instanceId); @@ -169,23 +174,48 @@ public sealed class CipSymbolObjectDecoderTests tag.Name.ShouldBe("StepIndex"); } - /// Verifies that multiple entries decode in wire order with proper padding. + /// Verifies that multiple entries decode in wire order. [Fact] - public void Multiple_entries_decode_in_wire_order_with_even_padding() + public void Multiple_entries_decode_in_wire_order() { - // Name "Abc" is 3 bytes — triggers the even-pad branch between entries. var bytes = Concat( - BuildEntry(1, 0xC4, 4, (0, 0, 0), "Abc"), // DINT named "Abc" (3-byte name, pads to 4) - BuildEntry(2, 0xCA, 4, (0, 0, 0), "Pi")); // REAL named "Pi" + BuildEntry(1, 0xC4, 4, (0, 0, 0), "Abcd"), + BuildEntry(2, 0xCA, 4, (0, 0, 0), "Pi")); var tags = CipSymbolObjectDecoder.Decode(bytes).ToList(); tags.Count.ShouldBe(2); - tags[0].Name.ShouldBe("Abc"); + tags[0].Name.ShouldBe("Abcd"); tags[0].DataType.ShouldBe(AbCipDataType.DInt); tags[1].Name.ShouldBe("Pi"); tags[1].DataType.ShouldBe(AbCipDataType.Real); } + /// + /// An odd-length name leaves the following entry on an odd offset, and the decoder must + /// read it there. + /// + /// + /// This is the case Gitea #468 could not reach: every prior multi-entry test used a builder + /// that padded exactly as the decoder did, so the pair agreed with each other and neither + /// agreed with a controller. Odd-length names are not exotic — TestArray, + /// Motor_Run and Line1_Count are all odd, and one of them is enough to lose + /// every tag discovered after it. + /// + [Fact] + public void An_entry_after_an_odd_length_name_still_decodes() + { + var bytes = Concat( + BuildEntry(1, 0xC4, 4, (0, 0, 0), "Abc"), // 3 bytes — next entry starts at 25 + BuildEntry(2, 0xCA, 4, (0, 0, 0), "Pi"), + BuildEntry(3, 0xC1, 1, (0, 0, 0), "Motor_Run")); // 9 bytes — and again after this one + + var tags = CipSymbolObjectDecoder.Decode(bytes).ToList(); + + tags.Select(t => t.Name).ShouldBe(["Abc", "Pi", "Motor_Run"]); + tags.Select(t => t.DataType) + .ShouldBe([AbCipDataType.DInt, AbCipDataType.Real, AbCipDataType.Bool]); + } + /// Verifies that truncated buffers stop decoding gracefully. [Fact] public void Truncated_buffer_stops_decoding_gracefully() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs new file mode 100644 index 00000000..b8fdea96 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs @@ -0,0 +1,118 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Decodes a captured @tags Symbol Object response, byte for byte as libplctag handed +/// it back — the evidence for the entry layout, rather than another statement of it. +/// +/// +/// +/// Every other decoder test builds its own input, so the builder and the decoder can share +/// a wrong assumption and still agree. They did: both padded each entry to an even length, +/// and a controller pads nothing (Gitea #468). +/// +/// +/// This capture came off the AbCip integration fixture — libplctag's ab_server, +/// patched to serve the Symbol Object listing (see the fixture's +/// Docker/patches/) — and the same server's replies are read correctly by +/// libplctag's own list_tags_logix client. That client is the reference +/// implementation exercised against real Logix hardware, so its agreement is what makes +/// this sample evidence about controllers and not merely about our fixture. +/// +/// +/// Re-capture with plc_tag_create("…&name=@tags") against the fixture and dump +/// plc_tag_get_uint8 across plc_tag_get_size if the fixture's tag set changes. +/// +/// +[Trait("Category", "Unit")] +public sealed class CipSymbolObjectWireCaptureTests +{ + /// + /// The nine tags the controllogix fixture profile serves, in wire order. 281 bytes. + /// + private const string CapturedTagsReply = + "01000000C120010001000000000000000000000009004D6F746F725F52756E" + + "02000000C42004000100000000000000000000000B004C696E65315F436F756E74" + + "03000000CA2004000100000000000000000000000C0050756D705F315F5370656564" + + "04000000C42004001000000000000000000000000900546573744172726179" + + "05000000D02058000100000000000000000000000A0054657374537472696E67" + + "06000000C220010001000000000000000000000008005465737453494E54" + + "07000000C1200100010000000000000000000000080054657374424F4F4C" + + "08000000CA2004000100000000000000000000000800546573745245414C" + + "09000000C420040001000000000000000000000008005465737444494E54"; + + private static byte[] Capture() => Convert.FromHexString(CapturedTagsReply); + + /// The capture is the length the entry layout predicts — no padding anywhere. + /// + /// Nine entries of 22 header bytes plus their name lengths + /// (9 + 11 + 12 + 9 + 10 + 8 + 8 + 8 + 8 = 83) come to 281. An even-padded stream would be + /// 285, so the length alone falsifies the old assumption. + /// + [Fact] + public void The_capture_is_packed_with_no_padding_between_entries() + { + Capture().Length.ShouldBe((9 * 22) + 83); + } + + /// Every tag the fixture serves comes back, in wire order. + /// + /// The first entry's name is 9 bytes, so the second entry begins at offset 31 — an odd + /// offset. Under the old even-alignment step the decoder read Motor_Run and then + /// lost all eight tags behind it. + /// + [Fact] + public void Every_captured_symbol_decodes_in_wire_order() + { + var tags = CipSymbolObjectDecoder.Decode(Capture()).ToList(); + + tags.Select(t => t.Name).ShouldBe([ + "Motor_Run", + "Line1_Count", + "Pump_1_Speed", + "TestArray", + "TestString", + "TestSINT", + "TestBOOL", + "TestREAL", + "TestDINT", + ]); + } + + /// Types survive the array-dimension bits a real symbol type carries. + /// + /// Each captured symbol type is 0x20xx, not 0x00xx: bits 13-14 hold the array + /// dimension count. Only the low 12 bits are the CIP type code, and reading the word whole + /// would classify every one of these as an unknown type. + /// + [Fact] + public void Captured_symbol_types_map_through_the_dimension_bits() + { + var tags = CipSymbolObjectDecoder.Decode(Capture()).ToList(); + + tags.Single(t => t.Name == "Motor_Run").DataType.ShouldBe(AbCipDataType.Bool); + tags.Single(t => t.Name == "Line1_Count").DataType.ShouldBe(AbCipDataType.DInt); + tags.Single(t => t.Name == "Pump_1_Speed").DataType.ShouldBe(AbCipDataType.Real); + tags.Single(t => t.Name == "TestString").DataType.ShouldBe(AbCipDataType.String); + tags.Single(t => t.Name == "TestSINT").DataType.ShouldBe(AbCipDataType.SInt); + } + + /// None of the captured symbols is misread as a system tag or a structure. + /// + /// The dimension bits sit next to the flags the decoder tests for, so a mask that is one + /// bit wide in the wrong place would show up as a plain DINT being reported as a UDT — and + /// a UDT is fanned out into member tags, so the error would not stay quiet. + /// + [Fact] + public void No_captured_symbol_is_misread_as_a_system_tag_or_a_structure() + { + var tags = CipSymbolObjectDecoder.Decode(Capture()).ToList(); + + tags.ShouldAllBe(t => !t.IsSystemTag); + tags.ShouldAllBe(t => t.DataType != AbCipDataType.Structure); + tags.ShouldAllBe(t => t.TemplateInstanceId == null); + } +}