From 31516f2d34649c0216640b205f7f67d11b2b0618 Mon Sep 17 00:00:00 2001 From: Norbert Richter Date: Sun, 6 Nov 2022 10:12:08 +0100 Subject: [PATCH 1/3] Add ModbusBridge malloc error notes --- .../xdrv_63_modbus_bridge.ino | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino b/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino index 479ea560b..43bea32bf 100644 --- a/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino +++ b/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino @@ -187,6 +187,10 @@ uint32_t swap_endian32(uint32_t num) ((num<<24)&0xff000000); // byte 0 to byte 3 } +void ModbusBridgeAllocError(const char* s) +{ + AddLog(LOG_LEVEL_ERROR, PSTR("MBS: could not allocate %s buffer"), s); +} /********************************************************************************************/ // @@ -248,6 +252,11 @@ void ModbusBridgeHandle(void) uint8_t *buffer; if (modbusBridge.byteCount == 0) modbusBridge.byteCount = modbusBridge.dataCount * 2; buffer = (uint8_t *)malloc(9 + modbusBridge.byteCount); // Addres(1), Function(1), Length(1), Data(1..n), CRC(2) + if (nullptr == buffer) + { + ModbusBridgeAllocError(PSTR("read")); + return; + } memset(buffer, 0, 9 + modbusBridge.byteCount); uint32_t error = tasmotaModbus->ReceiveBuffer(buffer, 0, modbusBridge.byteCount); @@ -562,9 +571,9 @@ void ModbusBridgeInit(void) #ifdef USE_MODBUS_BRIDGE_TCP // If TCP bridge is enabled allocate a TCP receive buffer modbusBridgeTCP.tcp_buf = (uint8_t *)malloc(MODBUS_BRIDGE_TCP_BUF_SIZE); - if (!modbusBridgeTCP.tcp_buf) + if (nullptr == modbusBridgeTCP.tcp_buf) { - AddLog(LOG_LEVEL_ERROR, PSTR("MBS: MBRTCP could not allocate buffer")); + ModbusBridgeAllocError(PSTR("TCP")); return; } #endif @@ -674,6 +683,11 @@ void ModbusTCPHandle(void) modbusBridge.dataCount = 1; writeData = (uint16_t *)malloc((byteCount / 2)+1); + if (nullptr == writeData) + { + ModbusBridgeAllocError(PSTR("write")); + return; + } if ((mbfunctioncode == 15) || (mbfunctioncode == 16)) count = (uint16_t)((((uint16_t)modbusBridgeTCP.tcp_buf[10]) << 8) | ((uint16_t)modbusBridgeTCP.tcp_buf[11])); else count = 1; @@ -860,6 +874,11 @@ void CmndModbusBridgeSend(void) else { writeData = (uint16_t *)malloc(modbusBridge.dataCount); + if (nullptr == writeData) + { + ModbusBridgeAllocError(PSTR("write")); + return; + } for (uint8_t jsonDataArrayPointer = 0; jsonDataArrayPointer < writeDataSize; jsonDataArrayPointer++) { From f76bed338be31a2e92f5ba290dbc47820ff6347e Mon Sep 17 00:00:00 2001 From: Norbert Richter Date: Sun, 6 Nov 2022 10:23:05 +0100 Subject: [PATCH 2/3] Localize ModbusBridge global func/var names --- .../xdrv_63_modbus_bridge.ino | 106 ++++++++---------- 1 file changed, 49 insertions(+), 57 deletions(-) diff --git a/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino b/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino index 43bea32bf..0e39e97a6 100644 --- a/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino +++ b/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino @@ -35,10 +35,10 @@ * * -- Write multiple coils -- * ModbusSend {"deviceaddress": 1, "functioncode": 15, "startaddress": 1, "type":"bit", "count":4, "values":[1,0,1,1]} - * - * Info for modbusBridgeTCPServer: + * + * Info for modbusBridgeTCPServer: * https://ipc2u.com/articles/knowledge-base/detailed-description-of-the-modbus-tcp-protocol-with-command-examples/ - * + * * Info for modbus serial communications: * https://ozeki.hu/p_5879-mobdbus-function-code-4-read-input-registers.html * https://www.modbustools.com/modbus.html @@ -102,7 +102,7 @@ ModbusBridgeTCP modbusBridgeTCP; #endif #include -TasmotaModbus *tasmotaModbus = nullptr; +TasmotaModbus *modbusBridgeModbus = nullptr; enum class ModbusBridgeError { @@ -172,21 +172,13 @@ ModbusBridge modbusBridge; /********************************************************************************************/ // -// Helper functions for data conversion between little and big endian +// Helper functions // -uint16_t swap_endian16(uint16_t num) +uint16_t ModbusBridgeSwapEndian16(uint16_t num) { return (num>>8) | (num<<8); } -uint32_t swap_endian32(uint32_t num) -{ - return ((num>>24)&0xff) | // move byte 3 to byte 0 - ((num<<8)&0xff0000) | // move byte 1 to byte 2 - ((num>>8)&0xff00) | // move byte 2 to byte 1 - ((num<<24)&0xff000000); // byte 0 to byte 3 -} - void ModbusBridgeAllocError(const char* s) { AddLog(LOG_LEVEL_ERROR, PSTR("MBS: could not allocate %s buffer"), s); @@ -203,7 +195,7 @@ bool ModbusBridgeBegin(void) if (Settings->modbus_sconfig > TS_SERIAL_8O2) Settings->modbus_sconfig = TS_SERIAL_8N1; - int result = tasmotaModbus->Begin(Settings->modbus_sbaudrate * 300, ConvertSerialConfig(Settings->modbus_sconfig)); // Reinitialize modbus port with new baud rate + int result = modbusBridgeModbus->Begin(Settings->modbus_sbaudrate * 300, ConvertSerialConfig(Settings->modbus_sconfig)); // Reinitialize modbus port with new baud rate if (result) { if (2 == result) @@ -215,7 +207,7 @@ bool ModbusBridgeBegin(void) return result; } -void SetModbusBridgeConfig(uint32_t serial_config) +void ModbusBridgeSetConfig(uint32_t serial_config) { if (serial_config > TS_SERIAL_8O2) { @@ -228,7 +220,7 @@ void SetModbusBridgeConfig(uint32_t serial_config) } } -void SetModbusBridgeBaudrate(uint32_t baudrate) +void ModbusBridgeSetBaudrate(uint32_t baudrate) { if ((baudrate >= 300) && (baudrate <= 115200)) { @@ -246,7 +238,7 @@ void SetModbusBridgeBaudrate(uint32_t baudrate) // void ModbusBridgeHandle(void) { - bool data_ready = tasmotaModbus->ReceiveReady(); + bool data_ready = modbusBridgeModbus->ReceiveReady(); if (data_ready) { uint8_t *buffer; @@ -258,7 +250,7 @@ void ModbusBridgeHandle(void) return; } memset(buffer, 0, 9 + modbusBridge.byteCount); - uint32_t error = tasmotaModbus->ReceiveBuffer(buffer, 0, modbusBridge.byteCount); + uint32_t error = modbusBridgeModbus->ReceiveBuffer(buffer, 0, modbusBridge.byteCount); #ifdef USE_MODBUS_BRIDGE_TCP for (uint32_t i = 0; i < nitems(modbusBridgeTCP.client_tcp); i++) @@ -283,7 +275,7 @@ void ModbusBridgeHandle(void) nrOfBytes += 1; client.write(header, 9); } - else if (buffer[1] <= 2) + else if (buffer[1] <= 2) { header[4] = modbusBridge.byteCount >> 8; header[5] = modbusBridge.byteCount + 3; @@ -293,7 +285,7 @@ void ModbusBridgeHandle(void) client.write(buffer + 3, modbusBridge.byteCount); // Don't send CRC nrOfBytes += modbusBridge.byteCount; } - else if (buffer[1] <= 4) + else if (buffer[1] <= 4) { header[4] = modbusBridge.byteCount >> 8; header[5] = modbusBridge.byteCount + 3; @@ -367,10 +359,10 @@ void ModbusBridgeHandle(void) if (modbusBridge.type == ModbusBridgeType::mb_raw) { Response_P(PSTR("{\"" D_JSON_MODBUS_RECEIVED "\":{\"RAW\":[")); - for (uint8_t i = 0; i < tasmotaModbus->ReceiveCount(); i++) + for (uint8_t i = 0; i < modbusBridgeModbus->ReceiveCount(); i++) { ResponseAppend_P(PSTR("%d"), buffer[i]); - if (i < tasmotaModbus->ReceiveCount() - 1) + if (i < modbusBridgeModbus->ReceiveCount() - 1) ResponseAppend_P(PSTR(",")); } ResponseAppend_P(PSTR("]}")); @@ -380,10 +372,10 @@ void ModbusBridgeHandle(void) else if (modbusBridge.type == ModbusBridgeType::mb_hex) { Response_P(PSTR("{\"" D_JSON_MODBUS_RECEIVED "\":{\"HEX\":[")); - for (uint8_t i = 0; i < tasmotaModbus->ReceiveCount(); i++) + for (uint8_t i = 0; i < modbusBridgeModbus->ReceiveCount(); i++) { ResponseAppend_P(PSTR("0x%02X"), buffer[i]); - if (i < tasmotaModbus->ReceiveCount() - 1) + if (i < modbusBridgeModbus->ReceiveCount() - 1) ResponseAppend_P(PSTR(",")); } ResponseAppend_P(PSTR("]}")); @@ -405,7 +397,7 @@ void ModbusBridgeHandle(void) ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_START_ADDRESS "\":%d,"), (buffer[2] << 8) + buffer[3]); dataOffset = 4; } - ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_LENGTH "\":%d,"), tasmotaModbus->ReceiveCount()); + ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_LENGTH "\":%d,"), modbusBridgeModbus->ReceiveCount()); ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_COUNT "\":%d,"), modbusBridge.count); ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_VALUES "\":[")); @@ -539,7 +531,7 @@ void ModbusBridgeHandle(void) ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_DEVICE_ADDRESS "\":%d,"), buffer[0]); ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_FUNCTION_CODE "\":%d,"), buffer[1]); ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_START_ADDRESS "\":%d,"), (buffer[2] << 8) + buffer[3]); - ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_LENGTH "\":%d,"), tasmotaModbus->ReceiveCount()); + ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_LENGTH "\":%d,"), modbusBridgeModbus->ReceiveCount()); ResponseAppend_P(PSTR("\"" D_JSON_MODBUS_COUNT "\":%d"), (buffer[4] << 8) + buffer[5]); ResponseAppend_P(PSTR("}")); ResponseJsonEnd(); @@ -566,7 +558,7 @@ void ModbusBridgeInit(void) { if (PinUsed(GPIO_MBR_RX) && PinUsed(GPIO_MBR_TX)) { - tasmotaModbus = new TasmotaModbus(Pin(GPIO_MBR_RX), Pin(GPIO_MBR_TX)); + modbusBridgeModbus = new TasmotaModbus(Pin(GPIO_MBR_RX), Pin(GPIO_MBR_TX)); ModbusBridgeBegin(); #ifdef USE_MODBUS_BRIDGE_TCP // If TCP bridge is enabled allocate a TCP receive buffer @@ -591,7 +583,7 @@ void ModbusTCPHandle(void) bool busy; // did we transfer some data? int32_t buf_len; - if (!tasmotaModbus) + if (!modbusBridgeModbus) return; // check for a new client connection @@ -665,8 +657,8 @@ void ModbusTCPHandle(void) if (mbfunctioncode <= 2) { count = (uint16_t)((((uint16_t)modbusBridgeTCP.tcp_buf[10]) << 8) | ((uint16_t)modbusBridgeTCP.tcp_buf[11])); - modbusBridge.byteCount = ((count - 1) >> 3) + 1; - modbusBridge.dataCount = ((count - 1) >> 4) + 1; + modbusBridge.byteCount = ((count - 1) >> 3) + 1; + modbusBridge.dataCount = ((count - 1) >> 4) + 1; } else if (mbfunctioncode <= 4) { @@ -676,7 +668,7 @@ void ModbusTCPHandle(void) } else { - // For functioncode 15 & 16 ignore bytecount, tasmotaModbus does calculate this + // For functioncode 15 & 16 ignore bytecount, modbusBridgeModbus does calculate this uint8_t dataStartByte = mbfunctioncode <= 6 ? 10 : 13; uint16_t byteCount = (buf_len - dataStartByte); modbusBridge.byteCount = 2; @@ -688,10 +680,10 @@ void ModbusTCPHandle(void) ModbusBridgeAllocError(PSTR("write")); return; } - + if ((mbfunctioncode == 15) || (mbfunctioncode == 16)) count = (uint16_t)((((uint16_t)modbusBridgeTCP.tcp_buf[10]) << 8) | ((uint16_t)modbusBridgeTCP.tcp_buf[11])); else count = 1; - + for (uint16_t dataPointer = 0; dataPointer < byteCount; dataPointer++) { if (dataPointer % 2 == 0) @@ -699,7 +691,7 @@ void ModbusTCPHandle(void) writeData[dataPointer / 2] = (uint16_t)(((uint16_t)modbusBridgeTCP.tcp_buf[dataStartByte + dataPointer]) << 8); } else - { + { writeData[dataPointer / 2] |= ((uint16_t)modbusBridgeTCP.tcp_buf[dataStartByte + dataPointer]); } } @@ -708,7 +700,7 @@ void ModbusTCPHandle(void) AddLog(LOG_LEVEL_DEBUG_MORE, PSTR("MBS: MBRTCP to Modbus TransactionId:%d, deviceAddress:%d, functionCode:%d, startAddress:%d, count:%d, recvCount:%d, recvBytes:%d"), modbusBridgeTCP.tcp_transaction_id, mbdeviceaddress, mbfunctioncode, mbstartaddress, count, modbusBridge.dataCount, modbusBridge.byteCount); - tasmotaModbus->Send(mbdeviceaddress, mbfunctioncode, mbstartaddress, count, writeData); + modbusBridgeModbus->Send(mbdeviceaddress, mbfunctioncode, mbstartaddress, count, writeData); free(writeData); } @@ -908,7 +900,7 @@ void CmndModbusBridgeSend(void) writeData[jsonDataArrayPointer / 2] = (int8_t)jsonDataArray[jsonDataArrayPointer / 2].getInt(0) << 8; if (modbusBridge.dataCount != writeDataSize / 2) errorcode = ModbusBridgeError::wrongcount; break; - + case ModbusBridgeType::mb_hex: case ModbusBridgeType::mb_raw: case ModbusBridgeType::mb_uint8: @@ -918,31 +910,31 @@ void CmndModbusBridgeSend(void) writeData[jsonDataArrayPointer / 2] = (uint8_t)jsonDataArray[jsonDataArrayPointer].getUInt(0) << 8; if (modbusBridge.dataCount != writeDataSize / 2) errorcode = ModbusBridgeError::wrongcount; break; - + case ModbusBridgeType::mb_int16: - writeData[jsonDataArrayPointer] = bitMode ? swap_endian16(jsonDataArray[jsonDataArrayPointer].getInt(0)) + writeData[jsonDataArrayPointer] = bitMode ? ModbusBridgeSwapEndian16(jsonDataArray[jsonDataArrayPointer].getInt(0)) : (int16_t)jsonDataArray[jsonDataArrayPointer].getInt(0); break; - + case ModbusBridgeType::mb_uint16: - writeData[jsonDataArrayPointer] = bitMode ? swap_endian16(jsonDataArray[jsonDataArrayPointer].getUInt(0)) + writeData[jsonDataArrayPointer] = bitMode ? ModbusBridgeSwapEndian16(jsonDataArray[jsonDataArrayPointer].getUInt(0)) : (int16_t)jsonDataArray[jsonDataArrayPointer].getUInt(0); break; - + case ModbusBridgeType::mb_int32: - writeData[(jsonDataArrayPointer * 2)] = bitMode ? swap_endian16(jsonDataArray[jsonDataArrayPointer].getInt(0)) + writeData[(jsonDataArrayPointer * 2)] = bitMode ? ModbusBridgeSwapEndian16(jsonDataArray[jsonDataArrayPointer].getInt(0)) : (int16_t)(jsonDataArray[jsonDataArrayPointer].getInt(0) >> 16); - writeData[(jsonDataArrayPointer * 2) + 1] = bitMode ? swap_endian16(jsonDataArray[jsonDataArrayPointer].getInt(0) >> 16) + writeData[(jsonDataArrayPointer * 2) + 1] = bitMode ? ModbusBridgeSwapEndian16(jsonDataArray[jsonDataArrayPointer].getInt(0) >> 16) : (uint16_t)(jsonDataArray[jsonDataArrayPointer].getInt(0)); break; - + case ModbusBridgeType::mb_uint32: - writeData[(jsonDataArrayPointer * 2)] = bitMode ? swap_endian16(jsonDataArray[jsonDataArrayPointer].getUInt(0)) + writeData[(jsonDataArrayPointer * 2)] = bitMode ? ModbusBridgeSwapEndian16(jsonDataArray[jsonDataArrayPointer].getUInt(0)) : (uint16_t)(jsonDataArray[jsonDataArrayPointer].getUInt(0) >> 16); - writeData[(jsonDataArrayPointer * 2) + 1] = bitMode ? swap_endian16(jsonDataArray[jsonDataArrayPointer].getUInt(0) >> 16) + writeData[(jsonDataArrayPointer * 2) + 1] = bitMode ? ModbusBridgeSwapEndian16(jsonDataArray[jsonDataArrayPointer].getUInt(0) >> 16) : (uint16_t)(jsonDataArray[jsonDataArrayPointer].getUInt(0)); break; - + case ModbusBridgeType::mb_float: // TODO default: @@ -971,20 +963,20 @@ void CmndModbusBridgeSend(void) if ((modbusBridge.functionCode == ModbusBridgeFunctionCode::mb_writeSingleCoil) || (modbusBridge.functionCode == ModbusBridgeFunctionCode::mb_writeSingleRegister)) modbusBridge.dataCount = 1; - uint8_t error = tasmotaModbus->Send(modbusBridge.deviceAddress, (uint8_t)modbusBridge.functionCode, modbusBridge.startAddress, modbusBridge.dataCount, writeData); + uint8_t error = modbusBridgeModbus->Send(modbusBridge.deviceAddress, (uint8_t)modbusBridge.functionCode, modbusBridge.startAddress, modbusBridge.dataCount, writeData); free(writeData); - + if (error) { AddLog(LOG_LEVEL_DEBUG, PSTR("MBS: MBR Driver send error %u"), error); return; - } + } ResponseCmndDone(); } void CmndModbusBridgeSetBaudrate(void) { - SetModbusBridgeBaudrate(XdrvMailbox.payload); + ModbusBridgeSetBaudrate(XdrvMailbox.payload); ResponseCmndNumber(Settings->modbus_sbaudrate * 300); } @@ -1000,7 +992,7 @@ void CmndModbusBridgeSetConfig(void) { // Use 0..23 as serial config option if ((XdrvMailbox.payload >= TS_SERIAL_5N1) && (XdrvMailbox.payload <= TS_SERIAL_8O2)) { - SetModbusBridgeConfig(XdrvMailbox.payload); + ModbusBridgeSetConfig(XdrvMailbox.payload); } } else if ((XdrvMailbox.payload >= 5) && (XdrvMailbox.payload <= 8)) @@ -1008,7 +1000,7 @@ void CmndModbusBridgeSetConfig(void) int8_t serial_config = ParseSerialConfig(XdrvMailbox.data); if (serial_config >= 0) { - SetModbusBridgeConfig(serial_config); + ModbusBridgeSetConfig(serial_config); } } } @@ -1023,7 +1015,7 @@ void CmndModbusBridgeSetConfig(void) void CmndModbusTCPStart(void) { - if (!tasmotaModbus) + if (!modbusBridgeModbus) { return; } @@ -1076,7 +1068,7 @@ void CmndModbusTCPConnect(void) { int32_t tcp_port = XdrvMailbox.payload; - if (!tasmotaModbus) + if (!modbusBridgeModbus) { return; } @@ -1135,7 +1127,7 @@ bool Xdrv63(uint8_t function) { ModbusBridgeInit(); } - else if (tasmotaModbus) + else if (modbusBridgeModbus) { switch (function) { From 050f2e7e61f9544e4996735e6b006a34dc20f2d1 Mon Sep 17 00:00:00 2001 From: Norbert Richter Date: Sun, 6 Nov 2022 12:32:02 +0100 Subject: [PATCH 3/3] Fix ModbusBridge buffer overflow (#16979) --- CHANGELOG.md | 1 + tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52a1b30b0..5a3da0ee0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ All notable changes to this project will be documented in this file. ### Fixed - Deduplicate code and fix %timer n% rule regression from v12.2.0 (#16914) - Serial initialization for baudrate and config (#16970) +- ModbusBridge buffer overflow (#16979) ### Removed - Define ``USE_PN532_DATA_RAW`` from NFC reader (#16939) diff --git a/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino b/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino index 0e39e97a6..31b1c7a19 100644 --- a/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino +++ b/tasmota/tasmota_xdrv_driver/xdrv_63_modbus_bridge.ino @@ -258,7 +258,7 @@ void ModbusBridgeHandle(void) WiFiClient &client = modbusBridgeTCP.client_tcp[i]; if (client) { - uint8_t header[8]; + uint8_t header[9]; uint8_t nrOfBytes = 8; header[0] = modbusBridgeTCP.tcp_transaction_id >> 8; header[1] = modbusBridgeTCP.tcp_transaction_id;