From ca6f38e789ade560b7b1458d6b43cad1dcf226ee Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 17:23:25 +0000 Subject: [PATCH] Fix bugs found in codebase-wide review, with regression tests Core infrastructure: - utils.js: CallbackPromise now observes rejections from async executors instead of discarding them, so throws inside async resolvers reject the promise rather than leaving it pending forever with an unhandledRejection; InvertKeyValues Map branch no longer relies on Iterator.prototype.forEach (missing on Node 20, the minimum supported engine) - Layer.js: context timeouts now remove their entry from __contextToCallbackTimeouts (previously leaked one entry per timed-out request); clearContexts materializes entries before clearing so callers no longer receive an always-empty live iterator - MultiplexLayer: layerContext called incrementContext(this) from a plain module function where `this` is undefined, so every multiplexed send threw a TypeError; duplicate disconnect() now returns after resolving - TCP layer: the socket 'error' handler never settled the connect promise, so a refused connection hung connected() forever; disconnect() during Connecting now destroys the in-flight socket instead of leaking the established connection; Scan never initiated a connection and therefore never yielded results; connected() resolves false when no connect was ever started - UDP layer: handleDestroy now closes the bound socket instead of only unref'ing it (the port stayed bound and receiving for the process life) Modbus: - PDU.Decode validates wire length fields (byte counts, exception codes, write echoes) against the received data; malformed frames now produce a decodable error that rejects the pending request instead of throwing an uncaught ERR_OUT_OF_RANGE from the socket data handler - readCoils/readDiscreteInputs unpack the packed bytes into the requested number of coil statuses (LSB-first) instead of resolving raw bytes with undefined pad bits - requests now arm a timeout (options.timeout, default 10s) so a dropped response rejects instead of pending forever and leaking its context - per-request unitID/protocolID use ?? instead of ||, so unitID 0 (broadcast) is no longer silently replaced by the default - constants: SerialLineFunctions renamed (deprecated misspelled alias retained) and merged into FunctionNames so serial-line responses no longer decode as 'Unknown' CIP core: - object.js: OptionalServiceList is class attribute 5 per CIP Vol 1 (was duplicated with OptionalAttributeList at 4); GetAttributeSingle fallback referenced CommonClassAttribute.getCode which does not exist (group object intended) - Identity: Status.configured reads bit 2 per CIP Vol 1 Table 5-2.3 (bit 3 is reserved and always decoded 0 on compliant devices) - datatypes: STRINGN honors the decoded character width instead of always decoding UTF-16LE; BOOL reads enough bytes to contain the addressed bit (positions 8+ always decoded false before); ARRAY/ABBREV_ARRAY EncodeSize sums per-element sizes so variable-size item types no longer throw mid-write or corrupt the stream - EPATH datatype segment: ARRAY encodeSize accounts for 1/2/4-byte bounds to match encodeTo (bounds > 255 threw a RangeError mid-encode) - service.js and Logix5000 objects/datatypes modules: add .js extensions to ESM imports (these modules threw ERR_MODULE_NOT_FOUND when imported); __shared.js exports SymbolType by name as symbol.js expects EIP / Logix5000 / PCCC: - CIPInternalLayer/CIPConnectionLayer: pass the required offsetRef to request.response(); every PCCC-over-CIP response previously threw a TypeError so no forwarded request could ever complete - CIPInternalLayer.exploreAttributes queries through maxAttribute inclusive (loop stopped one short) - EIP: connect() queues callbacks instead of overwriting, so a second connect while RegisterSession is in flight no longer silently drops the first caller's callback; host strings without a port parse correctly - Logix5000: getSymbolInstanceID strips member access from dotted tags (the stripping line was commented out, so writeTag on any dotted tag hung); writeTag rejects cleanly for unknown tags (the previous guard could never fire and the resulting TypeError was swallowed); readTemplateClassAttributes wraps its body so protocol errors reject; readTemplate clamps the per-fragment remaining-bytes field to 16 bits for definitions larger than 64KB - remove empty src/core/cipmodbus module; fix latent offset/this bugs inside the commented-out CIP Modbus layer Every fix is covered by a regression test that fails against the previous code (verified: 39 of the new/updated tests fail on the unfixed sources). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NzFBvKrXRNpxmUpeFLeWWF --- src/core/cipmodbus/index.js | 0 src/core/modbus/constants.js | 7 +- src/core/modbus/pdu.js | 56 +++++++++--- src/layers/Layer.js | 4 +- src/layers/cip/core/datatypes/decoding.js | 34 ++++++-- src/layers/cip/core/datatypes/encoding.js | 21 +++-- .../cip/core/epath/segments/datatype.js | 6 +- src/layers/cip/core/object.js | 4 +- src/layers/cip/core/objects/Identity.js | 3 +- src/layers/cip/core/service.js | 2 +- src/layers/cip/layers/EIP/index.js | 11 ++- .../cip/layers/Logix5000/datatypes/names.js | 4 +- .../cip/layers/Logix5000/datatypes/types.js | 2 +- src/layers/cip/layers/Logix5000/index.js | 77 ++++++++-------- .../cip/layers/Logix5000/objects/__shared.js | 10 ++- .../layers/Logix5000/objects/controller.js | 6 +- .../cip/layers/Logix5000/objects/index.js | 6 +- .../cip/layers/Logix5000/objects/symbol.js | 8 +- .../cip/layers/Logix5000/objects/template.js | 6 +- src/layers/cip/layers/Modbus.js | 8 +- .../cip/layers/internal/CIPConnectionLayer.js | 2 +- .../cip/layers/internal/CIPInternalLayer.js | 4 +- src/layers/extras/MultiplexLayer.js | 3 +- src/layers/modbus/index.js | 23 ++++- src/layers/tcp/index.js | 13 ++- src/layers/udp/index.js | 6 +- src/utils.js | 11 ++- test/unit/cip.datatypes.test.js | 75 ++++++++++++++++ test/unit/cip.epath.datatype.test.js | 15 ++++ test/unit/cip.internal.test.js | 74 ++++++++++++++++ test/unit/cip.objects.test.js | 60 +++++++++++++ test/unit/eip.layer.test.js | 28 ++++++ test/unit/layer.test.js | 41 +++++++++ test/unit/logix5000.layer.test.js | 87 +++++++++++++++++++ test/unit/modbus.layer.test.js | 59 ++++++++++++- test/unit/modbus.pdu.test.js | 54 ++++++++++++ test/unit/multiplex.layer.test.js | 57 ++++++++++++ test/unit/tcp.layer.test.js | 79 +++++++++++++++++ test/unit/udp.layer.test.js | 37 ++++++++ test/unit/utils.test.js | 60 ++++++++++++- 40 files changed, 947 insertions(+), 116 deletions(-) delete mode 100644 src/core/cipmodbus/index.js create mode 100644 test/unit/cip.internal.test.js create mode 100644 test/unit/cip.objects.test.js create mode 100644 test/unit/layer.test.js create mode 100644 test/unit/multiplex.layer.test.js create mode 100644 test/unit/udp.layer.test.js diff --git a/src/core/cipmodbus/index.js b/src/core/cipmodbus/index.js deleted file mode 100644 index e69de29..0000000 diff --git a/src/core/modbus/constants.js b/src/core/modbus/constants.js index d2e7384..23ea99e 100644 --- a/src/core/modbus/constants.js +++ b/src/core/modbus/constants.js @@ -15,7 +15,7 @@ export const Functions = Object.freeze({ EncapsulatedInterfaceTransport: 0x2B, }); -export const SearialLineFunctions = Object.freeze({ +export const SerialLineFunctions = Object.freeze({ ReadExceptionStatus: 0x07, Diagnostics: 0x08, GetCommEventCounter: 0x0B, @@ -23,7 +23,10 @@ export const SearialLineFunctions = Object.freeze({ ReportServerID: 0x11, }); -export const FunctionNames = InvertKeyValues(Functions); +/** @deprecated misspelled alias kept for backwards compatibility */ +export const SearialLineFunctions = SerialLineFunctions; + +export const FunctionNames = InvertKeyValues({ ...Functions, ...SerialLineFunctions }); export const ErrorDescriptions = Object.freeze({ 0x01: 'Illegal function', diff --git a/src/core/modbus/pdu.js b/src/core/modbus/pdu.js index bde134e..ef63034 100644 --- a/src/core/modbus/pdu.js +++ b/src/core/modbus/pdu.js @@ -83,43 +83,69 @@ export default class PDU { const fn = PDU.Fn(buffer, offsetRef); const data = PDU.Data(buffer, offsetRef, pduLength); + /** + * `data` comes off the wire — every length field inside it must be + * validated against the bytes actually received or a malformed frame + * turns into an out-of-range read that escapes the socket data handler + */ + const malformed = (reason) => ({ + code: undefined, + message: `Malformed Modbus response (fn ${fn}): ${reason}`, + }); + let error; let value; if (fn > 0x80) { - const errorCode = data.readUInt8(0); - error = { - code: errorCode, - message: ErrorDescriptions[errorCode] || 'Unknown error', - }; + if (data.length < 1) { + error = malformed('exception response missing exception code'); + } else { + const errorCode = data.readUInt8(0); + error = { + code: errorCode, + message: ErrorDescriptions[errorCode] || 'Unknown error', + }; + } } else { switch (fn) { case Functions.ReadCoils: case Functions.ReadDiscreteInputs: { + const dataLength = data.length < 1 ? -1 : data.readUInt8(0); + if (dataLength < 0 || data.length < 1 + dataLength) { + error = malformed(`byte count ${dataLength} exceeds received data (${data.length - 1} bytes)`); + break; + } value = []; - const dataLength = buffer.readUInt8(offsetRef.current + OFFSET_DATA); offsetRef.current += 1; for (let i = 0; i < dataLength; i++) { - value.push(buffer.readUInt8(offsetRef.current + OFFSET_DATA + i)); + value.push(data.readUInt8(1 + i)); } offsetRef.current += dataLength; break; } case Functions.ReadHoldingRegisters: case Functions.ReadInputRegisters: { + const dataLength = data.length < 1 ? -1 : data.readUInt8(0); + if (dataLength < 0 || dataLength % 2 !== 0 || data.length < 1 + dataLength) { + error = malformed(`register byte count ${dataLength} is odd or exceeds received data (${data.length - 1} bytes)`); + break; + } value = []; - const dataLength = buffer.readUInt8(offsetRef.current + OFFSET_DATA); offsetRef.current += 1; const count = dataLength / 2; for (let i = 0; i < count; i++) { - value.push(buffer.readUInt16BE(offsetRef.current + OFFSET_DATA + 2 * i)); + value.push(data.readUInt16BE(1 + 2 * i)); } offsetRef.current += dataLength; break; } case Functions.WriteSingleCoil: case Functions.WriteSingleHoldingRegister: { - const address = buffer.readUInt16BE(offsetRef.current + OFFSET_DATA); - const ivalue = buffer.readUInt16BE(offsetRef.current + OFFSET_DATA + 2); + if (data.length < 4) { + error = malformed(`expected 4 data bytes, received ${data.length}`); + break; + } + const address = data.readUInt16BE(0); + const ivalue = data.readUInt16BE(2); offsetRef.current += 4; value = { address, @@ -129,8 +155,12 @@ export default class PDU { } case Functions.WriteMultipleCoils: case Functions.WriteMultipleHoldingRegisters: { - const address = buffer.readUInt16BE(offsetRef.current + OFFSET_DATA); - const count = buffer.readUInt16BE(offsetRef.current + OFFSET_DATA + 2); + if (data.length < 4) { + error = malformed(`expected 4 data bytes, received ${data.length}`); + break; + } + const address = data.readUInt16BE(0); + const count = data.readUInt16BE(2); offsetRef.current += 4; value = { address, diff --git a/src/layers/Layer.js b/src/layers/Layer.js index 3043acb..1c38365 100644 --- a/src/layers/Layer.js +++ b/src/layers/Layer.js @@ -224,6 +224,7 @@ export default class Layer extends EventEmitter { if (timeout != null && timeout > 0) { const timeoutHandle = setTimeout(() => { this.__contextToCallback.delete(context); + this.__contextToCallbackTimeouts.delete(context); callback(`Timeout (${timeout}ms)`); }, timeout); @@ -268,7 +269,8 @@ export default class Layer extends EventEmitter { } clearContexts() { - const entries = this.__idContext.entries(); + /** materialize before clear() — Map iterators are live views */ + const entries = Array.from(this.__idContext.entries()); this.__idContext.clear(); return entries; } diff --git a/src/layers/cip/core/datatypes/decoding.js b/src/layers/cip/core/datatypes/decoding.js index cc5db45..2c2ae45 100644 --- a/src/layers/cip/core/datatypes/decoding.js +++ b/src/layers/cip/core/datatypes/decoding.js @@ -4,7 +4,6 @@ import convertToObject from './convertToObject.js'; import { getBits, - unsignedIntegerSize, decodeUnsignedInteger, } from '../../../../utils.js'; @@ -27,15 +26,23 @@ export function DecodeTypedData(buffer, offsetRef, dataType, ctx) { } switch (dataType.code) { - case DataTypeCodes.BOOL: + case DataTypeCodes.BOOL: { /** BOOL does not change the offset */ - value = decodeUnsignedInteger( - buffer, - offsetRef.current, - unsignedIntegerSize(dataType.position), - ); - value = getBits(value, dataType.position, dataType.position + 1) > 0; + const position = dataType.position || 0; + if (position >= 32) { + throw new Error(`BOOL bit position not supported: ${position}`); + } + /** read enough bytes to contain the bit at `position` */ + let size = 4; + if (position < 8) { + size = 1; + } else if (position < 16) { + size = 2; + } + value = decodeUnsignedInteger(buffer, offsetRef.current, size); + value = getBits(value, position, position + 1) > 0; break; + } case DataTypeCodes.SINT: value = buffer.readInt8(offsetRef.current); offsetRef.current += 1; break; @@ -84,7 +91,16 @@ export function DecodeTypedData(buffer, offsetRef, dataType, ctx) { const width = buffer.readUInt16LE(offsetRef.current); offsetRef.current += 2; const length = buffer.readUInt16LE(offsetRef.current); offsetRef.current += 2; const total = width * length; - value = buffer.toString('utf16le', offsetRef.current, offsetRef.current + total); offsetRef.current += total; + /** width is bytes per character */ + let encoding; + if (width === 1) { + encoding = 'latin1'; + } else if (width === 2) { + encoding = 'utf16le'; + } else { + throw new Error(`STRINGN character width not supported: ${width}`); + } + value = buffer.toString(encoding, offsetRef.current, offsetRef.current + total); offsetRef.current += total; break; } case DataTypeCodes.LTIME: diff --git a/src/layers/cip/core/datatypes/encoding.js b/src/layers/cip/core/datatypes/encoding.js index 8bdff58..d27d8f8 100644 --- a/src/layers/cip/core/datatypes/encoding.js +++ b/src/layers/cip/core/datatypes/encoding.js @@ -41,18 +41,27 @@ export function EncodeSize(dataType, value) { case DataTypeCodes.EPATH: return EPath.EncodeSize(dataType.padded, value); case DataTypeCodes.ARRAY: { + if (!Array.isArray(value)) { + throw new Error(`Value must be an array to determine encoding size. Received ${typeof value}`); + } + /** sum per element — item types like SHORT_STRING vary in size */ const bounds = (dataType.upperBound - dataType.lowerBound) + 1; - const size = EncodeSize(dataType.itemType, value[0]); - return bounds * size; + let size = 0; + for (let i = 0; i < bounds; i++) { + size += EncodeSize(dataType.itemType, value[i]); + } + return size; } - case DataTypeCodes.ABBREV_ARRAY: + case DataTypeCodes.ABBREV_ARRAY: { if (!Array.isArray(value)) { throw new Error(`Value must be an array to determine encoding size. Received ${typeof value}`); } - if (value.length === 0) { - return 0; + let size = 0; + for (let i = 0; i < value.length; i++) { + size += EncodeSize(dataType.itemType, value[i]); } - return value.length * EncodeSize(dataType.itemType, value[0]); + return size; + } case DataTypeCodes.STRUCT: { let size = 0; for (let i = 0; i < dataType.members.length; i++) { diff --git a/src/layers/cip/core/epath/segments/datatype.js b/src/layers/cip/core/epath/segments/datatype.js index 6e66fc3..050c205 100644 --- a/src/layers/cip/core/epath/segments/datatype.js +++ b/src/layers/cip/core/epath/segments/datatype.js @@ -71,7 +71,11 @@ function encodeSize(type) { } break; case DataTypeCodes.ARRAY: - size = 8 + encodeSize(type.itemType); + /** bounds encode with 1, 2, or 4 bytes each — must match encodeTo */ + size = 6 + + unsignedIntegerSize(type.lowerBound) + + unsignedIntegerSize(type.upperBound) + + encodeSize(type.itemType); break; case DataTypeCodes.ABBREV_STRUCT: size = 4; diff --git a/src/layers/cip/core/object.js b/src/layers/cip/core/object.js index 364285f..534de45 100644 --- a/src/layers/cip/core/object.js +++ b/src/layers/cip/core/object.js @@ -94,7 +94,7 @@ export default function CIPMetaObject(classCode, options) { }), (value) => value[1], )), - OptionalServiceList: new CIPAttribute.Class(4, 'Optional Service List', DataType.TRANSFORM( + OptionalServiceList: new CIPAttribute.Class(5, 'Optional Service List', DataType.TRANSFORM( DataType.STRUCT([ DataType.UINT, DataType.PLACEHOLDER((length) => DataType.ABBREV_ARRAY(DataType.UINT, length)), @@ -184,7 +184,7 @@ export default function CIPMetaObject(classCode, options) { instance = 0; } attributeID = ( - ClassAttributeGroup.getCode(attribute) || CommonClassAttribute.getCode(attribute) + ClassAttributeGroup.getCode(attribute) || CommonClassAttributeGroup.getCode(attribute) ); } else { /** instance attribute */ diff --git a/src/layers/cip/core/objects/Identity.js b/src/layers/cip/core/objects/Identity.js index 9780d87..62c9003 100644 --- a/src/layers/cip/core/objects/Identity.js +++ b/src/layers/cip/core/objects/Identity.js @@ -159,7 +159,8 @@ const InstanceAttribute = Object.freeze({ (value) => ({ code: value, owned: getBits(value, 0, 1), - configured: getBits(value, 3, 4), + /** CIP Vol 1 Table 5-2.3: Configured is bit 2 (bits 1 and 3 are reserved) */ + configured: getBits(value, 2, 3), extendedDeviceStatus: ExtendedDeviceStatusDescriptions[getBits(value, 4, 8)] || 'Vendor/Product specific', minorRecoverableFault: getBits(value, 8, 9), minorUnrecoverableFault: getBits(value, 9, 10), diff --git a/src/layers/cip/core/service.js b/src/layers/cip/core/service.js index d8a1984..6f0006e 100644 --- a/src/layers/cip/core/service.js +++ b/src/layers/cip/core/service.js @@ -1,6 +1,6 @@ /* eslint-disable max-classes-per-file */ -import CIPFeature from './feature'; +import CIPFeature from './feature.js'; class CIPService extends CIPFeature {} diff --git a/src/layers/cip/layers/EIP/index.js b/src/layers/cip/layers/EIP/index.js index 6b14e11..118df0c 100644 --- a/src/layers/cip/layers/EIP/index.js +++ b/src/layers/cip/layers/EIP/index.js @@ -70,7 +70,9 @@ function setupCallbacks(self) { if (packet.status.code === 0) { setConnectionState(self, 2); self._sessionHandle = packet.sessionHandle; - if (self._connectCallback) self._connectCallback(); + const connectCallbacks = self._connectCallbacks; + self._connectCallbacks = []; + connectCallbacks.forEach((connectCallback) => connectCallback()); self.sendNextMessage(); sendUserRequests(self); } else { @@ -192,6 +194,7 @@ export default class EIPLayer extends Layer { this._sessionHandle = 0; this._context = Buffer.alloc(8); this._userRequests = []; + this._connectCallbacks = []; setConnectionState(this, 0); setupCallbacks(this); @@ -236,7 +239,7 @@ export default class EIPLayer extends Layer { switch (typeof host) { case 'string': { const parts = host.split(':', 2); - if (parts.length === 0) { + if (parts.length === 1) { hosts.push({ host: parts[0] }); } else { hosts.push({ host: parts[0], port: parts[1] }); @@ -343,7 +346,9 @@ export default class EIPLayer extends Layer { if (callback) callback(); return; } - this._connectCallback = callback; + /** queue, don't overwrite — a connect while RegisterSession is in + * flight must not silently drop the earlier caller's callback */ + if (callback) this._connectCallbacks.push(callback); if (this._connectionState > 0) return; setConnectionState(this, 1); this.send(EIPPacket.RegisterSessionRequest(this._context), null, true); diff --git a/src/layers/cip/layers/Logix5000/datatypes/names.js b/src/layers/cip/layers/Logix5000/datatypes/names.js index ccd9d57..7037c9b 100644 --- a/src/layers/cip/layers/Logix5000/datatypes/names.js +++ b/src/layers/cip/layers/Logix5000/datatypes/names.js @@ -1,7 +1,7 @@ import { InvertKeyValues, -} from '../../../../../utils'; +} from '../../../../../utils.js'; -import DataTypeCodes from './codes'; +import DataTypeCodes from './codes.js'; export default InvertKeyValues(DataTypeCodes); diff --git a/src/layers/cip/layers/Logix5000/datatypes/types.js b/src/layers/cip/layers/Logix5000/datatypes/types.js index e9f7c21..16e1f1d 100644 --- a/src/layers/cip/layers/Logix5000/datatypes/types.js +++ b/src/layers/cip/layers/Logix5000/datatypes/types.js @@ -1,4 +1,4 @@ -import DataTypeCodes from './codes'; +import DataTypeCodes from './codes.js'; const DataType = Object.freeze({ Program() { diff --git a/src/layers/cip/layers/Logix5000/index.js b/src/layers/cip/layers/Logix5000/index.js index fc14396..ede1f1e 100644 --- a/src/layers/cip/layers/Logix5000/index.js +++ b/src/layers/cip/layers/Logix5000/index.js @@ -411,8 +411,8 @@ async function getSymbolInstanceID(layer, scope, tag) { let tagName; switch (typeof tag) { case 'string': - tagName = tag; - // tagName = tag.split('.')[0]; + /** the symbol list holds base names — strip member/element access */ + [tagName] = tag.split('.'); break; case 'number': return tag; @@ -830,13 +830,14 @@ export default class Logix5000 extends CIPLayer { SymbolInstanceAttributeCodes.Type, ]); - if (!tagInfo) { + const tagType = tagInfo ? tagInfo[SymbolInstanceAttributeCodes.Type] : undefined; + + /** getSymbolInfo returns an empty object when the symbol lookup fails */ + if (tagType == null) { resolver.reject(`Invalid tag: ${tag}`); return; } - const tagType = tagInfo[SymbolInstanceAttributeCodes.Type]; - if (tagType.atomic !== true) { resolver.reject('Writing to structure tags is not currently supported'); return; @@ -1141,7 +1142,9 @@ export default class Logix5000 extends CIPLayer { while (true) { reqData.writeUInt32LE(reqOffset, 0); - reqData.writeUInt16LE(bytesToRead - reqOffset, 4); + /** remaining bytes can exceed 16 bits for very large templates; + * the device replies with a partial transfer and we continue */ + reqData.writeUInt16LE(Math.min(bytesToRead - reqOffset, 0xFFFF), 4); const reply = await sendPromise(this, service, path, reqData, 5000); chunks.push(reply.data); @@ -1204,45 +1207,49 @@ export default class Logix5000 extends CIPLayer { readTemplateClassAttributes(callback) { return CallbackPromise(callback, async (resolver) => { - const service = CommonServiceCodes.GetAttributeList; + try { + const service = CommonServiceCodes.GetAttributeList; - const path = EPath.Encode(true, [ - new EPath.Segments.Logical.ClassID(Logix5000ClassCodes.Template), - new EPath.Segments.Logical.InstanceID(0), - ]); + const path = EPath.Encode(true, [ + new EPath.Segments.Logical.ClassID(Logix5000ClassCodes.Template), + new EPath.Segments.Logical.InstanceID(0), + ]); - const reply = await sendPromise(this, service, path, encodeAttributes([ - TemplateClassAttributeCodes.Unknown1, - TemplateClassAttributeCodes.Unknown2, - TemplateClassAttributeCodes.Unknown3, - TemplateClassAttributeCodes.Unknown8, - /** 9 timed out?? */ - ])); + const reply = await sendPromise(this, service, path, encodeAttributes([ + TemplateClassAttributeCodes.Unknown1, + TemplateClassAttributeCodes.Unknown2, + TemplateClassAttributeCodes.Unknown3, + TemplateClassAttributeCodes.Unknown8, + /** 9 timed out?? */ + ])); - const { data } = reply; + const { data } = reply; - const offsetRef = { current: 0 }; - const attributeCount = data.readUInt16LE(offsetRef.current); offsetRef.current += 2; - const attributes = {}; + const offsetRef = { current: 0 }; + const attributeCount = data.readUInt16LE(offsetRef.current); offsetRef.current += 2; + const attributes = {}; - for (let i = 0; i < attributeCount; i++) { - const attribute = DecodeTypedData(data, offsetRef, DataType.UINT); - const status = DecodeTypedData(data, offsetRef, DataType.UINT); + for (let i = 0; i < attributeCount; i++) { + const attribute = DecodeTypedData(data, offsetRef, DataType.UINT); + const status = DecodeTypedData(data, offsetRef, DataType.UINT); - const attributeDataType = TemplateClassAttributeDataTypes[attribute]; + const attributeDataType = TemplateClassAttributeDataTypes[attribute]; - if (attributeDataType == null) { - throw new Error(`Unknown template attribute received: ${attribute}`); - } + if (attributeDataType == null) { + throw new Error(`Unknown template attribute received: ${attribute}`); + } - if (status === 0) { - attributes[attribute] = DecodeTypedData(data, offsetRef, attributeDataType); - } else { - throw new Error(`Attribute ${attribute} has error status: ${GenericServiceStatusDescriptions[status] || 'Unknown'}`); + if (status === 0) { + attributes[attribute] = DecodeTypedData(data, offsetRef, attributeDataType); + } else { + throw new Error(`Attribute ${attribute} has error status: ${GenericServiceStatusDescriptions[status] || 'Unknown'}`); + } } - } - resolver.resolve(attributes); + resolver.resolve(attributes); + } catch (err) { + resolver.reject(err); + } }); } diff --git a/src/layers/cip/layers/Logix5000/objects/__shared.js b/src/layers/cip/layers/Logix5000/objects/__shared.js index d497181..96dc752 100644 --- a/src/layers/cip/layers/Logix5000/objects/__shared.js +++ b/src/layers/cip/layers/Logix5000/objects/__shared.js @@ -1,7 +1,7 @@ -import { getBits } from '../../../../../utils'; -import Logix5000DataType from '../datatypes/codes'; -import Logix5000DatatypeNames from '../datatypes/names'; -import { DataType, DataTypeCodes, DataTypeNames } from '../../../core/datatypes'; +import { getBits } from '../../../../../utils.js'; +import Logix5000DataType from '../datatypes/codes.js'; +import Logix5000DatatypeNames from '../datatypes/names.js'; +import { DataType, DataTypeCodes, DataTypeNames } from '../../../core/datatypes/index.js'; class SymbolType { constructor(code) { @@ -37,6 +37,8 @@ class SymbolType { } } +export { SymbolType }; + export default { SymbolType, }; diff --git a/src/layers/cip/layers/Logix5000/objects/controller.js b/src/layers/cip/layers/Logix5000/objects/controller.js index 04e2e19..75ede03 100644 --- a/src/layers/cip/layers/Logix5000/objects/controller.js +++ b/src/layers/cip/layers/Logix5000/objects/controller.js @@ -1,6 +1,6 @@ -import CIPMetaObject from '../../../core/object'; -import CIPAttribute from '../../../core/attribute'; -import { DataType } from '../../../core/datatypes'; +import CIPMetaObject from '../../../core/object.js'; +import CIPAttribute from '../../../core/attribute.js'; +import { DataType } from '../../../core/datatypes/index.js'; const InstanceAttribute = Object.freeze({ Unknown1: new CIPAttribute.Instance(1, 'Unknown1', DataType.UINT), diff --git a/src/layers/cip/layers/Logix5000/objects/index.js b/src/layers/cip/layers/Logix5000/objects/index.js index 24ae0ad..e29971a 100644 --- a/src/layers/cip/layers/Logix5000/objects/index.js +++ b/src/layers/cip/layers/Logix5000/objects/index.js @@ -1,6 +1,6 @@ -import Controller from './controller'; -import SymbolObject from './symbol'; -import Template from './template'; +import Controller from './controller.js'; +import SymbolObject from './symbol.js'; +import Template from './template.js'; export default { Controller, diff --git a/src/layers/cip/layers/Logix5000/objects/symbol.js b/src/layers/cip/layers/Logix5000/objects/symbol.js index 60eb02c..4ec0ee7 100644 --- a/src/layers/cip/layers/Logix5000/objects/symbol.js +++ b/src/layers/cip/layers/Logix5000/objects/symbol.js @@ -1,7 +1,7 @@ -import CIPMetaObject from '../../../core/object'; -import CIPAttribute from '../../../core/attribute'; -import { DataType } from '../../../core/datatypes'; -import { SymbolType } from './__shared'; +import CIPMetaObject from '../../../core/object.js'; +import CIPAttribute from '../../../core/attribute.js'; +import { DataType } from '../../../core/datatypes/index.js'; +import { SymbolType } from './__shared.js'; const InstanceAttribute = Object.freeze({ Name: new CIPAttribute.Instance(1, 'Name', DataType.STRING), diff --git a/src/layers/cip/layers/Logix5000/objects/template.js b/src/layers/cip/layers/Logix5000/objects/template.js index 3611514..68c773c 100644 --- a/src/layers/cip/layers/Logix5000/objects/template.js +++ b/src/layers/cip/layers/Logix5000/objects/template.js @@ -1,6 +1,6 @@ -import CIPMetaObject from '../../../core/object'; -import CIPAttribute from '../../../core/attribute'; -import { DataType } from '../../../core/datatypes'; +import CIPMetaObject from '../../../core/object.js'; +import CIPAttribute from '../../../core/attribute.js'; +import { DataType } from '../../../core/datatypes/index.js'; const InstanceAttribute = Object.freeze({ StructureHandle: new CIPAttribute.Instance(1, 'StructureHandle', DataType.UINT), /** Calculated CRC value for members of the structure */ diff --git a/src/layers/cip/layers/Modbus.js b/src/layers/cip/layers/Modbus.js index 6fd503c..e76829a 100644 --- a/src/layers/cip/layers/Modbus.js +++ b/src/layers/cip/layers/Modbus.js @@ -93,7 +93,7 @@ // buffer.writeUInt16LE(address, 0); // buffer.writeUInt16LE(values.length, 2); // for (let i = 0; i < values.length; i++) { -// buffer.writeUInt16LE(values[i]); +// buffer.writeUInt16LE(values[i], 4 + 2 * i); // } // send(this, Services.WriteHoldingRegisters, buffer, (error, reply) => { @@ -139,7 +139,7 @@ // /** Use driver specific error handling if exists */ // function send(self, service, data, callback) { -// self.sendRequest(false, new CIPRequest(service, MODBUS_EPATH, data), this.contextCallback(callback)); +// self.sendRequest(false, new CIPRequest(service, MODBUS_EPATH, data), self.contextCallback(callback)); // } // const Services = Object.freeze({ @@ -245,7 +245,7 @@ // // buffer.writeUInt16LE(address, 0); // // buffer.writeUInt16LE(values.length, 2); // // for (let i = 0; i < values.length; i++) { -// // buffer.writeUInt16LE(values[i]); +// // buffer.writeUInt16LE(values[i], 4 + 2 * i); // // } // // send(this, Services.WriteHoldingRegisters, buffer, (error, reply) => { @@ -292,7 +292,7 @@ // // /** Use driver specific error handling if exists */ // // function send(self, service, data, callback) { -// // self.sendRequest(false, new CIPRequest(service, MODBUS_EPATH, data), this.contextCallback(callback)); +// // self.sendRequest(false, new CIPRequest(service, MODBUS_EPATH, data), self.contextCallback(callback)); // // } // // const Services = Object.freeze({ diff --git a/src/layers/cip/layers/internal/CIPConnectionLayer.js b/src/layers/cip/layers/internal/CIPConnectionLayer.js index 628be1d..50359f8 100644 --- a/src/layers/cip/layers/internal/CIPConnectionLayer.js +++ b/src/layers/cip/layers/internal/CIPConnectionLayer.js @@ -294,7 +294,7 @@ function handleConnectedMessage(self, data, info) { if (savedContext.internal) { const callback = self.callbackForContext(savedContext.context); if (callback != null) { - const response = savedContext.request.response(data); + const response = savedContext.request.response(data, { current: 0 }); callback( response.status.error ? response.status.description || 'CIP Error' : null, diff --git a/src/layers/cip/layers/internal/CIPInternalLayer.js b/src/layers/cip/layers/internal/CIPInternalLayer.js index b86c520..9b54b24 100644 --- a/src/layers/cip/layers/internal/CIPInternalLayer.js +++ b/src/layers/cip/layers/internal/CIPInternalLayer.js @@ -65,7 +65,7 @@ class CIPInternalLayer extends Layer { const attributes = []; - for (let i = 1; i < maxAttribute; i++) { + for (let i = 1; i <= maxAttribute; i++) { try { const path = EPath.Encode(true, [ new EPath.Segments.Logical.ClassID(classCode), @@ -112,7 +112,7 @@ class CIPInternalLayer extends Layer { handleData(data, info, context) { if (context && context.internal === false) { - const response = context.request.response(data); + const response = context.request.response(data, { current: 0 }); // console.log(response); if (context.type === 'pccc') { this.forward( diff --git a/src/layers/extras/MultiplexLayer.js b/src/layers/extras/MultiplexLayer.js index 75083b0..b65ca3c 100644 --- a/src/layers/extras/MultiplexLayer.js +++ b/src/layers/extras/MultiplexLayer.js @@ -14,7 +14,7 @@ function incrementContext(self) { function layerContext(self, layer, context) { if (layer != null) { if (context == null) { - context = incrementContext(this); // eslint-disable-line no-param-reassign + context = incrementContext(self); // eslint-disable-line no-param-reassign } self.__contextToLayer.set(context, layer); } @@ -49,6 +49,7 @@ export default class MultiplexLayer extends Layer { return CallbackPromise(callback, async (resolver) => { if (this._disconnecting === 1) { resolver.resolve(); + return; } this._disconnecting = 1; diff --git a/src/layers/modbus/index.js b/src/layers/modbus/index.js index 5f710da..b8e3856 100644 --- a/src/layers/modbus/index.js +++ b/src/layers/modbus/index.js @@ -20,6 +20,15 @@ const DefaultOptions = { }, }; +/** Coil/discrete-input responses pack one status per bit, LSB first */ +function unpackBits(bytes, count) { + const values = []; + for (let i = 0; i < count; i++) { + values.push(((bytes[i >> 3] >> (i & 0b111)) & 1) === 1); + } + return values; +} + function readRequest(self, fn, address, count, callback) { if (typeof count === 'function' && callback == null) { callback = count; // eslint-disable-line no-param-reassign @@ -28,8 +37,13 @@ function readRequest(self, fn, address, count, callback) { if (count == null) { count = 1; // eslint-disable-line no-param-reassign } + const unpacksBits = fn === ReadCoils || fn === ReadDiscreteInputs; return CallbackPromise(callback, (resolver) => { - self._send(PDU.EncodeReadRequest(fn, address, count), {}, resolver); + const sendResolver = unpacksBits ? { + resolve: (bytes) => resolver.resolve(unpackBits(bytes, count)), + reject: resolver.reject, + } : resolver; + self._send(PDU.EncodeReadRequest(fn, address, count), {}, sendResolver); }); } @@ -48,6 +62,7 @@ export default class Modbus extends Layer { const cOpts = { unitID: 0xFF, protocolID: 0, + timeout: 10000, ...options, }; @@ -67,12 +82,14 @@ export default class Modbus extends Layer { return resolver; }), this._transactionCounter, + cOpts.timeout, ); + /** ?? not || — unitID 0 (broadcast) and protocolID 0 are valid overrides */ const message = Frames.TCP.Encode( this._transactionCounter, - opts.protocolID || cOpts.protocolID, - opts.unitID || cOpts.unitID, + opts.protocolID ?? cOpts.protocolID, + opts.unitID ?? cOpts.unitID, pdu, ); diff --git a/src/layers/tcp/index.js b/src/layers/tcp/index.js index b4f1cf2..70f4c7d 100644 --- a/src/layers/tcp/index.js +++ b/src/layers/tcp/index.js @@ -90,10 +90,14 @@ function connect(layer) { socket.once('error', (err) => { setConnectionState(layer, TCPStateCodes.Disconnected, err); layer.destroy(err); + /** a refused/failed connection emits 'error', never 'timeout' — + * the connect promise must still settle or connected() hangs */ + resolve(false); }); socket.once('close', () => { setConnectionState(layer, TCPStateCodes.Disconnected); + resolve(false); }); socket.once('timeout', () => { @@ -168,7 +172,7 @@ export default class TCPLayer extends Layer { connectTimeout: 500, }); - if (await layer.connected()) { // eslint-disable-line no-await-in-loop + if (await connect(layer)) { // eslint-disable-line no-await-in-loop yield { host, port }; } @@ -179,7 +183,7 @@ export default class TCPLayer extends Layer { connected(callback) { return CallbackPromise(callback, async (resolver) => { - resolver.resolve(await this._connect); + resolver.resolve(this._connect != null ? await this._connect : false); }); } @@ -206,6 +210,11 @@ export default class TCPLayer extends Layer { this._disconnect = CallbackPromise(callback, async (resolver) => { if (this._connectionState === TCPStateCodes.Connecting) { setConnectionState(this, TCPStateCodes.Disconnected); + /** abort the in-flight handshake or the OS completes it later + * and the established socket leaks */ + if (this.socket && !this.socket.destroyed) { + this.socket.destroy(); + } resolver.resolve(); } else if (this._connectionState === TCPStateCodes.Connected) { // console.log(`TCP layer queue size at disconnect: ${this.requestQueueSize()}`); diff --git a/src/layers/udp/index.js b/src/layers/udp/index.js index 465df64..7ba533c 100644 --- a/src/layers/udp/index.js +++ b/src/layers/udp/index.js @@ -140,8 +140,12 @@ export default class UDPLayer extends Layer { handleDestroy() { this._listening = false; if (this.socket) { - this.socket.unref(); removeSocketListeners(this.socket); + try { + this.socket.close(); + } catch (err) { + /** already closed */ + } this.socket = null; } } diff --git a/src/utils.js b/src/utils.js index dfb4e9c..7116ba8 100644 --- a/src/utils.js +++ b/src/utils.js @@ -59,9 +59,9 @@ export function InvertKeyValues(obj) { break; case '[object Map]': inverted = new Map(); - obj.entries().forEach(([key, value]) => { + for (const [key, value] of obj.entries()) { inverted.set(value, key); - }); + } break; default: break; @@ -150,7 +150,12 @@ export function CallbackPromise(callback, func, timeout) { } try { - return func(resolver); + const result = func(resolver); + if (result != null && typeof result.then === 'function') { + /** an async func's rejection must settle the promise, not vanish */ + result.then(undefined, (err) => resolver.reject(err)); + } + return result; } catch (err) { resolver.reject(err); return resolver; diff --git a/test/unit/cip.datatypes.test.js b/test/unit/cip.datatypes.test.js index 20a716c..7c14d31 100644 --- a/test/unit/cip.datatypes.test.js +++ b/test/unit/cip.datatypes.test.js @@ -454,3 +454,78 @@ describe('Constructed', () => { assert.equal(code, 0x43F4); }); }); + +describe('STRINGN decoding', () => { + test('decodes a width-1 STRINGN as single-byte characters', () => { + /** STRINGN was always decoded as UTF-16LE, so any width other than 2 + * silently produced garbage */ + const offsetRef = { current: 0 }; + const value = DecodeTypedData( + Buffer.from('0100 0400 41424344'.replace(/\s+/g, ''), 'hex'), + offsetRef, + DataType.STRINGN, + ); + assert.equal(value, 'ABCD'); + assert.equal(offsetRef.current, 8); + }); + + test('decodes a width-2 STRINGN as UTF-16LE', () => { + const offsetRef = { current: 0 }; + const value = DecodeTypedData( + Buffer.from('0200 0200 41004200'.replace(/\s+/g, ''), 'hex'), + offsetRef, + DataType.STRINGN, + ); + assert.equal(value, 'AB'); + assert.equal(offsetRef.current, 8); + }); +}); + +describe('BOOL decoding with bit positions', () => { + test('reads a bit at position >= 8', () => { + /** the read size was derived from the byte-width of the position + * number itself, so positions 8-255 read only one byte and always + * decoded as false */ + const value = DecodeTypedData( + Buffer.from([0x00, 0x01]), + { current: 0 }, + DataType.BOOL(8), + ); + assert.equal(value, true); + }); + + test('reads a bit at position < 8', () => { + assert.equal( + DecodeTypedData(Buffer.from([0b0000_0100]), { current: 0 }, DataType.BOOL(2)), + true, + ); + assert.equal( + DecodeTypedData(Buffer.from([0b0000_0100]), { current: 0 }, DataType.BOOL(3)), + false, + ); + }); +}); + +describe('Array encode sizing with variable-size item types', () => { + test('ABBREV_ARRAY of SHORT_STRING sums per-element sizes', () => { + /** EncodeSize used value[0]'s size for every element, so mixed-length + * strings either threw mid-write or corrupted the stream */ + assert.deepEqual( + Encode(DataType.ABBREV_ARRAY(DataType.SHORT_STRING), ['ab', 'longer']), + Buffer.concat([ + Buffer.from([2]), Buffer.from('ab', 'ascii'), + Buffer.from([6]), Buffer.from('longer', 'ascii'), + ]), + ); + }); + + test('ARRAY of SHORT_STRING sums per-element sizes', () => { + assert.deepEqual( + Encode(DataType.ARRAY(DataType.SHORT_STRING, 0, 1), ['ab', 'wxyz']), + Buffer.concat([ + Buffer.from([2]), Buffer.from('ab', 'ascii'), + Buffer.from([4]), Buffer.from('wxyz', 'ascii'), + ]), + ); + }); +}); diff --git a/test/unit/cip.epath.datatype.test.js b/test/unit/cip.epath.datatype.test.js index 8297d39..aa205db 100644 --- a/test/unit/cip.epath.datatype.test.js +++ b/test/unit/cip.epath.datatype.test.js @@ -91,3 +91,18 @@ describe('Decoding', () => { assert.equal(offsetRef.current, 3); }); }); + +describe('ARRAY segment size/write agreement', () => { + test('bounds larger than one byte encode within the reported size', () => { + /** encodeSize() assumed 1-byte bounds while encodeTo() wrote 1, 2, or + * 4 bytes per bound, so any bound > 255 threw a RangeError mid-encode + * (or corrupted a larger EPATH buffer) */ + const segment = new DataTypeSegment(DataType.ARRAY(DataType.UINT, 0, 300, 0xC7, 0xC7)); + const encoded = segment.encode(); + assert.equal(encoded.length, segment.encodeSize()); + assert.deepEqual( + encoded, + Buffer.from([0xA3, 0x08, 0xC7, 0x01, 0x00, 0xC7, 0x02, 0x2C, 0x01, 0xC7]), + ); + }); +}); diff --git a/test/unit/cip.internal.test.js b/test/unit/cip.internal.test.js new file mode 100644 index 0000000..6270a06 --- /dev/null +++ b/test/unit/cip.internal.test.js @@ -0,0 +1,74 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import CIPInternalLayer from '../../src/layers/cip/layers/internal/CIPInternalLayer.js'; +import PCCCLayer from '../../src/layers/pccc/index.js'; +import ScriptedTransport from '../harness/ScriptedTransport.js'; +import LogixResponder from '../harness/LogixResponder.js'; + +function hex(s) { + return Buffer.from(s.replace(/\s+/g, ''), 'hex'); +} + +function withTimeout(promise, label, ms = 2000) { + return new Promise((resolve, reject) => { + const handle = setTimeout(() => { + reject(new Error(`${label} did not settle within ${ms}ms`)); + }, ms); + promise.then( + (value) => { clearTimeout(handle); resolve(value); }, + (err) => { clearTimeout(handle); reject(err); }, + ); + }); +} + +describe('PCCC forwarded over the CIP layer', () => { + it('decodes the Execute-PCCC response and completes the request', async () => { + const transport = new ScriptedTransport(); + const cip = new CIPInternalLayer(transport); + const pccc = new PCCCLayer(cip); + + transport.onNextRequest((message, t, request) => { + /** Execute PCCC (0x4B) reply: echoed request header (7 bytes, + * length-prefixed) followed by the PCCC reply */ + t.deliver(Buffer.concat([ + hex('cb 00 00 00'), + hex('07 cdab 78563412'), + hex('4f00 0100 42 0500'), + ]), null, request.context); + }); + + /** the response was decoded without an offsetRef, so every PCCC + * response over CIP threw a TypeError and no request ever completed */ + const value = await withTimeout(pccc.typedRead('N7:1'), 'typedRead over CIP'); + assert.equal(value, 5); + + /** the outgoing request must be Execute PCCC to the PCCC object */ + const sent = transport.sent[0]; + assert.equal(sent.readUInt8(0), 0x4B); + assert.deepEqual(sent.subarray(2, 6), hex('2067 2401')); + }); +}); + +describe('CIPInternalLayer exploreAttributes', () => { + it('queries every attribute up to and including maxAttribute', async () => { + const transport = new ScriptedTransport(); + const responder = new LogixResponder(transport); + const cip = new CIPInternalLayer(transport); + + /** the loop stopped at maxAttribute - 1 */ + responder.replyToConnected(hex('8e 00 00 00 01')); + responder.replyToConnected(hex('8e 00 00 00 02')); + responder.replyToConnected(hex('8e 00 00 00 03')); + + const attributes = await withTimeout( + cip.exploreAttributes(0x01, 1, 3), + 'exploreAttributes', + ); + + assert.deepEqual(attributes.map((attribute) => attribute.code), [1, 2, 3]); + assert.equal(responder.connectedRequests.length, 3); + + await transport.close(); + }); +}); diff --git a/test/unit/cip.objects.test.js b/test/unit/cip.objects.test.js new file mode 100644 index 0000000..6372d0d --- /dev/null +++ b/test/unit/cip.objects.test.js @@ -0,0 +1,60 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import Identity from '../../src/layers/cip/core/objects/Identity.js'; + +describe('CIP common class attributes', () => { + it('OptionalAttributeList and OptionalServiceList have distinct codes (CIP Vol 1)', () => { + const common = Identity.CommonClassAttribute; + /** both were declared with code 4, so requesting the service list + * fetched the attribute list and code-4 responses decoded under the + * wrong name */ + assert.equal(common.OptionalAttributeList.code, 4); + assert.equal(common.OptionalServiceList.code, 5); + }); + + it('GetClassAttribute encodes the Optional Service List attribute as 5', () => { + const request = Identity.GetClassAttribute( + Identity.CommonClassAttribute.OptionalServiceList, + ); + const path = request.encode().subarray(2); + /** class 0x01, instance 0, attribute 5 */ + assert.deepEqual(path, Buffer.from([0x20, 0x01, 0x24, 0x00, 0x30, 0x05])); + }); + + it('GetAttributeSingle resolves common class attributes', () => { + /** the fallback referenced CommonClassAttribute.getCode, which does + * not exist (the group object was intended) */ + const request = Identity.GetAttributeSingle(Identity.CommonClassAttribute.Revision); + assert.deepEqual( + request.encode().subarray(2), + Buffer.from([0x20, 0x01, 0x24, 0x00, 0x30, 0x01]), + ); + }); +}); + +describe('Identity Status attribute (CIP Vol 1 Table 5-2.3)', () => { + function decodeStatus(word) { + const buffer = Buffer.alloc(2); + buffer.writeUInt16LE(word, 0); + return Identity.DecodeInstanceAttribute( + buffer, + { current: 0 }, + Identity.InstanceAttribute.Status, + ); + } + + it('reads Configured from bit 2', () => { + /** Configured was read from bit 3, which is reserved, so compliant + * devices always decoded as unconfigured */ + assert.equal(decodeStatus(0b0100).configured, 1); + }); + + it('does not read Configured from reserved bit 3', () => { + assert.equal(decodeStatus(0b1000).configured, 0); + }); + + it('reads Owned from bit 0', () => { + assert.equal(decodeStatus(0b0001).owned, 1); + }); +}); diff --git a/test/unit/eip.layer.test.js b/test/unit/eip.layer.test.js index 0d6861d..3fb9b88 100644 --- a/test/unit/eip.layer.test.js +++ b/test/unit/eip.layer.test.js @@ -145,3 +145,31 @@ describe('EIP layer', () => { await transport.close(); }); }); + +describe('EIP layer connect callbacks', () => { + it('invokes every callback queued while RegisterSession is in flight', async () => { + const transport = new ScriptedTransport(); + const layer = new EIPLayer(transport); + + let deliverResponse; + transport.onNextRequest((message, t) => { + deliverResponse = () => t.deliver( + hex('6500 0400 44332211 00000000 0000000000000000 00000000 01000000'), + ); + }); + + /** a second connect() while registering overwrote the first callback, + * which was then never invoked */ + const first = new Promise((resolve) => layer.connect(() => resolve('first'))); + const second = new Promise((resolve) => layer.connect(() => resolve('second'))); + + assert.equal(transport.sent.length, 1, 'only one RegisterSession request'); + deliverResponse(); + + assert.deepEqual( + await withTimeout(Promise.all([first, second]), 'both connect callbacks'), + ['first', 'second'], + ); + await transport.close(); + }); +}); diff --git a/test/unit/layer.test.js b/test/unit/layer.test.js new file mode 100644 index 0000000..1be8757 --- /dev/null +++ b/test/unit/layer.test.js @@ -0,0 +1,41 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import Layer from '../../src/layers/Layer.js'; + +function sleep(ms) { + return new Promise((resolve) => { setTimeout(resolve, ms); }); +} + +describe('Layer context bookkeeping', () => { + it('removes the timeout handle entry when a context times out', async () => { + const layer = new Layer('test', null); + + const errors = []; + layer.contextCallback((err) => errors.push(err), 'ctx', 20); + + assert.equal(layer.__contextToCallback.size, 1); + assert.equal(layer.__contextToCallbackTimeouts.size, 1); + + await sleep(50); + + assert.equal(errors.length, 1); + assert.match(errors[0], /Timeout/); + assert.equal(layer.__contextToCallback.size, 0); + /** the timeout handle map leaked one dead entry per timed-out + * request for the lifetime of the layer */ + assert.equal(layer.__contextToCallbackTimeouts.size, 0); + }); + + it('clearContexts returns the cleared entries', () => { + const layer = new Layer('test', null); + layer.setContextForID('id1', 'ctxA'); + layer.setContextForID('id2', 'ctxB'); + + /** a live Map iterator taken before clear() always yields nothing */ + const entries = Array.from(layer.clearContexts()); + + assert.deepEqual(entries, [['id1', 'ctxA'], ['id2', 'ctxB']]); + assert.equal(layer.getContextForID('id1'), undefined); + }); +}); diff --git a/test/unit/logix5000.layer.test.js b/test/unit/logix5000.layer.test.js index ab6b3d3..91aed37 100644 --- a/test/unit/logix5000.layer.test.js +++ b/test/unit/logix5000.layer.test.js @@ -222,3 +222,90 @@ describe('CIP connection layer: resend keep-alive', () => { await transport.close(); }); }); + +describe('Logix5000: writeTag symbol resolution', () => { + /** GetInstanceAttributeList (0x55) reply listing one symbol: + * instance 0x0A named 'TagA' */ + const nameListReply = hex('d5 00 00 00 0a000000 0400 54616741'); + /** GetInstanceAttributeList reply with instance 0x0A's type: DINT */ + const typeListReply = hex('d5 00 00 00 0a000000 c400'); + + it('rejects (instead of hanging) when the tag does not exist', async () => { + const { transport, responder, layer } = createStack(); + responder.replyToConnected(nameListReply); + + /** the unknown-tag guard could never fire (getSymbolInfo returns an + * object, never null), so tagType.atomic threw inside an async + * executor whose rejection was discarded — the promise never settled */ + await assert.rejects( + withTimeout(layer.writeTag('DoesNotExist', 5), 'writeTag unknown tag', 3000), + /Invalid tag/, + ); + await transport.close(); + }); + + it('resolves member-access tags against the base symbol name', async () => { + const { transport, responder, layer } = createStack(); + responder.replyToConnected(nameListReply); + responder.replyToConnected(typeListReply); + responder.replyToConnected(hex('cd 00 00 00')); + + /** the member-stripping line was commented out, so 'TagA.Member' was + * looked up verbatim in the symbol list and never matched */ + const reply = await withTimeout(layer.writeTag('TagA.Member', 1), 'writeTag dotted tag', 3000); + assert.equal(reply.status.code, 0); + + /** the write request must address the full dotted path */ + assert.deepEqual( + responder.connectedRequests[2], + hex('4d 07 9104 54616741 9106 4d656d626572 c400 0100 01000000'), + ); + await transport.close(); + }); +}); + +describe('Logix5000: readTemplate', () => { + it('clamps per-fragment read sizes to 16 bits for very large templates', async () => { + const { transport, responder, layer } = createStack(); + + /** GetAttributeList reply: handle, member count 1, + * definition size 16400 words (=> 65580 definition bytes, > 0xFFFF), + * structure size 4 */ + responder.replyToConnected(hex( + '83 00 00 00' + + '0400' + + '0100 0000 3412' + + '0200 0000 0100' + + '0400 0000 10400000' + + '0500 0000 04000000', + )); + + const definition = Buffer.alloc(65580); + /** member record: info 0, type DINT (0xC4), offset 0 */ + definition.writeUInt16LE(0x00C4, 2); + definition.write('MyTemplate;ZZ\0Member1\0', 8, 'ascii'); + + const chunk1 = definition.subarray(0, 40000); + const chunk2 = definition.subarray(40000); + /** status 0x06 = partial transfer, more to read */ + responder.replyToConnected(Buffer.concat([hex('cc 00 06 00'), chunk1])); + responder.replyToConnected(Buffer.concat([hex('cc 00 00 00'), chunk2])); + + /** the remaining-bytes field is UINT16 on the wire; writing 65580 + * unclamped threw ERR_OUT_OF_RANGE and the read never succeeded */ + const template = await withTimeout(layer.readTemplate(0x32), 'readTemplate', 5000); + + assert.equal(template.name, 'MyTemplate'); + assert.deepEqual(template.members.map((member) => member.name), ['Member1']); + + const readRequests = responder.connectedRequests.slice(1); + assert.equal(readRequests.length, 2); + /** template read request: service(1) path-size(1) path(4) offset(4) count(2) */ + assert.equal(readRequests[0].readUInt32LE(6), 0); + assert.equal(readRequests[0].readUInt16LE(10), 0xFFFF); + assert.equal(readRequests[1].readUInt32LE(6), 40000); + assert.equal(readRequests[1].readUInt16LE(10), 25580); + + await transport.close(); + }); +}); diff --git a/test/unit/modbus.layer.test.js b/test/unit/modbus.layer.test.js index 7949fad..c638447 100644 --- a/test/unit/modbus.layer.test.js +++ b/test/unit/modbus.layer.test.js @@ -37,20 +37,30 @@ function withTimeout(promise, label, ms = 1000) { } describe('Modbus TCP layer: spec example frames', () => { - it('readCoils 20-38 (spec 6.1)', async () => { + it('readCoils 20-38 unpacks 19 coil statuses LSB-first (spec 6.1)', async () => { const { transport, layer } = createStack(); transport.reply(hex('0001 0000 0006 ff 01 03 cd 6b 05')); const value = await layer.readCoils(0x0013, 0x13); assert.deepEqual(transport.sent, [hex('0001 0000 0006 ff 01 0013 0013')]); - assert.deepEqual(value, [0xCD, 0x6B, 0x05]); + /** 0xCD, 0x6B, 0x05 unpack to the spec's coil statuses 20-38; the + * undefined pad bits of the last byte must not leak into the result */ + assert.deepEqual(value, [ + true, false, true, true, false, false, true, true, + true, true, false, true, false, true, true, false, + true, false, true, + ]); }); - it('readDiscreteInputs 197-218 (spec 6.2)', async () => { + it('readDiscreteInputs 197-218 unpacks 22 input statuses (spec 6.2)', async () => { const { transport, layer } = createStack(); transport.reply(hex('0001 0000 0006 ff 02 03 ac db 35')); const value = await layer.readDiscreteInputs(0x00C4, 0x16); assert.deepEqual(transport.sent, [hex('0001 0000 0006 ff 02 00c4 0016')]); - assert.deepEqual(value, [0xAC, 0xDB, 0x35]); + assert.deepEqual(value, [ + false, false, true, true, false, true, false, true, + true, true, false, true, true, false, true, true, + true, false, true, false, true, true, + ]); }); it('readHoldingRegisters 108-110 (spec 6.3)', async () => { @@ -214,6 +224,47 @@ describe('Modbus TCP layer: framing', () => { }); }); +describe('Modbus TCP layer: robustness', () => { + it('rejects a response whose byte count exceeds the received data', async () => { + const { transport, layer } = createStack(); + /** byte count claims 6 data bytes but the frame only carries 1 — + * must reject, not crash the process with an out-of-range read */ + transport.reply(hex('0001 0000 0004 ff 03 06 2b')); + await assert.rejects( + withTimeout(layer.readHoldingRegisters(0x006B, 3), 'malformed byte count'), + /Malformed/, + ); + }); + + it('rejects an exception response missing its exception code', async () => { + const { transport, layer } = createStack(); + transport.reply(hex('0001 0000 0002 ff 83')); + await assert.rejects( + withTimeout(layer.readHoldingRegisters(0, 1), 'empty exception frame'), + /Malformed/, + ); + }); + + it('rejects with a timeout when the server never responds', async () => { + const { transport, layer } = createStack({ timeout: 50 }); + transport.ignoreNextRequest(); + await assert.rejects( + withTimeout(layer.readHoldingRegisters(0, 1), 'unanswered request'), + /Timeout/, + ); + }); + + it('honors a per-request unitID of 0 (broadcast)', async () => { + const { transport, layer } = createStack(); + transport.reply(hex('0001 0000 0005 00 03 02 0007')); + const value = await withTimeout(new Promise((resolve, reject) => { + layer._send(hex('03 0000 0001'), { unitID: 0 }, { resolve, reject }); + }), 'unitID 0 request'); + assert.deepEqual(transport.sent, [hex('0001 0000 0006 00 03 0000 0001')]); + assert.deepEqual(value, [7]); + }); +}); + describe('Modbus TCP layer: transactions', () => { it('increments the transaction ID per request', async () => { const { transport, layer } = createStack(); diff --git a/test/unit/modbus.pdu.test.js b/test/unit/modbus.pdu.test.js index a9433e7..dab271b 100644 --- a/test/unit/modbus.pdu.test.js +++ b/test/unit/modbus.pdu.test.js @@ -86,3 +86,57 @@ describe('Modbus TCP frame', () => { assert.deepEqual(packet.pdu.value, [7]); }); }); + +describe('Modbus PDU malformed responses', () => { + /** + * Every length field inside a response comes off the wire; before + * bounds-checking was added, each of these threw ERR_OUT_OF_RANGE from + * the socket data handler (an uncaught exception) instead of producing + * a decodable error. + */ + + it('flags a register byte count exceeding the received data', () => { + const pdu = PDU.Decode(hex('03 06 2b'), { current: 0 }, 3); + assert.match(pdu.error.message, /Malformed/); + assert.equal(pdu.value, undefined); + }); + + it('flags an odd register byte count', () => { + const pdu = PDU.Decode(hex('03 03 01 02 03'), { current: 0 }, 5); + assert.match(pdu.error.message, /Malformed/); + }); + + it('flags a coil byte count exceeding the received data', () => { + const pdu = PDU.Decode(hex('01 05 cd'), { current: 0 }, 3); + assert.match(pdu.error.message, /Malformed/); + }); + + it('flags an exception response with no exception code', () => { + const pdu = PDU.Decode(hex('83'), { current: 0 }, 1); + assert.match(pdu.error.message, /Malformed/); + }); + + it('flags a truncated write response', () => { + const pdu = PDU.Decode(hex('06 0001'), { current: 0 }, 3); + assert.match(pdu.error.message, /Malformed/); + }); + + it('still decodes a valid frame after the checks', () => { + const pdu = PDU.Decode(hex('01 03 cd 6b 05'), { current: 0 }, 5); + assert.equal(pdu.error, undefined); + assert.deepEqual(pdu.value, [0xCD, 0x6B, 0x05]); + }); +}); + +describe('Modbus constants', () => { + it('resolves serial-line function names', async () => { + const constants = await import('../../src/core/modbus/constants.js'); + /** serial-line codes were omitted from FunctionNames, so responses + * decoded with name 'Unknown'; the export name was also misspelled */ + assert.equal(constants.FunctionNames[0x07], 'ReadExceptionStatus'); + assert.equal(constants.FunctionNames[0x11], 'ReportServerID'); + assert.equal(constants.SerialLineFunctions.Diagnostics, 0x08); + /** deprecated misspelled alias must keep working */ + assert.equal(constants.SearialLineFunctions, constants.SerialLineFunctions); + }); +}); diff --git a/test/unit/multiplex.layer.test.js b/test/unit/multiplex.layer.test.js new file mode 100644 index 0000000..3158bc0 --- /dev/null +++ b/test/unit/multiplex.layer.test.js @@ -0,0 +1,57 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import Layer from '../../src/layers/Layer.js'; +import MultiplexLayer from '../../src/layers/extras/MultiplexLayer.js'; +import ScriptedTransport from '../harness/ScriptedTransport.js'; + +class CaptureLayer extends Layer { + constructor(lowerLayer, name) { + super(name, lowerLayer); + this.received = []; + } + + handleData(data) { + this.received.push(data); + } +} + +function sleep(ms) { + return new Promise((resolve) => { setTimeout(resolve, ms); }); +} + +describe('MultiplexLayer', () => { + it('sends messages from an upper layer without crashing', async () => { + /** layerContext() called incrementContext(this) from a plain module + * function where `this` is undefined, so every multiplexed send threw + * a TypeError */ + const transport = new ScriptedTransport(); + const mux = new MultiplexLayer(transport); + const upper = new CaptureLayer(mux, 'upperA'); + + upper.send(Buffer.from([0x01]), null, false); + await sleep(10); + + assert.deepEqual(transport.sent, [Buffer.from([0x01])]); + }); + + it('routes responses back to the layer that sent the request', async () => { + const transport = new ScriptedTransport(); + const mux = new MultiplexLayer(transport); + const upperA = new CaptureLayer(mux, 'upperA'); + const upperB = new CaptureLayer(mux, 'upperB'); + + upperA.send(Buffer.from([0xA1]), null, false); + upperB.send(Buffer.from([0xB1]), null, false); + await sleep(10); + + assert.equal(transport.requests.length, 2); + + /** answer in reverse order to prove context-based routing */ + transport.deliver(Buffer.from([0xB2]), null, transport.requests[1].context); + transport.deliver(Buffer.from([0xA2]), null, transport.requests[0].context); + + assert.deepEqual(upperA.received, [Buffer.from([0xA2])]); + assert.deepEqual(upperB.received, [Buffer.from([0xB2])]); + }); +}); diff --git a/test/unit/tcp.layer.test.js b/test/unit/tcp.layer.test.js index a33516d..7623a2e 100644 --- a/test/unit/tcp.layer.test.js +++ b/test/unit/tcp.layer.test.js @@ -58,3 +58,82 @@ describe('TCP layer', () => { server.close(); }); }); + +function withTimeout(promise, label, ms = 3000) { + return new Promise((resolve, reject) => { + const handle = setTimeout(() => { + reject(new Error(`${label} did not settle within ${ms}ms`)); + }, ms); + promise.then( + (value) => { clearTimeout(handle); resolve(value); }, + (err) => { clearTimeout(handle); reject(err); }, + ); + }); +} + +describe('TCP layer connection lifecycle', () => { + it('connected() resolves false on a layer that never connected', async () => { + const layer = new TCPLayer({ host: '127.0.0.1', port: 1 }); + assert.equal(await withTimeout(layer.connected(), 'connected()'), false); + }); + + it('settles the connect promise when the connection is refused', async () => { + /** grab a port that is guaranteed closed */ + const { server } = await listen(); + const { port } = server.address(); + await new Promise((resolve) => { server.close(resolve); }); + + const layer = new TCPLayer({ host: '127.0.0.1', port, connectTimeout: 2000 }); + layer.send(Buffer.from([1]), null, false); + + /** ECONNREFUSED emits 'error', never 'timeout' — the promise + * previously stayed pending forever */ + assert.equal(await withTimeout(layer.connected(), 'refused connect'), false); + }); + + it('destroys the in-flight socket when disconnecting while connecting', async () => { + const { server } = await listen(); + const layer = new TCPLayer({ host: '127.0.0.1', port: server.address().port }); + + layer.send(Buffer.from([1]), null, false); + assert.equal(layer._connectionState, 1, 'layer should be connecting'); + + await withTimeout(layer.disconnect(), 'disconnect while connecting'); + + /** without destroy() the OS completes the handshake later and the + * established socket leaks */ + assert.ok(layer.socket.destroyed, 'in-flight socket must be destroyed'); + + await new Promise((resolve) => { server.close(resolve); }); + }); +}); + +describe('TCP layer Scan', () => { + it('yields hosts with open ports', async () => { + const { server } = await listen(); + const { port } = server.address(); + + /** Scan awaited connected() on a layer that never initiated a + * connection, so it reported nothing on any network */ + const results = []; + for await (const result of TCPLayer.Scan(['127.0.0.1'], [port])) { + results.push(result); + } + + assert.deepEqual(results, [{ host: '127.0.0.1', port }]); + await new Promise((resolve) => { server.close(resolve); }); + }); + + it('skips hosts with closed ports', async () => { + const { server } = await listen(); + const { port } = server.address(); + await new Promise((resolve) => { server.close(resolve); }); + + const results = []; + for await (const result of TCPLayer.Scan(['127.0.0.1'], [port])) { + results.push(result); + } + + assert.deepEqual(results, []); + }); +}); diff --git a/test/unit/udp.layer.test.js b/test/unit/udp.layer.test.js new file mode 100644 index 0000000..b6d677f --- /dev/null +++ b/test/unit/udp.layer.test.js @@ -0,0 +1,37 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import UDPLayer from '../../src/layers/udp/index.js'; + +function withTimeout(promise, label, ms = 2000) { + return new Promise((resolve, reject) => { + const handle = setTimeout(() => { + reject(new Error(`${label} did not settle within ${ms}ms`)); + }, ms); + promise.then( + (value) => { clearTimeout(handle); resolve(value); }, + (err) => { clearTimeout(handle); reject(err); }, + ); + }); +} + +describe('UDP layer', () => { + it('closes the bound socket when the layer is destroyed', async () => { + const layer = new UDPLayer({ host: '127.0.0.1', port: 44818 }); + + const socket = layer.socket; + await withTimeout( + new Promise((resolve) => { socket.once('listening', resolve); }), + 'socket listening', + ); + + /** handleDestroy only unref'd the socket, leaving the port bound + * (and receiving) for the life of the process */ + const closed = new Promise((resolve) => { socket.once('close', resolve); }); + layer.destroy('destroyed by test'); + await withTimeout(closed, 'socket close after destroy'); + + assert.equal(layer.socket, null); + assert.throws(() => socket.address(), /not running/i); + }); +}); diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index c0743e1..3283bfb 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -1,6 +1,6 @@ import { describe, test } from 'node:test'; import assert from 'node:assert/strict'; -import { getBits } from '../../src/utils.js'; +import { getBits, CallbackPromise, InvertKeyValues } from '../../src/utils.js'; describe('getBits', () => { test('getBits', () => { @@ -16,3 +16,61 @@ describe('getBits', () => { assert.equal(getBits(2, 0, 2), 2); }); }); + +describe('CallbackPromise', () => { + function withTimeout(promise, label, ms = 1000) { + return new Promise((resolve, reject) => { + const handle = setTimeout(() => { + reject(new Error(`${label} did not settle within ${ms}ms`)); + }, ms); + promise.then( + (value) => { clearTimeout(handle); resolve(value); }, + (err) => { clearTimeout(handle); reject(err); }, + ); + }); + } + + test('rejects when an async executor throws', async () => { + /** rejections from async executors were discarded, leaving the + * returned promise pending forever plus an unhandledRejection */ + await assert.rejects( + withTimeout(CallbackPromise(null, async () => { + throw new Error('async boom'); + }), 'async executor rejection'), + /async boom/, + ); + }); + + test('rejects when an async executor throws after awaiting', async () => { + await assert.rejects( + withTimeout(CallbackPromise(null, async () => { + await new Promise((resolve) => { setImmediate(resolve); }); + throw new Error('late boom'); + }), 'late async executor rejection'), + /late boom/, + ); + }); + + test('invokes the node-style callback when an async executor throws', async () => { + const error = await withTimeout(new Promise((resolve) => { + CallbackPromise((err) => resolve(err), async () => { + throw new Error('callback boom'); + }); + }), 'callback style rejection'); + assert.match(error.message, /callback boom/); + }); + + test('still resolves through the resolver', async () => { + assert.equal(await CallbackPromise(null, (resolver) => resolver.resolve(42)), 42); + }); +}); + +describe('InvertKeyValues', () => { + test('inverts a Map', () => { + /** the Map branch called obj.entries().forEach, which does not exist + * on Node 20 (the minimum supported engine) */ + const inverted = InvertKeyValues(new Map([['a', 1], ['b', 2]])); + assert.equal(inverted.get(1), 'a'); + assert.equal(inverted.get(2), 'b'); + }); +});