dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(address-space): numeric array writes convert (checked) to the node element type; array-to-scalar refusal only for Modbus/S7 (#610 review R6)

Browse Source 
MAJOR-1: R4's exact element-type match refused ScadaBridge's CLR-list
writes (its element types are only Boolean/Int32/Float/Double/String/
DateTime) to every Int16/UInt16/Byte/UInt32/Int64/UInt64 array node and a
Double list to a Float array. EvaluateEquipmentWriteStructure now converts
a numeric array of another numeric element type element-wise, CHECKED, into
the node's element type and hands the converted array back through
OnEquipmentTagWrite's ref value, so the node stores and the driver receives
the node's own type. Any out-of-range, fractional-to-integer, NaN/Inf-to-
integer or Float-overflowing element refuses the whole write BadTypeMismatch
(nothing dispatched) - so the UInt32 -> Int32 bit reinterpretation stays
closed. Non-numeric mismatches stay refused. The wrong 'no client relies on
a looser rule' comment is corrected.

MINOR-1: the array-value-to-scalar-node refusal broke writes back to
browse-committed array tags (committed scalar, isArray never set) on
pass-through drivers. It now applies only when the owning driver's scalar
encoder cannot take an array (DriverArrayWriteSupport.
RefusesArrayValueOnScalarTag: Modbus, S7), carried RawTagPlan.
ScalarRefusesArrayValue / UnsReferenceVariable.BackingScalarRefusesArrayValue
-> EnsureVariable(refuseArrayOnScalar) -> a per-node set in the node
manager. OpcUaClient/Galaxy/others keep pre-#610 dispatch. Not on the
classifier whitelist (driver-type edit rebuilds); a mirror whose flag
changes is recreated, not updated in place.

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
2026-10-08 08:31:46 -04:00
parent a702875d41
commit 52708bff52
27 changed files with 509 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+160 -23
View File
@@ -1,4 +1,5 @@
using System.Collections.Concurrent;
using System.Globalization;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Opc.Ua;
@@ -69,6 +70,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// (later) HistoryRead override resolves a HistoryRead request's NodeId against this map. Cleared on
/// <see cref="RebuildAddressSpace"/>.</summary>
private readonly ConcurrentDictionary<RealmKey, string> _historizedTagnames = new();
/// <summary>Gitea #610 review (R6 MINOR-1): the variables whose owning driver cannot write an array value to a
/// SCALAR tag (Modbus / S7 — <c>DriverArrayWriteSupport.RefusesArrayValueOnScalarTag</c>), so an array write to
/// one while it is scalar-shaped is refused inline. Every other scalar node keeps the pre-#610 behaviour (the
/// write is dispatched and the driver decides). Set by <see cref="EnsureVariable"/>; cleared alongside
/// <see cref="_variables"/>.</summary>
private readonly ConcurrentDictionary<RealmKey, byte> _arrayOnScalarRefused = new();
/// <summary>Folders we have already promoted to event-notifiers + registered as root notifiers,
/// so repeated <see cref="MaterialiseAlarmCondition"/> calls don't double-add (idempotent guard).
/// Keyed by NodeId → the actual <see cref="FolderState"/> so <see cref="RebuildAddressSpace"/> can
@@ -1921,8 +1928,14 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// synchronously so the SDK never applies it (no optimistic-Good-then-revert round-trip + no needless
// device dispatch). Runs AFTER the authz gate (so we never leak structure detail to an unauthorised
// caller) but BEFORE the optimistic dispatch below.
var structure = EvaluateEquipmentWriteRange(indexRange) ?? EvaluateEquipmentWriteStructure(value, node);
// A numeric array of another element type is converted (checked) to the node's element type here and
// the converted array replaces `value`, so the SDK stores — and the driver receives — the node's own type.
var refuseArrayOnScalar = _arrayOnScalarRefused.ContainsKey(MapKey(node.NodeId));
var range = EvaluateEquipmentWriteRange(indexRange);
if (range is not null) return range;
var structure = EvaluateEquipmentWriteStructure(value, node, refuseArrayOnScalar, out var coerced);
if (structure is not null) return structure;
value = coerced!;
// Capture the optimistic value + the REAL prior value/status BEFORE the SDK applies the write
// (at handler entry the node still holds the prior value; returning Good makes the SDK apply `value`).
@@ -2025,18 +2038,45 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// <c>BaseVariableState.WriteValueAttribute</c> runs <c>TypeInfo.IsInstanceOfDataType</c> only when
/// the node has NO <c>OnWriteValue</c> handler; with one (every writable equipment node) it checks the
/// access level, calls the handler and stores the value as given. So: an ARRAY write is matched
/// EXACTLY (shape, declared length and element type — the SDK's own rule, restored), and an array
/// value to a scalar node is refused. A SCALAR write keeps the looser numeric rule above on purpose
/// (see <see cref="IsBuiltInTypeCompatible"/>).
/// by shape and declared length, and its element type must either match the node's DataType or be
/// CONVERTIBLE to it: a numeric array of another numeric element type is converted element-wise, CHECKED,
/// into the node's element type (<see cref="TryConvertNumericArray"/>) — refused whole if any element is
/// out of range, fractional for an integer node, or NaN/±Infinity for an integer node — and every other
/// mismatch is refused. An array value to a scalar node is refused only when
/// <paramref name="refuseArrayOnScalar"/> says the owning driver cannot take one. A SCALAR write keeps
/// the looser numeric rule above on purpose (see <see cref="IsBuiltInTypeCompatible"/>).
/// </para>
/// Pure (no SDK server / Lock needed): reads only <paramref name="value"/> and the node's static
/// <c>DataType</c>, so it is unit-testable in isolation.
/// </summary>
/// <param name="value">The value the client wrote (the SDK's pre-coercion payload).</param>
/// <param name="node">The target node (expected to be a writable <see cref="BaseDataVariableState"/>).</param>
/// <param name="refuseArrayOnScalar">True when the node's owning driver cannot write an array value to a
/// scalar tag (Modbus / S7): an array value to a scalar node is then refused. False keeps the pre-#610
/// behaviour for that case (dispatch; the driver — e.g. OpcUaClient's pass-through — decides).</param>
/// <param name="coerced">On a <c>null</c> return, the value to store and dispatch: <paramref name="value"/>
/// itself, or for a numeric array of another element type, the checked conversion to the node's element type.
/// <c>null</c> when the write is refused.</param>
/// <returns><c>null</c> to proceed; otherwise the veto <see cref="ServiceResult"/>
/// (<c>BadTypeMismatch</c> for a null write or a confidently-detected built-in-type mismatch).</returns>
internal static ServiceResult? EvaluateEquipmentWriteStructure(object? value, NodeState node)
internal static ServiceResult? EvaluateEquipmentWriteStructure(
object? value, NodeState node, bool refuseArrayOnScalar, out object? coerced)
{
coerced = null;
var result = EvaluateEquipmentWriteStructureCore(value, node, refuseArrayOnScalar, ref coerced);
if (result is not null) coerced = null;
else coerced ??= value;
return result;
}
/// <summary>The verdict half of <see cref="EvaluateEquipmentWriteStructure(object?, NodeState, bool, out object?)"/>
/// for callers that only need the decision; the array-on-scalar refusal is applied as for a Modbus / S7 node.
/// Production goes through the four-argument overload, which also returns the converted value.</summary>
internal static ServiceResult? EvaluateEquipmentWriteStructure(object? value, NodeState node) =>
EvaluateEquipmentWriteStructure(value, node, refuseArrayOnScalar: true, out _);
private static ServiceResult? EvaluateEquipmentWriteStructureCore(
object? value, NodeState node, bool refuseArrayOnScalar, ref object? coerced)
{
// Minimum sensible check: a null payload is never a valid value write to a value variable.
if (value is null)
@@ -2073,10 +2113,14 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
if (expected is BuiltInType.Null or BuiltInType.Variant or BuiltInType.DataValue) return null; // unresolved / wildcard
// Gitea #610 review — the mirror case: an array value to a SCALAR node of a resolved DataType is refused
// up front rather than optimistically stored, dispatched to a scalar encoder, refused and reverted. A
// byte[] is exempt: on the wire it is a ByteString, which is a scalar (the type check below still decides
// whether the node takes one). An unresolved-DataType node deferred above, as it always has.
if (variable.ValueRank == ValueRanks.Scalar && value is Array and not byte[])
// up front rather than optimistically stored, dispatched to a scalar encoder, refused and reverted — but
// ONLY for a driver whose scalar encoder cannot take one (R6 MINOR-1). A browse-committed array attribute
// lands as a SCALAR tag (the /raw commit never sets isArray) whose driver publishes an array into it, and
// writing that array back worked before #610 on the pass-through drivers (OpcUaClient forwards the
// Variant; Galaxy encodes it), so those keep dispatching and the driver decides. A byte[] is exempt: on
// the wire it is a ByteString, which is a scalar (the type check below still decides whether the node
// takes one). An unresolved-DataType node deferred above, as it always has.
if (refuseArrayOnScalar && variable.ValueRank == ValueRanks.Scalar && value is Array and not byte[])
{
return new ServiceResult(StatusCodes.BadTypeMismatch, "a scalar node takes a scalar value");
}
@@ -2091,22 +2135,41 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// unclassifiable value ⇒ Unknown (Null).
var actual = TypeInfo.Construct(value).BuiltInType;
// Gitea #610 review (MAJOR-1) — an ARRAY write is checked EXACTLY, element type against DataType. The SDK
// runs its own TypeInfo.IsInstanceOfDataType check only when a node has NO OnWriteValue handler, and every
// writable equipment node has one, so nothing else stands between a mistyped array and the driver: a
// UInt32[] to a Galaxy Int32 array matched MxValueEncoder's int[] arm by CLR array covariance and wrote
// uint.MaxValue as -1 with a Good result, and a Modbus/S7 node kept serving an int[] under an Int16
// DataType. This restores the SDK's own rule for arrays — the element type must match, with the single
// Byte-array/ByteString equivalence (a Byte[] decodes as byte[], which classifies as ByteString) — and
// an unclassifiable element type (a Variant[] / object[]) is refused rather than deferred. Arrays were
// never client-writable before #610, so no client relies on a looser rule.
// Gitea #610 review (MAJOR-1, R6) — an ARRAY write's element type is checked against the DataType here,
// because the SDK runs its own TypeInfo.IsInstanceOfDataType check only when a node has NO OnWriteValue
// handler, and every writable equipment node has one: nothing else stands between a mistyped array and the
// driver. Unchecked, a UInt32[] to a Galaxy Int32 array matched MxValueEncoder's int[] arm by CLR array
// covariance and wrote 4,000,000,000 bit-reinterpreted with a Good result, and a Modbus/S7 node kept
// serving an int[] under an Int16 DataType.
// • A matching element type (or the Byte-array/ByteString equivalence — a Byte[] decodes as byte[], which
// classifies as ByteString) proceeds unchanged.
// • A NUMERIC array of another numeric element type is CONVERTED, checked, into the node's element type,
// and the converted array is what the SDK stores and the driver receives. This is not optional: the
// consumer #610 exists for, ScadaBridge, writes the CLR list it decoded from a List attribute, and its
// element types are only Boolean/Int32/Float/Double/String/DateTime — an exact-match rule left every
// Int16/UInt16/Byte/UInt32/Int64/UInt64 array node (Modbus's default Int16 included) and a Double list
// to a Float array permanently unwritable from it. Any element out of the target's range, fractional
// for an integer target, or NaN/±Infinity for an integer target refuses the WHOLE write — so the
// UInt32 → Int32 reinterpretation stays closed (4,000,000,000 overflows; in-range values convert).
// • Anything else — a non-numeric mismatch, or an unclassifiable element type (Variant[] / object[]) — is
// refused rather than deferred.
if (isArrayNode)
{
return actual == expected || (expected, actual) is (BuiltInType.Byte, BuiltInType.ByteString)
? null
: new ServiceResult(
if (actual == expected || (expected, actual) is (BuiltInType.Byte, BuiltInType.ByteString)) return null;
if (IsNumeric(expected) && IsNumeric(actual) && value is Array source)
{
if (TryConvertNumericArray(source, expected, out var converted))
{
coerced = converted;
return null;
}
return new ServiceResult(
StatusCodes.BadTypeMismatch,
$"array element type {actual} does not match node data type {expected}");
$"array element of type {actual} is not exactly representable as node data type {expected}");
}
return new ServiceResult(
StatusCodes.BadTypeMismatch,
$"array element type {actual} does not match node data type {expected}");
}
if (actual == BuiltInType.Null) return null; // couldn't classify the payload ⇒ defer to the SDK
@@ -2166,6 +2229,76 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
BuiltInType.Int32 or BuiltInType.UInt32 or BuiltInType.Int64 or BuiltInType.UInt64 or
BuiltInType.Float or BuiltInType.Double;
/// <summary>
/// #610 review R6 — CHECKED element-wise conversion of a numeric array to the node's numeric element type.
/// Fails (nothing converted) when ANY element is out of the target's range, is fractional or NaN/±Infinity
/// for an integer target, or is finite yet overflows to ±Infinity for a <see cref="BuiltInType.Float"/>
/// target. Precision loss inside the target's range (a Double to a Float, a large Int64 to a Double) is
/// accepted, as the scalar path's drivers accept it. Pure + static.
/// </summary>
/// <param name="source">The 1-D numeric array the client wrote.</param>
/// <param name="target">The node's numeric element type.</param>
/// <param name="converted">The converted array (element CLR type of <paramref name="target"/>), or null.</param>
/// <returns>True when every element converted exactly enough to be written.</returns>
internal static bool TryConvertNumericArray(Array source, BuiltInType target, out Array? converted)
{
converted = null;
var elementType = target switch
{
BuiltInType.SByte => typeof(sbyte), BuiltInType.Byte => typeof(byte),
BuiltInType.Int16 => typeof(short), BuiltInType.UInt16 => typeof(ushort),
BuiltInType.Int32 => typeof(int), BuiltInType.UInt32 => typeof(uint),
BuiltInType.Int64 => typeof(long), BuiltInType.UInt64 => typeof(ulong),
BuiltInType.Float => typeof(float), BuiltInType.Double => typeof(double),
_ => null,
};
if (elementType is null) return false;
var result = Array.CreateInstance(elementType, source.Length);
for (var i = 0; i < source.Length; i++)
{
if (!TryConvertNumericElement(source.GetValue(i), target, out var element)) return false;
result.SetValue(element, i);
}
converted = result;
return true;
}
private static bool TryConvertNumericElement(object? element, BuiltInType target, out object? result)
{
result = null;
if (element is null) return false;
var integerTarget = target is not (BuiltInType.Float or BuiltInType.Double);
if (element is float or double)
{
var d = Convert.ToDouble(element, CultureInfo.InvariantCulture);
// Convert.ToInt16(1.5) ROUNDS (banker's) rather than failing, so a fractional value is refused here.
if (integerTarget && (!double.IsFinite(d) || Math.Truncate(d) != d)) return false;
if (target == BuiltInType.Float && double.IsFinite(d) && float.IsInfinity((float)d)) return false;
}
try
{
result = target switch
{
BuiltInType.SByte => Convert.ToSByte(element, CultureInfo.InvariantCulture),
BuiltInType.Byte => Convert.ToByte(element, CultureInfo.InvariantCulture),
BuiltInType.Int16 => Convert.ToInt16(element, CultureInfo.InvariantCulture),
BuiltInType.UInt16 => Convert.ToUInt16(element, CultureInfo.InvariantCulture),
BuiltInType.Int32 => Convert.ToInt32(element, CultureInfo.InvariantCulture),
BuiltInType.UInt32 => Convert.ToUInt32(element, CultureInfo.InvariantCulture),
BuiltInType.Int64 => Convert.ToInt64(element, CultureInfo.InvariantCulture),
BuiltInType.UInt64 => Convert.ToUInt64(element, CultureInfo.InvariantCulture),
BuiltInType.Float => Convert.ToSingle(element, CultureInfo.InvariantCulture),
BuiltInType.Double => Convert.ToDouble(element, CultureInfo.InvariantCulture),
_ => null,
};
return result is not null;
}
catch (Exception ex) when (ex is OverflowException or InvalidCastException or FormatException)
{
return false;
}
}
/// <summary>The CLOSED set of expected built-in DataTypes the structural fail-fast is allowed to reject
/// against — exactly the types <see cref="ResolveBuiltInDataType"/> can emit for a writable equipment node
/// (the numeric families + Boolean / String / DateTime / ByteString). Any expected type OUTSIDE this set is
@@ -2510,7 +2643,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// <paramref name="dataType"/> — rank + dimensions carry the array-ness.</param>
/// <param name="arrayLength">Phase 4c: the declared length of the 1-D array when
/// <paramref name="isArray"/> is true; ignored for scalars. Null ⇒ length 0.</param>
public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, AddressSpaceRealm realm, string? historianTagname = null, bool isArray = false, uint? arrayLength = null)
public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, AddressSpaceRealm realm, string? historianTagname = null, bool isArray = false, uint? arrayLength = null, bool refuseArrayOnScalar = false)
{
ArgumentException.ThrowIfNullOrEmpty(variableNodeId);
ArgumentException.ThrowIfNullOrEmpty(displayName);
@@ -2564,6 +2697,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// NodeId back to its Aveva/historian source. A historized UNS reference node passes the SAME
// tagname as its backing raw node — each NodeId keys its own entry, both mapping to one tagname.
if (historized) _historizedTagnames[key] = historianTagname!;
if (refuseArrayOnScalar) _arrayOnScalarRefused[key] = 0;
}
}
@@ -2984,6 +3118,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
_variables.Clear();
// Phase C: drop the NodeId→historian-tagname registrations alongside the variables they map.
_historizedTagnames.Clear();
_arrayOnScalarRefused.Clear();
foreach (var alarm in _alarmConditions.Values)
{
@@ -3059,6 +3194,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
PredefinedNodes?.Remove(variable.NodeId);
// Drop the historized-tagname registration alongside the variable it maps (Phase C parity).
_historizedTagnames.TryRemove(key, out _);
_arrayOnScalarRefused.TryRemove(key, out _);
e = BuildNodesRemovedModelChange(variableNodeId, realm);
}
ReportModelChangeOutsideLock(e, variableNodeId);
@@ -3157,6 +3293,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
PredefinedNodes?.Remove(v.NodeId);
}
_historizedTagnames.TryRemove(id, out _);
_arrayOnScalarRefused.TryRemove(id, out _);
}
// Part 9 condition nodes (+ their native flags).