Skip to content

Commit 45b6eb6

Browse files
lathoublathoub
committed
harden midi parsing and timing state
Fix RTP clock rollover and parser state handling Fix buffer overflow when output queue is full, seed participant sequence numbers on invite, and make parser consume loops safe. normalize protocol constants/types, reset journal state on errors, and improve rtp clock initialization.
1 parent db2141f commit 45b6eb6

File tree

9 files changed

+87
-53
lines changed

9 files changed

+87
-53
lines changed

‎src/AppleMIDI.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,7 @@ class AppleMIDISession
248248
if (nullptr != _exceptionCallback)
249249
_exceptionCallback(ssrc, BufferFullException, 0);
250250
#endif
251+
return;
251252
}
252253
}
253254

‎src/AppleMIDI.hpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ void AppleMIDISession<UdpClass, Settings, Platform>::ReceivedControlInvitation(A
143143
#endif
144144
participant.kind = Listener;
145145
participant.ssrc = invitation.ssrc;
146+
participant.sendSequenceNr = random(1, UINT16_MAX); // http://www.rfc-editor.org/rfc/rfc6295.txt , 2.1. RTP Header
146147
participant.remoteIP = controlPort.remoteIP();
147148
participant.remotePort = controlPort.remotePort();
148149
participant.lastSyncExchangeTime = now;
@@ -1050,6 +1051,7 @@ bool AppleMIDISession<UdpClass, Settings, Platform>::sendInvite(IPAddress ip, ui
10501051
Participant<Settings> participant;
10511052
#endif
10521053
participant.kind = Initiator;
1054+
participant.sendSequenceNr = random(1, UINT16_MAX); // http://www.rfc-editor.org/rfc/rfc6295.txt , 2.1. RTP Header
10531055
participant.remoteIP = ip;
10541056
participant.remotePort = port;
10551057
participant.lastInviteSentTime = now - 1000; // forces invite to be send immediately

‎src/AppleMIDI_Defs.h‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ union conversionBuffer
2222
uint16_t value16;
2323
uint32_t value32;
2424
uint64_t value64;
25-
byte buffer[8];
25+
uint8_t buffer[8];
2626
};
2727

2828

@@ -147,19 +147,19 @@ using sentRtpMidiCallback = void (*)(const RtpMIDI_t&);
147147
#endif
148148

149149
/* Signature "Magic Value" for Apple network MIDI session establishment */
150-
const byte amSignature[] = {0xff, 0xff};
150+
static constexpr uint8_t amSignature[] = {0xff, 0xff};
151151

152152
/* 2 (stored in network byte order (big-endian)) */
153-
const byte amProtocolVersion[] = {0x00, 0x00, 0x00, 0x02};
153+
static constexpr uint8_t amProtocolVersion[] = {0x00, 0x00, 0x00, 0x02};
154154

155155
/* Apple network MIDI valid commands */
156-
const byte amInvitation[] = {'I', 'N'};
157-
const byte amEndSession[] = {'B', 'Y'};
158-
const byte amSynchronization[] = {'C', 'K'};
159-
const byte amInvitationAccepted[] = {'O', 'K'};
160-
const byte amInvitationRejected[] = {'N', 'O'};
161-
const byte amReceiverFeedback[] = {'R', 'S'};
162-
const byte amBitrateReceiveLimit[] = {'R', 'L'};
156+
static constexpr uint8_t amInvitation[] = {'I', 'N'};
157+
static constexpr uint8_t amEndSession[] = {'B', 'Y'};
158+
static constexpr uint8_t amSynchronization[] = {'C', 'K'};
159+
static constexpr uint8_t amInvitationAccepted[] = {'O', 'K'};
160+
static constexpr uint8_t amInvitationRejected[] = {'N', 'O'};
161+
static constexpr uint8_t amReceiverFeedback[] = {'R', 'S'};
162+
static constexpr uint8_t amBitrateReceiveLimit[] = {'R', 'L'};
163163

164164
const uint8_t SYNC_CK0 = 0;
165165
const uint8_t SYNC_CK1 = 1;

‎src/AppleMIDI_Namespace.h‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,4 @@
66
{
77
#define END_APPLEMIDI_NAMESPACE }
88

9-
#define USING_NAMESPACE_APPLEMIDI using namespace APPLEMIDI_NAMESPACE;
10-
11-
BEGIN_APPLEMIDI_NAMESPACE
12-
13-
END_APPLEMIDI_NAMESPACE
9+
#define USING_NAMESPACE_APPLEMIDI using namespace APPLEMIDI_NAMESPACE;

‎src/AppleMIDI_Parser.h‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ class AppleMIDIParser
2121
{
2222
conversionBuffer cb;
2323

24-
byte signature[2]; // Signature "Magic Value" for Apple network MIDI session establishment
25-
byte command[2]; // 16-bit command identifier (two ASCII characters, first in high 8 bits, second in low 8 bits)
24+
uint8_t signature[2]; // Signature "Magic Value" for Apple network MIDI session establishment
25+
uint8_t command[2]; // 16-bit command identifier (two ASCII characters, first in high 8 bits, second in low 8 bits)
2626

2727
size_t minimumLen = (sizeof(signature) + sizeof(command)); // Signature + Command
2828
if (buffer.size() < minimumLen)
@@ -42,7 +42,7 @@ class AppleMIDIParser
4242

4343
if (0 == memcmp(command, amInvitation, sizeof(amInvitation)))
4444
{
45-
byte protocolVersion[4];
45+
uint8_t protocolVersion[4];
4646

4747
minimumLen += (sizeof(protocolVersion) + sizeof(initiatorToken_t) + sizeof(ssrc_t));
4848
if (buffer.size() < minimumLen)
@@ -98,19 +98,22 @@ class AppleMIDIParser
9898
// flush the remainder of the packet.
9999
// First part if the session name is kept, processing continues
100100
if (i > minimumLen)
101-
if (i == buffer.size() && buffer[i] != 0x00)
101+
if (i == buffer.size() && buffer[buffer.size() - 1] != 0x00)
102102
retVal = parserReturn::SessionNameVeryLong;
103103

104-
while (i--)
104+
while (i > 0)
105+
{
105106
buffer.pop_front(); // consume all the bytes that made up this message
107+
--i;
108+
}
106109

107110
session->ReceivedInvitation(invitation, portType);
108111

109112
return retVal;
110113
}
111114
else if (0 == memcmp(command, amEndSession, sizeof(amEndSession)))
112115
{
113-
byte protocolVersion[4];
116+
uint8_t protocolVersion[4];
114117

115118
minimumLen += (sizeof(protocolVersion) + sizeof(initiatorToken_t) + sizeof(ssrc_t));
116119
if (buffer.size() < minimumLen)
@@ -142,8 +145,11 @@ class AppleMIDIParser
142145
cb.buffer[3] = buffer[i++];
143146
endSession.ssrc = __ntohl(cb.value32);
144147

145-
while (i--)
148+
while (i > 0)
149+
{
146150
buffer.pop_front(); // consume all the bytes that made up this message
151+
--i;
152+
}
147153

148154
session->ReceivedEndSession(endSession);
149155

@@ -199,8 +205,11 @@ class AppleMIDIParser
199205
cb.buffer[7] = buffer[i++];
200206
synchronization.timestamps[2] = __ntohll(cb.value64);
201207

202-
while (i--)
208+
while (i > 0)
209+
{
203210
buffer.pop_front(); // consume all the bytes that made up this message
211+
--i;
212+
}
204213

205214
session->ReceivedSynchronization(synchronization);
206215

@@ -237,7 +246,7 @@ class AppleMIDIParser
237246
#ifdef APPLEMIDI_INITIATOR
238247
else if (0 == memcmp(command, amInvitationAccepted, sizeof(amInvitationAccepted)))
239248
{
240-
byte protocolVersion[4];
249+
uint8_t protocolVersion[4];
241250

242251
minimumLen += (sizeof(protocolVersion) + sizeof(initiatorToken_t) + sizeof(ssrc_t));
243252
if (buffer.size() < minimumLen)
@@ -294,19 +303,22 @@ class AppleMIDIParser
294303
// flush the remainder of the packet.
295304
// First part if the session name is kept, processing continues
296305
if (i > minimumLen)
297-
if (i == buffer.size() && buffer[i] != 0x00)
306+
if (i == buffer.size() && buffer[buffer.size() - 1] != 0x00)
298307
retVal = parserReturn::SessionNameVeryLong;
299308

300-
while (i--)
309+
while (i > 0)
310+
{
301311
buffer.pop_front(); // consume all the bytes that made up this message
312+
--i;
313+
}
302314

303315
session->ReceivedInvitationAccepted(invitationAccepted, portType);
304316

305317
return retVal;
306318
}
307319
else if (0 == memcmp(command, amInvitationRejected, sizeof(amInvitationRejected)))
308320
{
309-
byte protocolVersion[4];
321+
uint8_t protocolVersion[4];
310322

311323
minimumLen += (sizeof(protocolVersion) + sizeof(initiatorToken_t) + sizeof(ssrc_t));
312324
if (buffer.size() < minimumLen)
@@ -359,8 +371,11 @@ class AppleMIDIParser
359371
if (i == buffer.size() || buffer[i++] != 0x00)
360372
return parserReturn::NotEnoughData;
361373

362-
while (i--)
374+
while (i > 0)
375+
{
363376
buffer.pop_front(); // consume all the bytes that made up this message
377+
--i;
378+
}
364379

365380
session->ReceivedInvitationRejected(invitationRejected);
366381

@@ -386,8 +401,11 @@ class AppleMIDIParser
386401
cb.buffer[3] = buffer[i++];
387402
bitrateReceiveLimit.bitratelimit = __ntohl(cb.value32);
388403

389-
while (i--)
404+
while (i > 0)
405+
{
390406
buffer.pop_front(); // consume all the bytes that made up this message
407+
--i;
408+
}
391409

392410
session->ReceivedBitrateReceiveLimit(bitrateReceiveLimit);
393411

‎src/AppleMIDI_Participant.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,23 +9,23 @@ BEGIN_APPLEMIDI_NAMESPACE
99
template <class Settings>
1010
struct Participant
1111
{
12-
ParticipantKind kind;
12+
ParticipantKind kind = Listener;
1313
ssrc_t ssrc = 0;
1414
IPAddress remoteIP = INADDR_NONE;
1515
uint16_t remotePort = 0;
1616

17-
unsigned long receiverFeedbackStartTime;
17+
unsigned long receiverFeedbackStartTime = 0;
1818
bool doReceiverFeedback = false;
1919

20-
uint16_t sendSequenceNr = random(1, UINT16_MAX); // http://www.rfc-editor.org/rfc/rfc6295.txt , 2.1. RTP Header
20+
uint16_t sendSequenceNr = 0; // seeded when session/participant is created
2121
uint16_t receiveSequenceNr = 0;
2222

23-
unsigned long lastSyncExchangeTime;
23+
unsigned long lastSyncExchangeTime = 0;
2424

2525
#ifdef APPLEMIDI_INITIATOR
2626
uint8_t connectionAttempts = 0;
2727
uint32_t initiatorToken = 0;
28-
unsigned long lastInviteSentTime;
28+
unsigned long lastInviteSentTime = 0;
2929
InviteStatus invitationStatus = Initiating;
3030

3131
uint8_t synchronizationHeartBeats = 0;
@@ -35,7 +35,7 @@ struct Participant
3535

3636
#ifdef USE_EXT_CALLBACKS
3737
bool firstMessageReceived = true;
38-
uint32_t offsetEstimate;
38+
uint32_t offsetEstimate = 0;
3939
#endif
4040

4141
#ifdef KEEP_SESSION_NAME

‎src/AppleMIDI_Settings.h‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,10 @@ BEGIN_APPLEMIDI_NAMESPACE
66

77
struct DefaultSettings
88
{
9-
static const size_t UdpTxPacketMaxSize = 24;
9+
// Small default to fit constrained MCUs; raise if you send larger SysEx.
10+
static const size_t UdpTxPacketMaxSize = 24;
1011

12+
// MIDI buffer size in bytes; should be >= 3 * max message length.
1113
static const size_t MaxBufferSize = 64;
1214

1315
static const size_t MaxSessionNameLen = 24;
@@ -26,11 +28,12 @@ struct DefaultSettings
2628
// two sequence numbers. The sender can then free the memory containing old journalling data if necessary.
2729
static const unsigned long ReceiversFeedbackThreshold = 1000;
2830

29-
// The initiator must initiate a new sync exchange at least once every 60 seconds
30-
// as in https://developer.apple.com/library/archive/documentation/Audio/Conceptual/MIDINetworkDriverProtocol/MIDI/MIDI.html
31+
// The initiator must initiate a new sync exchange at least once every 60 seconds.
32+
// This value includes a small 1s slack.
33+
// https://developer.apple.com/library/archive/documentation/Audio/Conceptual/MIDINetworkDriverProtocol/MIDI/MIDI.html
3134
static const unsigned long CK_MaxTimeOut = 61000;
3235

33-
// when set to true, the lower 32-bits of the rtpClock ae send
36+
// when set to true, the lower 32-bits of the rtpClock are sent
3437
// when set to false, 0 will be set for immediate playout.
3538
static const bool TimestampRtpPackets = true;
3639

‎src/rtpMIDI_Clock.h‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,22 +14,26 @@ typedef struct rtpMidi_Clock
1414

1515
uint64_t startTime_;
1616
uint64_t initialTimeStamp_;
17+
uint32_t low32_;
18+
uint32_t high32_;
1719

1820
void Init(uint64_t initialTimeStamp, uint32_t clockRate)
1921
{
20-
initialTimeStamp_ = 0;
22+
initialTimeStamp_ = initialTimeStamp;
2123
clockRate_ = clockRate;
2224

2325
if (clockRate_ == 0)
2426
{
2527
clockRate_ = MIDI_SAMPLING_RATE_DEFAULT;
2628
}
2729

30+
low32_ = millis();
31+
high32_ = 0;
2832
startTime_ = Ticks();
2933
}
3034

3135
/// <summary>
32-
/// Returns an timestamp value suitable for inclusion in a RTP packet header.
36+
/// Returns a timestamp value suitable for inclusion in a RTP packet header.
3337
/// </summary>
3438
uint64_t Now()
3539
{
@@ -39,14 +43,12 @@ typedef struct rtpMidi_Clock
3943
private:
4044
uint64_t CalculateCurrentTimeStamp()
4145
{
42-
return (CalculateTimeSpent() * clockRate_) / MSEC_PER_SEC;
46+
return initialTimeStamp_ + (CalculateTimeSpent() * clockRate_) / MSEC_PER_SEC;
4347
}
4448

4549
/// <summary>
4650
/// Returns the time spent since the initial clock timestamp value.
47-
/// The returned value is expressed in units of "clock pulsations",
48-
/// that are equivalent to seconds, scaled by the clock rate.
49-
/// i.e: 1 second difference will result in a delta value equals to the clock rate.
51+
/// The returned value is expressed in milliseconds.
5052
/// </summary>
5153
uint64_t CalculateTimeSpent()
5254
{
@@ -56,14 +58,14 @@ typedef struct rtpMidi_Clock
5658
/// <summary>
5759
/// millis() as a 64bit (not the default 32bit)
5860
/// this prevents wrap around.
61+
/// Note: rollover tracking is per instance; call Init() before use.
5962
/// </summary>
60-
uint64_t Ticks() const
63+
uint64_t Ticks()
6164
{
62-
static uint32_t low32, high32;
6365
uint32_t new_low32 = millis();
64-
if (new_low32 < low32) high32++;
65-
low32 = new_low32;
66-
return (uint64_t) high32 << 32 | low32;
66+
if (new_low32 < low32_) high32_++;
67+
low32_ = new_low32;
68+
return (uint64_t) high32_ << 32 | low32_;
6769
}
6870

6971

‎src/rtpMIDI_Parser.h‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,13 @@ class rtpMIDIParser
3333
void debugPrintBuffer(RtpBuffer_t &buffer)
3434
{
3535
#ifdef DEBUG
36-
for (int i = 0; i < buffer.size(); i++)
36+
for (size_t i = 0; i < buffer.size(); i++)
3737
{
3838
SerialMon.print(" ");
3939
SerialMon.print(i);
4040
SerialMon.print(i < 10 ? " " : " ");
4141
}
42-
for (int i = 0; i < buffer.size(); i++)
42+
for (size_t i = 0; i < buffer.size(); i++)
4343
{
4444
SerialMon.print("0x");
4545
SerialMon.print(buffer[i] < 16 ? "0" : "");
@@ -169,8 +169,11 @@ class rtpMIDIParser
169169
cmdCount = 0;
170170
runningstatus = 0;
171171

172-
while (i--)
172+
while (i > 0)
173+
{
173174
buffer.pop_front();
175+
--i;
176+
}
174177

175178
_rtpHeadersComplete = true;
176179

@@ -202,7 +205,16 @@ class rtpMIDIParser
202205
return parserReturn::NotEnoughData;
203206
case parserReturn::UnexpectedJournalData:
204207
_rtpHeadersComplete = false;
208+
_journalSectionComplete = false;
209+
_channelJournalSectionComplete = false;
210+
_journalTotalChannels = 0;
205211
default:
212+
// Reset all journal state on any non-recoverable error to avoid
213+
// leaking partial state into the next packet.
214+
_rtpHeadersComplete = false;
215+
_journalSectionComplete = false;
216+
_channelJournalSectionComplete = false;
217+
_journalTotalChannels = 0;
206218
return retVal;
207219
}
208220
}

0 commit comments

Comments
 (0)