laforge has submitted this change. (
https://gerrit.osmocom.org/c/libosmo-netif/+/30174 )
Change subject: osmux: Obey current batch_size restrictions when creating
forged RTP packets to fill holes
......................................................................
osmux: Obey current batch_size restrictions when creating forged RTP packets to
fill holes
osmux_link_add() is renamed to osmux_link_handle_rtp_req(), and the last
part of it is split out and kept as osmux_link_add().
hence osmux_link_handle_rtp_req() does proper input checking (like
duplicates, holes, etc.) while osmux_link_add() expects all that to be
sorted out.
Reuse osmux_link_add() in osmux_replay_lost_packets() to properly update
the link state of the to-be-transmitted packet, circuit state, etc.
Change-Id: I4ea435bfb2490a375ad3e5068ee926e48b53cf5c
---
M src/osmux_input.c
M tests/osmux/osmux_input_test.c
M tests/osmux/osmux_input_test.ok
3 files changed, 125 insertions(+), 110 deletions(-)
Approvals:
Jenkins Builder: Verified
osmith: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/osmux_input.c b/src/osmux_input.c
index a50a179..0126da7 100644
--- a/src/osmux_input.c
+++ b/src/osmux_input.c
@@ -353,71 +353,6 @@
}
-/* returns: 1 if batch is full, 0 if batch still not full, negative on error.
*/
-static int osmux_replay_lost_packets(struct osmux_link *link, const struct
osmux_in_req *req)
-{
- int16_t diff;
- struct msgb *last;
- struct rtp_hdr *last_rtph, *rtph;
- uint16_t last_seq, cur_seq;
- int i, rc;
-
- /* Have we seen any RTP packet in this batch before? */
- if (llist_empty(&req->circuit->msg_list))
- return 0;
-
- /* Get last RTP packet seen in this batch */
- last = llist_entry(req->circuit->msg_list.prev, struct msgb, list);
- last_rtph = osmo_rtp_get_hdr(last);
- if (last_rtph == NULL)
- return -1;
- last_seq = ntohs(last_rtph->sequence);
- cur_seq = ntohs(req->rtph->sequence);
- diff = cur_seq - last_seq;
-
- /* If diff between last RTP packet seen and this one is > 1,
- * then we lost several RTP packets, let's replay them.
- */
- if (diff <= 1)
- return 0;
-
- LOGP(DLMUX, LOGL_INFO, "RTP seq jump detected, recreating %" PRId16
- " lost packets (seq jump: %" PRIu16 " -> %" PRIu16 ")\n",
- diff - 1, last_seq, cur_seq);
-
- /* Lifesaver: make sure bugs don't spawn lots of clones */
- if (diff > 16)
- diff = 16;
-
- rc = 0;
- rtph = last_rtph;
- for (i = 1; i < diff; i++) {
- struct msgb *clone;
-
- /* Clone last RTP packet seen */
- clone = msgb_copy(last, "RTP clone");
- if (!clone)
- continue;
-
- /* The original RTP message has been already sanity checked. */
- rtph = osmo_rtp_get_hdr(clone);
- /* Faking a follow up RTP pkt here, so no Marker bit: */
- rtph->marker = false;
- /* Adjust sequence number and timestamp */
- rtph->sequence = htons(ntohs(rtph->sequence) + i);
- rtph->timestamp = htonl(ntohl(rtph->timestamp) +
- DELTA_RTP_TIMESTAMP);
-
- /* No more room in this batch, skip padding with more clones */
- rc = osmux_circuit_enqueue(link, req->circuit, clone);
- if (rc != 0) {
- msgb_free(clone);
- return rc;
- }
- }
- return rc;
-}
-
static struct osmux_circuit *
osmux_link_find_circuit(struct osmux_link *link, int ccid)
{
@@ -440,51 +375,10 @@
}
/* returns: 1 if batch is full, 0 if batch still not full, negative on error.
*/
-static int
-osmux_link_add(struct osmux_link *link, const struct osmux_in_req *req)
+static int osmux_link_add(struct osmux_link *link, const struct osmux_in_req
*req)
{
- struct msgb *cur, *next;
- int rc;
unsigned int needed_bytes = 0;
-
- /* We've seen the first RTP message, disable dummy padding */
- if (req->circuit->dummy) {
- req->circuit->dummy = 0;
- link->ndummy--;
- }
-
- /* Extra validation: check if this message already exists, should not
- * happen but make sure we don't propagate duplicated messages.
- */
- llist_for_each_entry_safe(cur, next, &req->circuit->msg_list, list) {
- struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur);
- OSMO_ASSERT(rtph2);
-
- /* Already exists message with this sequence. Let's copy over
- * the new RTP, since there's the chance that the existing one
may
- * be a forged copy we did when we detected a hole. */
- if (rtph2->sequence == req->rtph->sequence) {
- if (msgb_length(cur) != msgb_length(req->msg)) {
- /* Different packet size, AMR FT may have
changed. Let's avoid changing it to
- * break accounted size to be written (would
need new osmux_hdr, etc.) */
- LOGP(DLMUX, LOGL_NOTICE, "RTP pkt with seq=%u
and different len %u != %u already exists, skip it\n",
- ntohs(req->rtph->sequence),
msgb_length(cur), msgb_length(req->msg));
- return -1;
- }
- LOGP(DLMUX, LOGL_INFO, "RTP pkt with seq=%u already
exists, replace it\n",
- ntohs(req->rtph->sequence));
- __llist_add(&req->msg->list, &cur->list,
cur->list.next);
- llist_del(&cur->list);
- msgb_free(cur);
- return 0;
- }
- }
-
- /* Handle RTP packet loss scenario */
- rc = osmux_replay_lost_packets(link, req);
- if (rc != 0)
- return rc;
-
+ int rc;
/* Init of talkspurt (RTP M marker bit) needs to be in the first AMR
slot
* of the OSMUX packet, enforce sending previous batch if required:
*/
@@ -528,6 +422,119 @@
link->nmsgs++;
return 0;
+};
+
+/* returns: 1 if batch is full, 0 if batch still not full, negative on error.
*/
+static int osmux_replay_lost_packets(struct osmux_link *link, const struct
osmux_in_req *req)
+{
+ int16_t diff;
+ struct msgb *last;
+ struct rtp_hdr *last_rtph;
+ uint16_t last_seq, cur_seq;
+ int i, rc;
+ struct osmux_in_req clone_req;
+
+ /* Have we seen any RTP packet in this batch before? */
+ if (llist_empty(&req->circuit->msg_list))
+ return 0;
+
+ /* Get last RTP packet seen in this batch */
+ last = llist_entry(req->circuit->msg_list.prev, struct msgb, list);
+ last_rtph = osmo_rtp_get_hdr(last);
+ if (last_rtph == NULL)
+ return -1;
+ last_seq = ntohs(last_rtph->sequence);
+ cur_seq = ntohs(req->rtph->sequence);
+ diff = cur_seq - last_seq;
+
+ /* If diff between last RTP packet seen and this one is > 1,
+ * then we lost several RTP packets, let's replay them.
+ */
+ if (diff <= 1)
+ return 0;
+
+ LOGP(DLMUX, LOGL_INFO, "RTP seq jump detected, recreating %" PRId16
+ " lost packets (seq jump: %" PRIu16 " -> %" PRIu16 ")\n",
+ diff - 1, last_seq, cur_seq);
+
+ /* Lifesaver: make sure bugs don't spawn lots of clones */
+ if (diff > 16)
+ diff = 16;
+
+ rc = 0;
+ clone_req = *req;
+ for (i = 1; i < diff; i++) {
+ /* Clone last RTP packet seen */
+ clone_req.msg = msgb_copy(last, "RTP clone");
+ if (!clone_req.msg)
+ continue;
+
+ /* The original RTP message has been already sanity checked. */
+ clone_req.rtph = osmo_rtp_get_hdr(clone_req.msg);
+ clone_req.amrh = osmo_rtp_get_payload(clone_req.rtph,
clone_req.msg, &clone_req.rtp_payload_len);
+ clone_req.amr_payload_len =
osmux_rtp_amr_payload_len(clone_req.amrh, clone_req.rtp_payload_len);
+
+ /* Faking a follow up RTP pkt here, so no Marker bit: */
+ clone_req.rtph->marker = false;
+ /* Adjust sequence number and timestamp */
+ clone_req.rtph->sequence =
htons(ntohs(clone_req.rtph->sequence) + i);
+ clone_req.rtph->timestamp =
htonl(ntohl(clone_req.rtph->timestamp) +
+ DELTA_RTP_TIMESTAMP);
+ rc = osmux_link_add(link, &clone_req);
+ /* No more room in this batch, skip padding with more clones */
+ if (rc != 0) {
+ msgb_free(clone_req.msg);
+ return rc;
+ }
+ }
+ return rc;
+}
+
+/* returns: 1 if batch is full, 0 if batch still not full, negative on error.
*/
+static int osmux_link_handle_rtp_req(struct osmux_link *link, struct
osmux_in_req *req)
+{
+ struct msgb *cur, *next;
+ int rc;
+
+ /* We've seen the first RTP message, disable dummy padding */
+ if (req->circuit->dummy) {
+ req->circuit->dummy = 0;
+ link->ndummy--;
+ }
+
+ /* Extra validation: check if this message already exists, should not
+ * happen but make sure we don't propagate duplicated messages.
+ */
+ llist_for_each_entry_safe(cur, next, &req->circuit->msg_list, list) {
+ struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur);
+ OSMO_ASSERT(rtph2);
+
+ /* Already exists message with this sequence. Let's copy over
+ * the new RTP, since there's the chance that the existing one
may
+ * be a forged copy we did when we detected a hole. */
+ if (rtph2->sequence == req->rtph->sequence) {
+ if (msgb_length(cur) != msgb_length(req->msg)) {
+ /* Different packet size, AMR FT may have
changed. Let's avoid changing it to
+ * break accounted size to be written (would
need new osmux_hdr, etc.) */
+ LOGP(DLMUX, LOGL_NOTICE, "RTP pkt with seq=%u
and different len %u != %u already exists, skip it\n",
+ ntohs(req->rtph->sequence),
msgb_length(cur), msgb_length(req->msg));
+ return -1;
+ }
+ LOGP(DLMUX, LOGL_INFO, "RTP pkt with seq=%u already
exists, replace it\n",
+ ntohs(req->rtph->sequence));
+ __llist_add(&req->msg->list, &cur->list,
cur->list.next);
+ llist_del(&cur->list);
+ msgb_free(cur);
+ return 0;
+ }
+ }
+
+ /* Handle RTP packet loss scenario */
+ rc = osmux_replay_lost_packets(link, req);
+ if (rc != 0)
+ return rc;
+
+ return osmux_link_add(link, req);
}
/**
@@ -592,7 +599,7 @@
}
/* Add this RTP to the OSMUX batch */
- ret = osmux_link_add(link, &req);
+ ret = osmux_link_handle_rtp_req(link, &req);
if (ret < 0) {
/* Cannot put this message into the batch.
* Malformed, duplicated, OOM. Drop it and tell
diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c
index a728f6b..eb2fb79 100644
--- a/tests/osmux/osmux_input_test.c
+++ b/tests/osmux/osmux_input_test.c
@@ -683,7 +683,7 @@
clock_debug("osmux batch transmitted");
clock_override_add(0, TIME_RTP_PKT_MS*1000);
osmo_select_main(0);
- OSMO_ASSERT(osmux_transmitted == 1); /* FIXME: this is a bug, should be
2! */
+ OSMO_ASSERT(osmux_transmitted == 2);
clock_debug("Closing circuit");
osmux_xfrm_input_close_circuit(h_input, cid);
diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok
index 006d3f0..f8a5e6f 100644
--- a/tests/osmux/osmux_input_test.ok
+++ b/tests/osmux/osmux_input_test.ok
@@ -119,6 +119,8 @@
sys={0.280000}, mono={0.280000}: clock_override_add
sys={0.280000}, mono={0.280000}: osmux batch transmitted
sys={0.300000}, mono={0.300000}: clock_override_add
+sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124
ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 ]
+
sys={0.300000}, mono={0.300000}: Closing circuit
===test_rtp_pkt_gap_bigger_than_batch_factor(65533)===
sys={0.000000}, mono={0.000000}: clock_override_set
@@ -146,6 +148,8 @@
sys={0.280000}, mono={0.280000}: clock_override_add
sys={0.280000}, mono={0.280000}: osmux batch transmitted
sys={0.300000}, mono={0.300000}: clock_override_add
+sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124
ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 ]
+
sys={0.300000}, mono={0.300000}: Closing circuit
===test_rtp_pkt_gap_bigger_than_batch_factor(65534)===
sys={0.000000}, mono={0.000000}: clock_override_set
@@ -173,6 +177,8 @@
sys={0.280000}, mono={0.280000}: clock_override_add
sys={0.280000}, mono={0.280000}: osmux batch transmitted
sys={0.300000}, mono={0.300000}: clock_override_add
+sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124
ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 ]
+
sys={0.300000}, mono={0.300000}: Closing circuit
===test_rtp_pkt_gap_bigger_than_batch_factor(65535)===
sys={0.000000}, mono={0.000000}: clock_override_set
@@ -200,5 +206,7 @@
sys={0.280000}, mono={0.280000}: clock_override_add
sys={0.280000}, mono={0.280000}: osmux batch transmitted
sys={0.300000}, mono={0.300000}: clock_override_add
+sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124
ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 ]
+
sys={0.300000}, mono={0.300000}: Closing circuit
OK: Test passed
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30174
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I4ea435bfb2490a375ad3e5068ee926e48b53cf5c
Gerrit-Change-Number: 30174
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: laforge <[email protected]>
Gerrit-Reviewer: msuraev <[email protected]>
Gerrit-Reviewer: osmith <[email protected]>
Gerrit-MessageType: merged
