Merge pull request firmata#76 from firmata/memory-leak-fix

fixed memory leak in string buffer

Author: soundanalogous

Firmata.cpp

@@ -170,17 +170,24 @@ void FirmataClass::processSysexMessage(void)
   case STRING_DATA:
     if(currentStringCallback) {
       byte bufferLength = (sysexBytesRead - 1) / 2;
-      char *buffer = (char*)malloc(bufferLength * sizeof(char));
       byte i = 1;
       byte j = 0;
       while(j < bufferLength) {
-        buffer[j] = (char)storedInputData[i];
+        // The string length will only be at most half the size of the
+        // stored input buffer so we can decode the string within the buffer.
+        storedInputData[j] = storedInputData[i];
         i++;
-        buffer[j] += (char)(storedInputData[i] << 7);
+        storedInputData[j] += (storedInputData[i] << 7);
         i++;
         j++;
       }
-      (*currentStringCallback)(buffer);
+      // Make sure string is null terminated. This may be the case for data
+      // coming from client libraries in languages that don't null terminate
+      // strings.
+      if (storedInputData[j-1] != '\0') {
+        storedInputData[j] = '\0';
+      }
+      (*currentStringCallback)((char*)&storedInputData[0]);
     }
     break;
   default:

Firmata.h

@@ -21,9 +21,9 @@
  * installed firmware. */
 #define FIRMATA_MAJOR_VERSION   2 // for non-compatible changes
 #define FIRMATA_MINOR_VERSION   3 // for backwards compatible changes
-#define FIRMATA_BUGFIX_VERSION  6 // for bugfix releases
+#define FIRMATA_BUGFIX_VERSION  7 // for bugfix releases
 
-#define MAX_DATA_BYTES 32 // max number of data bytes in non-Sysex messages
+#define MAX_DATA_BYTES 64 // max number of data bytes in incoming messages
 
 // message command bytes (128-255/0x80-0xFF)
 #define DIGITAL_MESSAGE         0x90 // send data for a digital pin

examples/EchoString/EchoString.ino

@@ -28,7 +28,7 @@ void sysexCallback(byte command, byte argc, byte*argv)
 
 void setup()
 {
-    Firmata.setFirmwareVersion(0, 1);
+    Firmata.setFirmwareVersion(FIRMATA_MAJOR_VERSION, FIRMATA_MINOR_VERSION);
     Firmata.attach(STRING_DATA, stringCallback);
     Firmata.attach(START_SYSEX, sysexCallback);
     Firmata.begin(57600);

test/unit/firmata_test/firmata_test.ino

@@ -18,21 +18,6 @@ void loop()
   Test::run();
 }
 
-// Note: this test required adding a method (Firmata.unsetFirmwareVersion()) to 
-// Firmata.cpp solely for the purpose of running this test. The method has been 
-// removed from Firmata.cpp, but keeping the test here as a recored 
-// test(setFirmwareVersionDoesNotLeakMemory)
-// {
-//   Firmata.setFirmwareVersion(1, 0);
-//   int initialMemory = freeMemory();
-
-//   Firmata.setFirmwareVersion(1, 0);
-
-//   assertEquals(0, initialMemory - freeMemory());
-  
-//   Firmata.unsetFirmwareVersion();
-// }
-
 test(beginPrintsVersion)
 {
   FakeStream stream;
@@ -140,3 +125,13 @@ test(specifiedDigitalWritePort)
 
   assertEqual(1, _digitalPort);
 }
+
+test(setFirmwareVersionDoesNotLeakMemory)
+{
+  Firmata.setFirmwareVersion(1, 0);
+  int initialMemory = freeMemory();
+
+  Firmata.setFirmwareVersion(1, 0);
+
+  assertEqual(0, initialMemory - freeMemory());
+}