Merge pull request #129 from lathoub/mallformed_packets

Fix for Malformed packets and long session names
Thanks @folkertvanheusden and @hallvardkristiansen for reporting and fixing

Author: lathoub

src/AppleMIDI.hpp

@@ -38,6 +38,11 @@ void AppleMIDISession<UdpClass, Settings, Platform>::parseControlPackets()
 #endif            
             controlBuffer.pop_front();
         }
+        else if (retVal == parserReturn::SessionNameVeryLong)
+        {
+            // purge the rest of the data in controlPort
+            while (controlPort.read() >= 0) {}
+        }
     }
 }
 

src/AppleMIDI_Defs.h

@@ -31,7 +31,10 @@ enum parserReturn: uint8_t
     Processed,
     NotSureGiveMeMoreData,
     NotEnoughData,
-    UnexpectedData,
+	UnexpectedData,
+	UnexpectedMidiData,
+	UnexpectedJournalData,
+	SessionNameVeryLong,
 };
 
 #if defined(__AVR__)

src/AppleMIDI_Parser.h

@@ -78,29 +78,35 @@ class AppleMIDIParser
 
 #ifdef KEEP_SESSION_NAME
             uint16_t bi = 0;
-            while ((i < buffer.size()) && (buffer[i] != 0x00))
+            while (i < buffer.size())
             {
                 if (bi < DefaultSettings::MaxSessionNameLen)
-                    invitation.sessionName[bi++] = buffer[i];
-                i++;
+                    invitation.sessionName[bi++] = buffer[i++];
+                else
+                    i++;
             }
             invitation.sessionName[bi++] = '\0';
 #else
-            while ((i < buffer.size()) && (buffer[i] != 0x00))
+            while (i < buffer.size())
                 i++;
 #endif
-            // session name is optional.
-            // If i > minimum size (16), then a sessionName was provided and must include 0x00
+            auto retVal = parserReturn::Processed;
+
+            // when given a Session Name and the buffer has been fully processed and the 
+            // last character is not 'endl', then we got a very long sessionName. It will
+            // continue in the next memory chunk of the packet. We don't care, so indicated
+            // flush the remainder of the packet.
+            // First part if the session name is kept, processing continues
             if (i > minimumLen)
-                if (i == buffer.size() || buffer[i++] != 0x00)
-                    return parserReturn::NotEnoughData;
+                if (i == buffer.size() && buffer[i] != 0x00)
+                    retVal = parserReturn::SessionNameVeryLong;
 
             while (i--)
                 buffer.pop_front(); // consume all the bytes that made up this message
 
 			session->ReceivedInvitation(invitation, portType);
 
