From db29102aa2ac4a11428978dc35e2b3cb0c5c0dcd Mon Sep 17 00:00:00 2001 From: lathoub Date: Thu, 15 Jul 2021 21:33:44 +0200 Subject: [PATCH 1/8] test for mallformed packages --- src/AppleMIDI_Defs.h | 3 ++- src/rtpMIDI_Parser_MidiCommandSection.hpp | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/AppleMIDI_Defs.h b/src/AppleMIDI_Defs.h index 07ca036..46ed883 100644 --- a/src/AppleMIDI_Defs.h +++ b/src/AppleMIDI_Defs.h @@ -31,7 +31,8 @@ enum parserReturn: uint8_t Processed, NotSureGiveMeMoreData, NotEnoughData, - UnexpectedData, + UnexpectedData, + UnexpectedMidiData, }; #if defined(__AVR__) diff --git a/src/rtpMIDI_Parser_MidiCommandSection.hpp b/src/rtpMIDI_Parser_MidiCommandSection.hpp index 9063066..190cbc4 100644 --- a/src/rtpMIDI_Parser_MidiCommandSection.hpp +++ b/src/rtpMIDI_Parser_MidiCommandSection.hpp @@ -34,6 +34,11 @@ parserReturn decodeMidiSection(RtpBuffer_t &buffer) return parserReturn::NotEnoughData; } + if (consumed > midiCommandLength) { + buffer.clear(); + return parserReturn::UnexpectedMidiData; + } + midiCommandLength -= consumed; while (consumed--) From 59e03b7c82b4d6e6c0ef57207b439dab7ebcdb64 Mon Sep 17 00:00:00 2001 From: lathoub Date: Fri, 16 Jul 2021 06:28:06 +0200 Subject: [PATCH 2/8] additional parser checking --- src/AppleMIDI_Defs.h | 1 + src/rtpMIDI_Parser.h | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/AppleMIDI_Defs.h b/src/AppleMIDI_Defs.h index 46ed883..2284c05 100644 --- a/src/AppleMIDI_Defs.h +++ b/src/AppleMIDI_Defs.h @@ -33,6 +33,7 @@ enum parserReturn: uint8_t NotEnoughData, UnexpectedData, UnexpectedMidiData, + UnexpectedJournalData, }; #if defined(__AVR__) diff --git a/src/rtpMIDI_Parser.h b/src/rtpMIDI_Parser.h index c34a642..e2676e8 100644 --- a/src/rtpMIDI_Parser.h +++ b/src/rtpMIDI_Parser.h @@ -153,8 +153,14 @@ class rtpMIDIParser if (midiCommandLength > 0) { auto retVal = decodeMidiSection(buffer); - if (retVal != parserReturn::Processed) + switch (retVal) { + case parserReturn::Processed: + break; + case parserReturn::UnexpectedMidiData: + _rtpHeadersComplete = false; + default: return retVal; + } } // The payload MAY also contain a journal section. The journal section @@ -165,8 +171,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; From b0ed165cbb486aae9a70fcb29d5bdb69a6a7cd4b Mon Sep 17 00:00:00 2001 From: lathoub Date: Fri, 16 Jul 2021 06:37:37 +0200 Subject: [PATCH 3/8] Update rtpMIDI_Parser.h --- src/rtpMIDI_Parser.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rtpMIDI_Parser.h b/src/rtpMIDI_Parser.h index e2676e8..0fe0cc8 100644 --- a/src/rtpMIDI_Parser.h +++ b/src/rtpMIDI_Parser.h @@ -157,6 +157,7 @@ class rtpMIDIParser case parserReturn::Processed: break; case parserReturn::UnexpectedMidiData: + // already processed MIDI data will be played _rtpHeadersComplete = false; default: return retVal; From 278f7ef0a2232f39d4cb0f7a6a7dd766abc26249 Mon Sep 17 00:00:00 2001 From: lathoub Date: Mon, 19 Jul 2021 07:01:22 +0200 Subject: [PATCH 4/8] added comments --- src/rtpMIDI_Parser.h | 1 + src/rtpMIDI_Parser_MidiCommandSection.hpp | 38 +++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/rtpMIDI_Parser.h b/src/rtpMIDI_Parser.h index 0fe0cc8..f662ef9 100644 --- a/src/rtpMIDI_Parser.h +++ b/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 diff --git a/src/rtpMIDI_Parser_MidiCommandSection.hpp b/src/rtpMIDI_Parser_MidiCommandSection.hpp index 190cbc4..0817d50 100644 --- a/src/rtpMIDI_Parser_MidiCommandSection.hpp +++ b/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! */ From 744e84ecda472b5cd173448f65489e62bb26738a Mon Sep 17 00:00:00 2001 From: lathoub Date: Fri, 20 Aug 2021 11:13:26 +0200 Subject: [PATCH 5/8] added test for long sessionNames --- src/AppleMIDI_Defs.h | 1 + test/Ethernet.h | 95 ++++++++++++++++---------------------------- 2 files changed, 35 insertions(+), 61 deletions(-) diff --git a/src/AppleMIDI_Defs.h b/src/AppleMIDI_Defs.h index 2284c05..d87e7b8 100644 --- a/src/AppleMIDI_Defs.h +++ b/src/AppleMIDI_Defs.h @@ -34,6 +34,7 @@ enum parserReturn: uint8_t UnexpectedData, UnexpectedMidiData, UnexpectedJournalData, + SessionNameVeryLong, }; #if defined(__AVR__) diff --git a/test/Ethernet.h b/test/Ethernet.h index 8ff9018..9d16b35 100644 --- a/test/Ethernet.h +++ b/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(notOKSessionName, sizeof(notOKSessionName)); + } 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,19 @@ class EthernetUDP return _buffer.size(); }; + int read() + { + int uu = _buffer.size(); + + 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 +395,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(); }; From d3e08f961306423c05f9018664e8a62863a15dc4 Mon Sep 17 00:00:00 2001 From: lathoub Date: Fri, 20 Aug 2021 11:13:42 +0200 Subject: [PATCH 6/8] fixing long session name --- src/AppleMIDI.hpp | 5 +++++ src/AppleMIDI_Parser.h | 28 ++++++++++++++++------------ 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/AppleMIDI.hpp b/src/AppleMIDI.hpp index 8a56b41..8482b34 100644 --- a/src/AppleMIDI.hpp +++ b/src/AppleMIDI.hpp @@ -38,6 +38,11 @@ void AppleMIDISession::parseControlPackets() #endif controlBuffer.pop_front(); } + else if (retVal == parserReturn::SessionNameVeryLong) + { + // purge the rest of the data in controlPort + while (controlPort.read() >= 0) {} + } } } diff --git a/src/AppleMIDI_Parser.h b/src/AppleMIDI_Parser.h index 198740e..22778db 100644 --- a/src/AppleMIDI_Parser.h +++ b/src/AppleMIDI_Parser.h @@ -89,18 +89,17 @@ class AppleMIDIParser while ((i < buffer.size()) && (buffer[i] != 0x00)) i++; #endif - // session name is optional. - // If i > minimum size (16), then a sessionName was provided and must include 0x00 - if (i > minimumLen) - if (i == buffer.size() || buffer[i++] != 0x00) - return parserReturn::NotEnoughData; + parserReturn parserReturn = parserReturn::Processed; + + if (i > minimumLen && i == buffer.size() && buffer[i] != 0x00) + parserReturn = parserReturn::SessionNameVeryLong; while (i--) buffer.pop_front(); // consume all the bytes that made up this message session->ReceivedInvitation(invitation, portType); - return parserReturn::Processed; + return parserReturn; } else if (0 == memcmp(command, amEndSession, sizeof(amEndSession))) { @@ -278,18 +277,23 @@ class AppleMIDIParser while ((i < buffer.size()) && (buffer[i] != 0x00)) i++; #endif - // session name is optional. - // If i > minimum size (16), then a sessionName was provided and must include 0x00 - if (i > minimumLen) - if (i == buffer.size() || buffer[i++] != 0x00) - return parserReturn::NotEnoughData; + //// session name is optional. + //// If i > minimum size (16), then a sessionName was provided and must include 0x00 + //if (i > minimumLen) + // if (i == buffer.size() || buffer[i++] != 0x00) + // return parserReturn::NotEnoughData; + + parserReturn parserReturn = parserReturn::Processed; + + if (i > minimumLen && i == buffer.size() && buffer[i] != 0x00) + parserReturn = parserReturn::SessionNameVeryLong; while (i--) buffer.pop_front(); // consume all the bytes that made up this message session->ReceivedInvitationAccepted(invitationAccepted, portType); - return parserReturn::Processed; + return parserReturn; } else if (0 == memcmp(command, amInvitationRejected, sizeof(amInvitationRejected))) { From ae67e78e5c5250befbbfec4a908cf8a458fe7d5c Mon Sep 17 00:00:00 2001 From: lathoub Date: Fri, 20 Aug 2021 14:45:09 +0200 Subject: [PATCH 7/8] cont fix long session names --- src/AppleMIDI_Parser.h | 37 ++++++++++++++++++++----------------- test/Ethernet.h | 4 +--- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/AppleMIDI_Parser.h b/src/AppleMIDI_Parser.h index 22778db..593bdb1 100644 --- a/src/AppleMIDI_Parser.h +++ b/src/AppleMIDI_Parser.h @@ -78,27 +78,31 @@ 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 parserReturn parserReturn = parserReturn::Processed; - if (i > minimumLen && i == buffer.size() && buffer[i] != 0x00) - parserReturn = parserReturn::SessionNameVeryLong; + if (i > minimumLen) + if (i == buffer.size() && buffer[i] != 0x00) + parserReturn = parserReturn::SessionNameVeryLong; while (i--) buffer.pop_front(); // consume all the bytes that made up this message session->ReceivedInvitation(invitation, portType); + int yy = buffer.size(); + return parserReturn; } else if (0 == memcmp(command, amEndSession, sizeof(amEndSession))) @@ -266,31 +270,30 @@ 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 - //if (i > minimumLen) - // if (i == buffer.size() || buffer[i++] != 0x00) - // return parserReturn::NotEnoughData; parserReturn parserReturn = parserReturn::Processed; - if (i > minimumLen && i == buffer.size() && buffer[i] != 0x00) - parserReturn = parserReturn::SessionNameVeryLong; + if (i > minimumLen) + if (i == buffer.size() && buffer[i] != 0x00) + parserReturn = parserReturn::SessionNameVeryLong; while (i--) buffer.pop_front(); // consume all the bytes that made up this message + int yy = buffer.size(); + session->ReceivedInvitationAccepted(invitationAccepted, portType); return parserReturn; diff --git a/test/Ethernet.h b/test/Ethernet.h index 9d16b35..086caa2 100644 --- a/test/Ethernet.h +++ b/test/Ethernet.h @@ -41,7 +41,7 @@ class EthernetUDP 0x6f, 0x72, 0x74, 0x20, 0x28, 0x36, 0x34, 0x37, 0x38, 0x3a, 0x30, 0x29, 0x00 }; - write(notOKSessionName, sizeof(notOKSessionName)); + write(okSessionName, sizeof(okSessionName)); } @@ -360,8 +360,6 @@ class EthernetUDP int read() { - int uu = _buffer.size(); - if (_buffer.size() == 0) return -1; From be0be95c14db975897e9e48ae88f4b59669727ce Mon Sep 17 00:00:00 2001 From: lathoub Date: Fri, 20 Aug 2021 21:27:37 +0200 Subject: [PATCH 8/8] Update AppleMIDI_Parser.h added comments --- src/AppleMIDI_Parser.h | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/AppleMIDI_Parser.h b/src/AppleMIDI_Parser.h index 593bdb1..940fa13 100644 --- a/src/AppleMIDI_Parser.h +++ b/src/AppleMIDI_Parser.h @@ -90,20 +90,23 @@ class AppleMIDIParser while (i < buffer.size()) i++; #endif - parserReturn parserReturn = parserReturn::Processed; + 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) - parserReturn = parserReturn::SessionNameVeryLong; + retVal = parserReturn::SessionNameVeryLong; while (i--) buffer.pop_front(); // consume all the bytes that made up this message session->ReceivedInvitation(invitation, portType); - int yy = buffer.size(); - - return parserReturn; + return retVal; } else if (0 == memcmp(command, amEndSession, sizeof(amEndSession))) { @@ -283,20 +286,23 @@ class AppleMIDIParser i++; #endif - parserReturn parserReturn = parserReturn::Processed; + 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) - parserReturn = parserReturn::SessionNameVeryLong; + retVal = parserReturn::SessionNameVeryLong; while (i--) buffer.pop_front(); // consume all the bytes that made up this message - int yy = buffer.size(); - session->ReceivedInvitationAccepted(invitationAccepted, portType); - return parserReturn; + return retVal; } else if (0 == memcmp(command, amInvitationRejected, sizeof(amInvitationRejected))) {