Fix bugs found in codebase-wide review, with regression tests

src/core/modbus/constants.js

@@ -15,15 +15,18 @@ export const Functions = Object.freeze({
   EncapsulatedInterfaceTransport: 0x2B,
 });
 
-export const SearialLineFunctions = Object.freeze({
+export const SerialLineFunctions = Object.freeze({
   ReadExceptionStatus: 0x07,
   Diagnostics: 0x08,
   GetCommEventCounter: 0x0B,
   GetCommEventLog: 0x0C,
   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',

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,

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;
   }

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:

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++) {

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;

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 */

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),

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 {}
 

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);

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);

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() {