-            return parserReturn::Processed;
+            return retVal;
 		}
 		else if (0 == memcmp(command, amEndSession, sizeof(amEndSession)))
 		{
@@ -267,29 +273,36 @@ class AppleMIDIParser
 
 #ifdef KEEP_SESSION_NAME
             uint16_t bi = 0;
-            while ((i < buffer.size()) && (buffer[i] != 0x00))
+            while (i < buffer.size())
             {
                 if (bi < DefaultSettings::MaxSessionNameLen)
-                    invitationAccepted.sessionName[bi++] = buffer[i];
-                i++;
+                    invitationAccepted.sessionName[bi++] = buffer[i++];
+                else
+                    i++;
             }
             invitationAccepted.sessionName[bi++] = '\0';
 #else
-            while ((i < buffer.size()) && (buffer[i] != 0x00))
+            while (i < buffer.size())
                 i++;
 #endif
-            // session name is optional.
-            // If i > minimum size (16), then a sessionName was provided and must include 0x00
+
+            auto retVal = parserReturn::Processed;
+
+            // when given a Session Name and the buffer has been fully processed and the 
+            // last character is not 'endl', then we got a very long sessionName. It will
+            // continue in the next memory chunk of the packet. We don't care, so indicated
+            // flush the remainder of the packet.
+            // First part if the session name is kept, processing continues
             if (i > minimumLen)
-                if (i == buffer.size() || buffer[i++] != 0x00)
-                    return parserReturn::NotEnoughData;
+                if (i == buffer.size() && buffer[i] != 0x00)
+                    retVal = parserReturn::SessionNameVeryLong;
 
             while (i--)
                 buffer.pop_front(); // consume all the bytes that made up this message
 
             session->ReceivedInvitationAccepted(invitationAccepted, portType);
 
-            return parserReturn::Processed;
+            return retVal;
 		}
 		else if (0 == memcmp(command, amInvitationRejected, sizeof(amInvitationRejected)))
 		{

src/rtpMIDI_Parser.h

@@ -116,6 +116,7 @@ class rtpMIDIParser
             if (buffer.size() < minimumLen)
                 return parserReturn::NotSureGiveMeMoreData;
 
+            // 2.2. MIDI Payload (https://www.ietf.org/rfc/rfc4695.html#section-2.2)
             // The payload MUST begin with the MIDI command section. The
             // MIDI command section codes a (possibly empty) list of timestamped
             // MIDI commands and provides the essential service of the payload
@@ -153,8 +154,15 @@ class rtpMIDIParser
 		if (midiCommandLength > 0)
         {
 			auto retVal = decodeMidiSection(buffer);
-            if (retVal != parserReturn::Processed)
+            switch (retVal) {
+            case parserReturn::Processed:
+                break;
+            case parserReturn::UnexpectedMidiData:
+                // already processed MIDI data will be played
+                _rtpHeadersComplete = false;
+            default:
                 return retVal;
+            }
         }
 
         // The payload MAY also contain a journal section. The journal section
@@ -165,8 +173,14 @@ class rtpMIDIParser
         if (rtpMidi_Flags & RTP_MIDI_CS_FLAG_J)
         {
             auto retVal = decodeJournalSection(buffer);
-            if (retVal != parserReturn::Processed)
+            switch (retVal) {
+            case parserReturn::Processed:
+                break;
+            case parserReturn::UnexpectedJournalData:
+                _rtpHeadersComplete = false;
+            default:
                 return retVal;
+            }
         }
 
         _rtpHeadersComplete = false;

src/rtpMIDI_Parser_MidiCommandSection.hpp

@@ -1,7 +1,45 @@
+// https://www.ietf.org/rfc/rfc4695.html#section-3
+
 parserReturn decodeMidiSection(RtpBuffer_t &buffer)
 {
     int cmdCount = 0;
 
+    // https://www.ietf.org/rfc/rfc4695.html#section-3.2
+    // 
+    // The first MIDI channel command in the MIDI list MUST include a status
+    // octet.Running status coding, as defined in[MIDI], MAY be used for
+    // all subsequent MIDI channel commands in the list.As in[MIDI],
+    // System Commonand System Exclusive messages(0xF0 ... 0xF7) cancel
+    // the running status state, but System Real - time messages(0xF8 ...
+    // 0xFF) do not affect the running status state. All System commands in
+    // the MIDI list MUST include a status octet.
+
+    // As we note above, the first channel command in the MIDI list MUST
+    // include a status octet.However, the corresponding command in the
+    // original MIDI source data stream might not have a status octet(in
+    // this case, the source would be coding the command using running
+    // status). If the status octet of the first channel command in the
+    // MIDI list does not appear in the source data stream, the P(phantom)
+    // header bit MUST be set to 1.  In all other cases, the P bit MUST be
+    // set to 0.
+    //
+    // Note that the P bit describes the MIDI source data stream, not the
+    // MIDI list encoding; regardless of the state of the P bit, the MIDI
+    // list MUST include the status octet.
+    // 
+    // As receivers MUST be able to decode running status, sender
+    // implementors should feel free to use running status to improve
+    // bandwidth efficiency. However, senders SHOULD NOT introduce timing
+    // jitter into an existing MIDI command stream through an inappropriate
+    // use or removal of running status coding. This warning primarily
+    // applies to senders whose RTP MIDI streams may be transcoded onto a
+    // MIDI 1.0 DIN cable[MIDI] by the receiver : both the timestamps and
+    // the command coding (running status or not) must comply with the
+    // physical restrictions of implicit time coding over a slow serial
+    // line.
+
+    // (lathoub: RTP_MIDI_CS_FLAG_P((phantom) not implemented
+
     uint8_t runningstatus = 0;
 
     /* Multiple MIDI-commands might follow - the exact number can only be discovered by really decoding the commands! */
@@ -34,6 +72,11 @@ parserReturn decodeMidiSection(RtpBuffer_t &buffer)
                 return parserReturn::NotEnoughData;
             }
 
+            if (consumed > midiCommandLength) {
+                buffer.clear();
+                return parserReturn::UnexpectedMidiData;
+            }
+
             midiCommandLength -= consumed;
 
             while (consumed--)

test/Ethernet.h

@@ -15,6 +15,10 @@ class EthernetUDP
     EthernetUDP()
     {
         _port = 0;
+
+
+
+
     }
 
     void begin(uint16_t port)
@@ -24,6 +28,21 @@ class EthernetUDP
         if (port == DEFAULT_CONTROL_PORT && true)
         {
             // AppleMIDI messages
+            byte okSessionName[] = {
+               0xff, 0xff, 0x49, 0x4e, 0x00, 0x00, 0x00, 0x02, 0x4e, 0x27, 0x95, 0x9e, 0x00, 0x00, 0xec, 0xf9,
+               0x6c, 0x61, 0x70, 0x70, 0x69, 0x65, 0x6d, 0x63, 0x74, 0x6f, 0x70, 0x66, 0x61, 0x63, 0x65, 0x00
+            };
+
+            byte notOKSessionName[] = {
+                0xff, 0xff, 0x49, 0x4e, 0x00, 0x00, 0x00, 0x02, 0xcc, 0x0f, 0x6c, 0x49, 0x00, 0x00, 0xa4, 0x9b, 
+                0x6c, 0x61, 0x70, 0x70, 0x69, 0x65, 0x6d, 0x63, 0x74, 0x6f, 0x70, 0x66, 0x61, 0x63, 0x65, 0x2f,  
+                0x46, 0x4c, 0x55, 0x49, 0x44, 0x20, 0x53, 0x79, 0x6e, 0x74, 0x68, 0x20, 0x28, 0x36, 0x34, 0x37,  
+                0x38, 0x29, 0x2d, 0x53, 0x79, 0x6e, 0x74, 0x68, 0x20, 0x69, 0x6e, 0x70, 0x75, 0x74, 0x20, 0x70,  
+                0x6f, 0x72, 0x74, 0x20, 0x28, 0x36, 0x34, 0x37, 0x38, 0x3a, 0x30, 0x29, 0x00           
+            };
+
+            write(okSessionName, sizeof(okSessionName));
+
         }
 
         if (port == (DEFAULT_CONTROL_PORT + 1) && true)
@@ -309,7 +328,7 @@ class EthernetUDP
 
             byte slecht[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06};
 
-            write(rr2, sizeof(rr2));
+           // write(rr2, sizeof(rr2));
         }
 
         if (port == 5005 && true)
