From 0d95b1afcc56a5c027e6fc5a8465ac580d52f722 Mon Sep 17 00:00:00 2001 From: raulperdomo <36527079+raulperdomo@users.noreply.github.com> Date: Thu, 20 Mar 2025 11:40:13 -0400 Subject: [PATCH] Added bounds checking to memcpy and use memory-safe strlcpy (#6351) * Added bounds checking to memcpy and use memory-safe strlcpy for reading serial data in processWXSerial() function. * Fixed linting with trunk --- src/modules/SerialModule.cpp | 134 ++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 66 deletions(-) diff --git a/src/modules/SerialModule.cpp b/src/modules/SerialModule.cpp index 811d1ec91..34ece2312 100644 --- a/src/modules/SerialModule.cpp +++ b/src/modules/SerialModule.cpp @@ -468,81 +468,83 @@ void SerialModule::processWXSerial() // Extract the current line char line[meshtastic_Constants_DATA_PAYLOAD_LEN]; memset(line, '\0', sizeof(line)); - memcpy(line, &serialBytes[lineStart], lineEnd - lineStart); - if (strstr(line, "Wind") != NULL) // we have a wind line - { - gotwind = true; - // Find the positions of "=" signs in the line - char *windDirPos = strstr(line, "WindDir = "); - char *windSpeedPos = strstr(line, "WindSpeed = "); - char *windGustPos = strstr(line, "WindGust = "); + if (lineEnd - lineStart < sizeof(line) - 1) { + memcpy(line, &serialBytes[lineStart], lineEnd - lineStart); + if (strstr(line, "Wind") != NULL) // we have a wind line + { + gotwind = true; + // Find the positions of "=" signs in the line + char *windDirPos = strstr(line, "WindDir = "); + char *windSpeedPos = strstr(line, "WindSpeed = "); + char *windGustPos = strstr(line, "WindGust = "); - if (windDirPos != NULL) { - // Extract data after "=" for WindDir - strcpy(windDir, windDirPos + 15); // Add 15 to skip "WindDir = " - double radians = GeoCoord::toRadians(strtof(windDir, nullptr)); - dir_sum_sin += sin(radians); - dir_sum_cos += cos(radians); - dirCount++; - } else if (windSpeedPos != NULL) { - // Extract data after "=" for WindSpeed - strcpy(windVel, windSpeedPos + 15); // Add 15 to skip "WindSpeed = " - float newv = strtof(windVel, nullptr); - velSum += newv; - velCount++; - if (newv < lull || lull == -1) - lull = newv; + if (windDirPos != NULL) { + // Extract data after "=" for WindDir + strlcpy(windDir, windDirPos + 15, sizeof(windDir)); // Add 15 to skip "WindDir = " + double radians = GeoCoord::toRadians(strtof(windDir, nullptr)); + dir_sum_sin += sin(radians); + dir_sum_cos += cos(radians); + dirCount++; + } else if (windSpeedPos != NULL) { + // Extract data after "=" for WindSpeed + strlcpy(windVel, windSpeedPos + 15, sizeof(windVel)); // Add 15 to skip "WindSpeed = " + float newv = strtof(windVel, nullptr); + velSum += newv; + velCount++; + if (newv < lull || lull == -1) + lull = newv; - } else if (windGustPos != NULL) { - strcpy(windGust, windGustPos + 15); // Add 15 to skip "WindSpeed = " - float newg = strtof(windGust, nullptr); - if (newg > gust) - gust = newg; - } + } else if (windGustPos != NULL) { + strlcpy(windGust, windGustPos + 15, sizeof(windGust)); // Add 15 to skip "WindSpeed = " + float newg = strtof(windGust, nullptr); + if (newg > gust) + gust = newg; + } - // these are also voltage data we care about possibly - } else if (strstr(line, "BatVoltage") != NULL) { // we have a battVoltage line - char *batVoltagePos = strstr(line, "BatVoltage = "); - if (batVoltagePos != NULL) { - strcpy(batVoltage, batVoltagePos + 17); // 18 for ws 80, 17 for ws85 - batVoltageF = strtof(batVoltage, nullptr); - break; // last possible data we want so break - } - } else if (strstr(line, "CapVoltage") != NULL) { // we have a cappVoltage line - char *capVoltagePos = strstr(line, "CapVoltage = "); - if (capVoltagePos != NULL) { - strcpy(capVoltage, capVoltagePos + 17); // 18 for ws 80, 17 for ws85 - capVoltageF = strtof(capVoltage, nullptr); - } - // GXTS04Temp = 24.4 - } else if (strstr(line, "GXTS04Temp") != NULL) { // we have a temperature line - char *tempPos = strstr(line, "GXTS04Temp = "); - if (tempPos != NULL) { - strcpy(temperature, tempPos + 15); // 15 spaces for ws85 - temperatureF = strtof(temperature, nullptr); - } + // these are also voltage data we care about possibly + } else if (strstr(line, "BatVoltage") != NULL) { // we have a battVoltage line + char *batVoltagePos = strstr(line, "BatVoltage = "); + if (batVoltagePos != NULL) { + strlcpy(batVoltage, batVoltagePos + 17, sizeof(batVoltage)); // 18 for ws 80, 17 for ws85 + batVoltageF = strtof(batVoltage, nullptr); + break; // last possible data we want so break + } + } else if (strstr(line, "CapVoltage") != NULL) { // we have a cappVoltage line + char *capVoltagePos = strstr(line, "CapVoltage = "); + if (capVoltagePos != NULL) { + strlcpy(capVoltage, capVoltagePos + 17, sizeof(capVoltage)); // 18 for ws 80, 17 for ws85 + capVoltageF = strtof(capVoltage, nullptr); + } + // GXTS04Temp = 24.4 + } else if (strstr(line, "GXTS04Temp") != NULL) { // we have a temperature line + char *tempPos = strstr(line, "GXTS04Temp = "); + if (tempPos != NULL) { + strlcpy(temperature, tempPos + 15, sizeof(temperature)); // 15 spaces for ws85 + temperatureF = strtof(temperature, nullptr); + } - } else if (strstr(line, "RainIntSum") != NULL) { // we have a rainsum line - // LOG_INFO(line); - char *pos = strstr(line, "RainIntSum = "); - if (pos != NULL) { - strcpy(rainStr, pos + 17); // 17 spaces for ws85 - rainSum = int(strtof(rainStr, nullptr)); - } - - } else if (strstr(line, "Rain") != NULL) { // we have a rain line - if (strstr(line, "WaveRain") == NULL) { // skip WaveRain lines though. + } else if (strstr(line, "RainIntSum") != NULL) { // we have a rainsum line // LOG_INFO(line); - char *pos = strstr(line, "Rain = "); + char *pos = strstr(line, "RainIntSum = "); if (pos != NULL) { - strcpy(rainStr, pos + 17); // 17 spaces for ws85 - rain = strtof(rainStr, nullptr); + strlcpy(rainStr, pos + 17, sizeof(rainStr)); // 17 spaces for ws85 + rainSum = int(strtof(rainStr, nullptr)); + } + + } else if (strstr(line, "Rain") != NULL) { // we have a rain line + if (strstr(line, "WaveRain") == NULL) { // skip WaveRain lines though. + // LOG_INFO(line); + char *pos = strstr(line, "Rain = "); + if (pos != NULL) { + strlcpy(rainStr, pos + 17, sizeof(rainStr)); // 17 spaces for ws85 + rain = strtof(rainStr, nullptr); + } } } - } - // Update lineStart for the next line - lineStart = lineEnd + 1; + // Update lineStart for the next line + lineStart = lineEnd + 1; + } } } break;