Fix memory issues

.github/workflows/tests.yml

@@ -18,4 +18,4 @@ jobs:
           CC:   gcc-13
           CXX:  g++-13
       - run: cmake --build build --parallel --target DCCEXProtocolTests
-      - run: ctest --test-dir build --schedule-random
+      - run: ./build/tests/DCCEXProtocolTests --gtest_shuffle

CMakeLists.txt

@@ -8,8 +8,8 @@ file(GLOB_RECURSE SRC src/*.cpp)
 add_library(DCCEXProtocol STATIC ${SRC})
 add_library(DCCEX::Protocol ALIAS DCCEXProtocol)
 
-# Requires at least C++11
-target_compile_features(DCCEXProtocol PUBLIC cxx_std_11)
+# Stuck at C++11
+target_compile_options(DCCEXProtocol PRIVATE -std=c++11)
 
 # Don't bother users with warnings by setting 'SYSTEM'
 if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
@@ -36,13 +36,7 @@ endforeach()
 
 source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR} FILES ${SRC})
 
-if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
-  include(CTest)
+if(PROJECT_IS_TOP_LEVEL)
   add_subdirectory(docs)
-endif()
-
-if(BUILD_TESTING
-   AND CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME
-   AND CMAKE_SYSTEM_NAME STREQUAL CMAKE_HOST_SYSTEM_NAME)
   add_subdirectory(tests)
 endif()

cmake/sanitize.cmake

@@ -1,17 +1,20 @@
 macro(sanitize SANITIZERS)
+  # Set in current scope
+  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=${SANITIZERS}")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=${SANITIZERS}")
+  set(LDFLAGS "${LDFLAGS} -fsanitize=${SANITIZERS}")
+
+  # Set in PARENT_SCOPE
   get_directory_property(HAS_PARENT_SCOPE PARENT_DIRECTORY)
   if(HAS_PARENT_SCOPE)
     set(CMAKE_C_FLAGS
-        "${CMAKE_C_FLAGS} -fsanitize=${SANITIZERS}"
+        ${CMAKE_C_FLAGS}
         PARENT_SCOPE)
     set(CMAKE_CXX_FLAGS
-        "${CMAKE_CXX_FLAGS} -fsanitize=${SANITIZERS}"
+        ${CMAKE_CXX_FLAGS}
         PARENT_SCOPE)
     set(LDFLAGS
-        "${LDFLAGS} -fsanitize=${SANITIZERS}"
+        ${LDFLAGS}
         PARENT_SCOPE)
   endif()
-  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=${SANITIZERS}")
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=${SANITIZERS}")
-  set(LDFLAGS "${LDFLAGS} -fsanitize=${SANITIZERS}")
 endmacro()

src/DCCEXInbound.cpp

@@ -48,21 +48,28 @@ enum splitState : byte {
   COMPLETE_i_COMMAND
 };
 
-int16_t DCCEXInbound::_maxParams;
-int16_t DCCEXInbound::_parameterCount;
-byte DCCEXInbound::_opcode;
-int32_t *DCCEXInbound::_parameterValues;
-char *DCCEXInbound::_cmdBuffer;
+int16_t DCCEXInbound::_maxParams = 0;
+int16_t DCCEXInbound::_parameterCount = 0;
+byte DCCEXInbound::_opcode = 0;
+int32_t *DCCEXInbound::_parameterValues = nullptr;
+char *DCCEXInbound::_cmdBuffer = nullptr;
 
 // Public methods
 
 void DCCEXInbound::setup(int16_t maxParameterValues) {
-  _parameterValues = (int32_t *)malloc(maxParameterValues * sizeof(int32_t));
+  _parameterValues = (int32_t *)realloc(_parameterValues, maxParameterValues * sizeof(int32_t));
   _maxParams = maxParameterValues;
   _parameterCount = 0;
   _opcode = 0;
 }
 
+void DCCEXInbound::cleanup() {
+  if (_parameterValues) {
+    free(_parameterValues);
+    _parameterValues = nullptr;
+  }
+}
+
 byte DCCEXInbound::getOpcode() { return _opcode; }
 
 int16_t DCCEXInbound::getParameterCount() { return _parameterCount; }
@@ -81,19 +88,18 @@ bool DCCEXInbound::isTextParameter(int16_t parameterNumber) {
   return _isTextInternal(parameterNumber);
 }
 
-char *DCCEXInbound::getText(int16_t parameterNumber) {
+char *DCCEXInbound::getTextParameter(int16_t parameterNumber) {
   if (parameterNumber < 0 || parameterNumber >= _parameterCount)
     return 0;
   if (!_isTextInternal(parameterNumber))
     return 0;
   return _cmdBuffer + (_parameterValues[parameterNumber] & ~QUOTE_FLAG_AREA);
-  ;
 }
 
-char *DCCEXInbound::getSafeText(int16_t parameterNumber) {
-  char *unsafe = getText(parameterNumber);
+char *DCCEXInbound::copyTextParameter(int16_t parameterNumber) {
+  char *unsafe = getTextParameter(parameterNumber);
   if (!unsafe)
-    return unsafe; // bad parameter number probabaly
+    return unsafe; // bad parameter number probably
   char *safe = (char *)malloc(strlen(unsafe) + 1);
   strcpy(safe, unsafe);
   return safe;
@@ -203,10 +209,10 @@ void DCCEXInbound::dump(Print *out) {
 
   for (int i = 0; i < getParameterCount(); i++) {
     if (isTextParameter(i)) {
-      out->print(F("getText("));
+      out->print(F("getTextParameter("));
       out->print(i);
       out->print(F(")=\""));
-      out->print(getText(i));
+      out->print(getTextParameter(i));
       out->println('"');
     } else {
       out->print(F("getNumber("));

src/DCCEXInbound.h

@@ -49,6 +49,9 @@ class DCCEXInbound {
   /// @param maxParameterValues Maximum parameter values to accommodate
   static void setup(int16_t maxParameterValues);
 
+  /// @brief Cleanup parser
+  static void cleanup();
+
   /// @brief Pass in a command string to parse
   /// @param command Char array of command to parse
   /// @return True if parsed ok, false if badly terminated command or too many parameters
@@ -74,12 +77,12 @@ class DCCEXInbound {
   ///         does not create permanent copy
   /// @param parameterNumber The number of the parameter to retrieve
   /// @return Char array of text (use once and discard)
-  static char *getText(int16_t parameterNumber);
+  static char *getTextParameter(int16_t parameterNumber);
 
   /// @brief gets address of a heap copy of text type parameter.
   /// @param parameterNumber
   /// @return
-  static char *getSafeText(int16_t parameterNumber);
+  static char *copyTextParameter(int16_t parameterNumber);
 
   /// @brief dump list of parameters obtained
   /// @param out Address of output e.g. &Serial

src/DCCEXLoco.cpp

@@ -29,7 +29,6 @@
 #include "DCCEXLoco.h"
 #include <Arduino.h>
 
-
 // class Loco
 // Public methods
 
@@ -72,7 +71,7 @@ Direction Loco::getDirection() { return (Direction)_direction; }
 LocoSource Loco::getSource() { return (LocoSource)_source; }
 
 void Loco::setupFunctions(char *functionNames) {
-  // Importtant note:
+  // Important note:
   // The functionNames string is modified in place.
   //   console->print(F("Splitting \""));
   //   console->print(functionNames);

src/DCCEXProtocol.cpp

@@ -62,6 +62,14 @@ DCCEXProtocol::DCCEXProtocol(int maxCmdBuffer) {
   _bufflen = 0;
 }
 
+DCCEXProtocol::~DCCEXProtocol() {
+  // Free memory for command buffer
+  delete[] (_cmdBuffer);
+
+  // Cleanup command parser
+  DCCEXInbound::cleanup();
+}
+
 // Set the delegate instance for callbacks
 void DCCEXProtocol::setDelegate(DCCEXProtocolDelegate *delegate) { this->_delegate = delegate; }
 
@@ -153,11 +161,11 @@ void DCCEXProtocol::requestServerVersion() {
 
 bool DCCEXProtocol::receivedVersion() { return _receivedVersion; }
 
-int DCCEXProtocol::getMajorVersion() { return _majorVersion; }
+int DCCEXProtocol::getMajorVersion() { return _version[0]; }
 
-int DCCEXProtocol::getMinorVersion() { return _minorVersion; }
+int DCCEXProtocol::getMinorVersion() { return _version[1]; }
 
-int DCCEXProtocol::getPatchVersion() { return _patchVersion; }
+int DCCEXProtocol::getPatchVersion() { return _version[2]; }
 
 unsigned long DCCEXProtocol::getLastServerResponseTime() { return _lastServerResponseTime; }
 
@@ -572,7 +580,7 @@ void DCCEXProtocol::_processCommand() {
         }
       } else if (DCCEXInbound::getNumber(0) == 'P') { // Receive turntable position info
         if (DCCEXInbound::getParameterCount() == 5 &&
-            DCCEXInbound::isTextParameter(4)) { // Turntable position index enry
+            DCCEXInbound::isTextParameter(4)) { // Turntable position index entry
           _processTurntableIndexEntry();
         }
       } else if (DCCEXInbound::getNumber(0) == 'R') { // Receive roster info
@@ -618,57 +626,50 @@ void DCCEXProtocol::_processCommand() {
 }
 
 void DCCEXProtocol::_processServerDescription() { //<iDCCEX version / microprocessorType / MotorControllerType /
-                                                  //buildNumber>
+                                                  // buildNumber>
   // console->println(F("processServerDescription()"));
   if (_delegate) {
-    char *description;
-    description = (char *)malloc(strlen(DCCEXInbound::getSafeText(0)) + 1);
-    sprintf(description, "%s", DCCEXInbound::getText(0));
-    int versionStartAt = 7; // e.g. "DCC-EX V-"
-    char *temp = _nextServerDescriptionParam(description, versionStartAt, true);
-    _majorVersion = atoi(temp);
-    versionStartAt = versionStartAt + strlen(temp) + 1;
-    temp = _nextServerDescriptionParam(description, versionStartAt, true);
-    _minorVersion = atoi(temp);
-    versionStartAt = versionStartAt + strlen(temp) + 1;
-    temp = _nextServerDescriptionParam(description, versionStartAt, true);
-    _patchVersion = atoi(temp);
+    char *description{DCCEXInbound::getTextParameter(0) + 7};
+    int *version = _version;
+
+    while (description < _cmdBuffer + _maxCmdBuffer) {
+      // Delimiter
+      char const delim = *description++;
+      if (delim != '-' && delim != '.')
+        continue;
+
+      // Int
+      char const first_digit = *description;
+      if (!isdigit(first_digit))
+        continue;
+
+      // string to int
+      int const v = atoi(description);
+      if (v < 0)
+        return; // Error
+      else if (v < 10)
+        description += 1;
+      else if (v < 100)
+        description += 2;
+      else if (v < 1000)
+        description += 3;
+      else
+        return; // Error
+
+      // Done after 3 numbers
+      *version++ = v;
+      if (version - _version >= 3)
+        break;
+    }
+
     _receivedVersion = true;
-    _delegate->receivedServerVersion(_majorVersion, _minorVersion, _patchVersion);
+    _delegate->receivedServerVersion(_version[0], _version[1], _version[2]);
   }
   // console->println(F("processServerDescription(): end"));
 }
 
 void DCCEXProtocol::_processMessage() { //<m "message">
-  _delegate->receivedMessage(DCCEXInbound::getSafeText(0));
-}
-
-char *DCCEXProtocol::_nextServerDescriptionParam(char *description, int startAt, bool lookingAtVersionNumber) {
-  char _tempString[MAX_SERVER_DESCRIPTION_PARAM_LENGTH];
-  int i = 0;
-  size_t j;
-  bool started = false;
-  for (j = startAt; j < strlen(description) && i < (MAX_SERVER_DESCRIPTION_PARAM_LENGTH - 1); j++) {
-    if (started) {
-      if (description[j] == ' ' || description[j] == '\0')
-        break;
-      if (lookingAtVersionNumber && (description[j] == '-' || description[j] == '.'))
-        break;
-      _tempString[i] = description[j];
-      i++;
-    } else {
-      if (description[j] == ' ')
-        started = true;
-      if (lookingAtVersionNumber && (description[j] == '-' || description[j] == '.'))
-        started = true;
-    }
-  }
-  _tempString[i] = '\0';
-  char *_result;
-  _result = (char *)malloc(strlen(_tempString));
-  sprintf(_result, "%s", _tempString);
-  // console->println(_result);
-  return _result;
+  _delegate->receivedMessage(DCCEXInbound::getTextParameter(0));
 }
 
 // Consist/loco methods
@@ -781,8 +782,8 @@ void DCCEXProtocol::_processRosterEntry() { //<jR id ""|"desc" ""|"funct1/funct2
   // console->println(F("processRosterEntry()"));
   // find the roster entry to update
   int address = DCCEXInbound::getNumber(1);
-  char *name = DCCEXInbound::getSafeText(2);
-  char *funcs = DCCEXInbound::getSafeText(3);
+  char *name = DCCEXInbound::copyTextParameter(2);
+  char *funcs = DCCEXInbound::copyTextParameter(3);
   bool missingRosters = false;
 
   Loco *loco = roster->getByAddress(address);
@@ -854,7 +855,7 @@ void DCCEXProtocol::_processTurnoutEntry() {
   // find the turnout entry to update
   int id = DCCEXInbound::getNumber(1);
   bool thrown = (DCCEXInbound::getNumber(2) == 'T');
-  char *name = DCCEXInbound::getSafeText(3);
+  char *name = DCCEXInbound::copyTextParameter(3);
   bool missingTurnouts = false;
 
   Turnout *t = turnouts->getById(id);
@@ -938,7 +939,7 @@ void DCCEXProtocol::_processRouteEntry() {
   // find the Route entry to update
   int id = DCCEXInbound::getNumber(1);
   RouteType type = (RouteType)DCCEXInbound::getNumber(2);
-  char *name = DCCEXInbound::getSafeText(3);
+  char *name = DCCEXInbound::copyTextParameter(3);
   bool missingRoutes = false;
 
   Route *r = routes->getById(id);
@@ -1008,7 +1009,7 @@ void DCCEXProtocol::_processTurntableEntry() { // <jO id type position position_
   TurntableType ttType = (TurntableType)DCCEXInbound::getNumber(2);
   int index = DCCEXInbound::getNumber(3);
   int indexCount = DCCEXInbound::getNumber(4);
-  char *name = DCCEXInbound::getSafeText(5);
+  char *name = DCCEXInbound::copyTextParameter(5);
 
   Turntable *tt = turntables->getById(id);
   if (tt) {
@@ -1040,7 +1041,7 @@ void DCCEXProtocol::_processTurntableIndexEntry() { // <jP id index angle "[desc
     int ttId = DCCEXInbound::getNumber(1);
     int index = DCCEXInbound::getNumber(2);
     int angle = DCCEXInbound::getNumber(3);
-    char *name = DCCEXInbound::getSafeText(4);
+    char *name = DCCEXInbound::copyTextParameter(4);
     if (index == 0) { // Index 0 is always home, and never has a label, so set one
       sprintf(name, "Home");
     }

src/DCCEXProtocol.h

@@ -20,7 +20,7 @@
  *
  * All other rights reserved.
  *
- * This library is aimed at making thinges easier for throttle developers writing software for
+ * This library is aimed at making things easier for throttle developers writing software for
  * Arduino based hardware throttles that wish to use DCC-EX EX-CommandStation native API
  * commands.
  *
@@ -183,6 +183,9 @@ class DCCEXProtocol {
   /// @param maxCmdBuffer Optional - maximum number of bytes for the command buffer (default 500)
   DCCEXProtocol(int maxCmdBuffer = 500);
 
+  /// @brief Destructor for the DCCEXProtocol object
+  ~DCCEXProtocol();
+
   /// @brief Set the delegate object for callbacks
   /// @param delegate
   void setDelegate(DCCEXProtocolDelegate *delegate);
@@ -438,7 +441,6 @@ class DCCEXProtocol {
   void _processCommand();
   void _processServerDescription();
   void _processMessage();
-  char *_nextServerDescriptionParam(char *description, int startAt, bool lookingAtVersionNumber);
 
   // Consist/loco methods
   void _processLocoBroadcast();
@@ -489,9 +491,7 @@ class DCCEXProtocol {
   int _turnoutCount = 0;                              // Count of turnout objects received
   int _routeCount = 0;                                // Count of route objects received
   int _turntableCount = 0;                            // Count of turntable objects received
-  int _majorVersion = 0;                              // EX-CommandStation major version X.y.z
-  int _minorVersion = 0;                              // EX-CommandStation minor version x.Y.z
-  int _patchVersion = 0;                              // EX-CommandStation patch version x.y.Z
+  int _version[3] = {};                               // EX-CommandStation version x.y.z
   Stream *_stream;                                    // Stream object where commands are sent/received
   Stream *_console;                                   // Stream object for console output
   NullStream _nullStream;                             // Send streams to null if no object provided