ntripcaster-2.0.37 UDP-based mountpoint time-out after RTP Sequence number overflow

[emiravalls]

Hello,

We are using the BKG Ntrip Caster in the following setup:

Corrections Data Source [IPv4 / UDP / RTP / RTCM] -> BKG Caster [TCP / NTRIP] <-> Web Proxy [IPv4 / TCP / TLS / NTRIP] <-> BNC

What we found is that the following check in store_udp_data() at main.c:997:

if((int)(seq-con->udpbuffers->seq) > 0)

Is always false once the Corrections Data Source has sent more than 65535 messages, which is the maximum number of messages that can be sent without repeating sequence numbers in RTP (the sequence numbers wrap around 0). After some seconds, the mountpoint expires.

We believe the Caster should be able to support wrap around of sequence number for very long-lived mountpoints. Is that limit imposed in some standard?

The check could be changed to something like

#define MAX_RTP_SEQNUM 0x00FFFFU
#define MAX_PACKETS_IN_FLIGHT (MAX_RTP_SEQNUM/2)
choose a value between 1 and MAX_RTP_SEQNUM

if ((seq != con->udpbuffers->seq) && (((seq - con->udpbuffers->seq) & MAX_RTP_SEQNUM) <= MAX_PACKETS_IN_FLIGHT))

so that it would allow jumps of upto MAX_PACKETS_IN_FLIGHT RTP messages. We have found that this proposed check is similar to the ones performed in rtp_recieve_datagram_buffered() in rtp.c.

Given that the old check allowed jumps from 0 to 65535, MAX_PACKETS_IN_FLIGHT could be set upto to MAX_RTP_SEQNUM, but this would allow accepting packets potentially from the past if there is a very high rate of packets per second, which it didn't with the original check. Thus a threshold less than half the range seems reasonable (and it would be very similar to the checks in rtp.c).

We would like some feedback on this issue and the proposed solution.

Thank you,
Kind regards.

[stoecker]

Can you test following patch:

src/main.c

@@ -997 +997 @@
 static int store_udp_data(connection_t *con, unsigned char *buffer, int len, unsigned int seq)
 {
 //printf("Store %d seq %d last %d ssrc %lu\n", len, seq, con->udpbuffers->seq, con->rtp->datagram->ssrc);
-  if((int)(seq-con->udpbuffers->seq) > 0)
+  /* handle 16 bit overflows, needs an upper limit in the following if clause */
+  unsigned int diff = seq < con->udpbuffers->seq ? 65536+seq-con->udpbuffers->seq : seq-con->udpbuffers->seq;
+  /* when 1000 blocks got missing essentially consider the stream broken or
+   * the overflow detection handling was wrong */
+  if(diff > 0 && diff < 1000)
   {
     con->udpbuffers->seq = seq;
     if(len)

It's essentially what you propose only with a little less magic code :-)

[stoecker]

In 9813/ntrip:

fix UDP isses, fix #149

[emiravalls]

Agreed. We are testing your solution and we will confirm if it works for us, thank you!

One comment though, since the issue is the same, perhaps I would suggest to have the exact same computation implementation of "diff" in rtp.c:220 (either both using the modulo or the ternary operator) to avoid having two different implementations in the code. Sorry I didn't though of that from the beginning...

[stoecker]

In principle I'd agree, but the code in rtp.c seems to cache out-of-order datagrams and reorder them which the plain UDP code doesn't try at all. So when I do changes there this would need intensive testing which I think isn't worth only for theoretical considerations. Especially as nearly nobody uses RTSP or UDP modes of Ntrip ;-)