@@ -339,6 +358,17 @@ class EthernetUDP
         return _buffer.size();
     };
 
+    int read()
+    {
+        if (_buffer.size() == 0)
+            return -1; 
+
+        byte value = _buffer.front();
+        _buffer.pop_front();
+
+        return value;
+    }
+
     size_t read(byte* buffer, size_t size)
     {
         size = min(size, _buffer.size());
@@ -363,68 +393,9 @@ class EthernetUDP
     };
 
     void endPacket() { };
+
     void flush()
     {
-        if (_port == 5004)
-        {
-            if (_buffer[0] == 0xff && _buffer[1] == 0xff && _buffer[2] == 'I' &&_buffer[3] == 'N')
-            {
-                _buffer.clear();
-
-                
-                byte u[] = {
-                    0xff, 0xff,
-                    0x4f, 0x4b,
-                    0x00, 0x00, 0x00, 0x02,
-                    0xb7, 0x06, 0x20, 0x30,
-                    0xda, 0x8d, 0xc5, 0x8a,
-                    0x4d, 0x61, 0x63, 0x62, 0x6f, 0x6f, 0x6b, 0x20, 0x50, 0x72, 0x6f, 0x20, 0x66, 0x72, 0x6f, 0x6d, 0x20, 0x53, 0x61, 0x6e, 0x64, 0x72, 0x61, 0x20, 0x56, 0x65, 0x72, 0x62, 0x65, 0x6b, 0x65, 0x6e, 0x20, 0x28, 0x32, 0x29, 0x00 };
-                
-                
-                
-                
-                
-                byte r[] = { 0xff, 0xff,
-                                      0x4f, 0x4b,
-                                      0x00, 0x0, 0x00, 0x02,
-                                      0xb7, 0x06, 0x20, 0x30,
-                                      0xda, 0x8d, 0xc5, 0x8a,
-                                      0x53, 0x65, 0x73, 0x73, 0x69, 0x6, 0x6e, 0x31, 0x2d, 0x42, 0x00 };
-                write(u, sizeof(u));
-            }
-        }
-        if (_port == 5005)
-        {
-            if (_buffer[0] == 0xff && _buffer[1] == 0xff && _buffer[2] == 'I' &&_buffer[3] == 'N')
-            {
-                _buffer.clear();
-                byte r[] = { 0xff, 0xff,
-                                      0x4f, 0x4b,
-                                      0x00, 0x0, 0x00, 0x02,
-                                      0xb7, 0x06, 0x20, 0x30,
-                                      0xda, 0x8d, 0xc5, 0x8a,
-                                      0x53, 0x65, 0x73, 0x73, 0x69, 0x6, 0x6e, 0x31, 0x2d, 0x42, 0x00 };
-                write(r, sizeof(r));
-            }
-            else if (_buffer[0] == 0xff && _buffer[1] == 0xff && _buffer[2] == 'C' &&_buffer[3] == 'K')
-            {
-                if (_buffer[8] == 0x00)
-                {
-                    _buffer.clear();
-                    byte r[] = { 0xff, 0xff,
-                                 0x43, 0x4b,
-                                 0xda, 0x8d, 0xc5, 0x8a,
-                                 0x01,
-                                 0x65, 0x73, 0x73,
-                                 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x17, 0x34,
-                                 0x00, 0x00, 0x00, 0x00, 0x00, 0x0b, 0x6c, 0x83,
-                                 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
-                    write(r, sizeof(r));
-                }
-                else
-                    _buffer.clear();
-            }
-        }
     };
 
     void stop() { _buffer.clear(); };