17 Nov
2022
17 Nov
'22
11:16 a.m.
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30168 )
Change subject: osmux: dup in RTP pkt: Replace potentially internally forged pkt with
incoming one
......................................................................
osmux: dup in RTP pkt: Replace potentially internally forged pkt with incoming one
When RTP packet provided by user to osmux layer, it may contain a seq
gap, and osmux will refill the packets to avoid losing that information
on the other side when it receives the batch.
If out of order UDP packets are received, it can happen that we first
detect a gap and later on we receive the previous RTP packet, which we
is then detected as duplicate because it was previously forged to fill
the hole. In that case, let's better keep the incoming packet instead of
the potentially internall forged one which doesn't contain the real AMR
payload.
Related: SYS#6161
Change-Id: I82e11ef3dcd20ffea33c94ed83abcedf0f186871
---
M src/osmux_input.c
M tests/osmux/osmux_input_test.c
M tests/osmux/osmux_input_test.ok
3 files changed, 106 insertions(+), 6 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 10598d2..bdc7fd3 100644
--- a/src/osmux_input.c
+++ b/src/osmux_input.c
@@ -442,7 +442,7 @@
static int
osmux_link_add(struct osmux_link *link, const struct osmux_in_req *req)
{
- struct msgb *cur;
+ struct msgb *cur, *next;
int rc;
unsigned int needed_bytes = 0;
@@ -455,15 +455,27 @@
/* Extra validation: check if this message already exists, should not
* happen but make sure we don't propagate duplicated messages.
*/
- llist_for_each_entry(cur, &req->circuit->msg_list, list) {
+ 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, skip */
+ /* 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) {
- LOGP(DLMUX, LOGL_ERROR, "RTP pkt with seq=%u already exists, skip it\n",
- ntohs(req->rtph->sequence));
- return -1;
+ 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;
}
}
diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c
index 8645929..7a547d9 100644
--- a/tests/osmux/osmux_input_test.c
+++ b/tests/osmux/osmux_input_test.c
@@ -413,6 +413,84 @@
talloc_free(h_input);
}
+static void test_rtp_dup_osmux_deliver_cb(struct msgb *batch_msg, void *data)
+{
+ struct osmux_hdr *osmuxh;
+ char buf[2048];
+ uint8_t *osmux_pl;
+ bool *osmux_transmitted = (bool *)data;
+
+ osmux_snprintf(buf, sizeof(buf), batch_msg);
+ clock_debug("OSMUX message (len=%d): %s\n", batch_msg->len, buf);
+
+ /* We expect 1 batch: */
+ osmuxh = osmux_xfrm_output_pull(batch_msg);
+ /* Check seqnum is the one configured beforehand: */
+ OSMO_ASSERT(osmuxh->seq == 123);
+ osmux_pl = (uint8_t *)osmuxh + sizeof(*osmuxh);
+ OSMO_ASSERT(osmux_pl[0] == 0x12);
+
+ osmuxh = osmux_xfrm_output_pull(batch_msg);
+ OSMO_ASSERT(osmuxh == NULL);
+
+ msgb_free(batch_msg);
+
+ *osmux_transmitted = true;
+}
+/* Test user pushes duplicated RTP (dup seqnum) to osmux: */
+static void test_rtp_dup(void)
+{
+ struct msgb *msg, *msg_dup;
+ struct amr_hdr *amrh;
+ int rc;
+ const uint8_t cid = 33;
+ bool osmux_transmitted = false;
+ struct osmux_in_handle *h_input;
+
+ printf("===%s===\n", __func__);
+
+
+
+ clock_override_enable(true);
+ clock_override_set(0, 0);
+ rtp_init(0, 0);
+
+ h_input = osmux_xfrm_input_alloc(tall_ctx);
+ osmux_xfrm_input_set_initial_seqnum(h_input, 123);
+ osmux_xfrm_input_set_batch_factor(h_input, 2);
+ osmux_xfrm_input_set_deliver_cb(h_input,
+ test_rtp_dup_osmux_deliver_cb,
+ &osmux_transmitted);
+ osmux_xfrm_input_open_circuit(h_input, cid, false);
+
+ /* First RTP frame at t=0 */
+ msg = rtp_next();
+ msg_dup = msgb_copy(msg, "dup");
+ rtp_append_amr(msg, AMR_FT_2);
+ rc = osmux_xfrm_input(h_input, msg, cid);
+ OSMO_ASSERT(rc == 0);
+
+ clock_debug("Submit 2nd RTP packet, seqnum dup");
+ clock_override_add(0, TIME_RTP_PKT_MS*1000);
+ amrh = rtp_append_amr(msg_dup, AMR_FT_2);
+ amrh->data[0] = 0x12; /* Change AMR payload to check it is updated. */
+ rc = osmux_xfrm_input(h_input, msg_dup, cid);
+ OSMO_ASSERT(rc == 0);
+ OSMO_ASSERT(osmux_transmitted == false);
+
+ /* t=60, osmux batch is scheduled to be transmitted: */
+ clock_debug("Submit 3rd RTP packet, triggers osmux batch");
+ clock_override_add(0, TIME_RTP_PKT_MS*1000);
+ msg = rtp_next();
+ rtp_append_amr(msg, AMR_FT_2);
+ osmo_select_main(0);
+ OSMO_ASSERT(osmux_transmitted == true);
+
+ clock_debug("Closing circuit");
+ osmux_xfrm_input_close_circuit(h_input, cid);
+ talloc_free(h_input);
+}
+
int main(int argc, char **argv)
{
@@ -433,6 +511,7 @@
test_amr_ft_change_middle_batch();
test_last_amr_cmr_f_q_used();
test_initial_osmux_seqnum();
+ test_rtp_dup();
fprintf(stdout, "OK: Test passed\n");
return EXIT_SUCCESS;
diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok
index 60daf26..b5b2f00 100644
--- a/tests/osmux/osmux_input_test.ok
+++ b/tests/osmux/osmux_input_test.ok
@@ -28,4 +28,13 @@
sys={0.020000}, mono={0.020000}: OSMUX message (len=19): OSMUX seq=123 ccid=033 ft=1
rtp_m=0 ctr=0 amr_f=1 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 ]
sys={0.020000}, mono={0.020000}: Closing circuit
+===test_rtp_dup===
+sys={0.000000}, mono={0.000000}: clock_override_set
+sys={0.000000}, mono={0.000000}: Submit 2nd RTP packet, seqnum dup
+sys={0.020000}, mono={0.020000}: clock_override_add
+sys={0.020000}, mono={0.020000}: Submit 3rd RTP packet, triggers osmux batch
+sys={0.040000}, mono={0.040000}: clock_override_add
+sys={0.040000}, mono={0.040000}: OSMUX message (len=19): OSMUX seq=123 ccid=033 ft=1
rtp_m=0 ctr=0 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 12 00 00 00 00 00 00 00 00 00 00 00
00 00 00 ]
+
+sys={0.040000}, mono={0.040000}: Closing circuit
OK: Test passed
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30168
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I82e11ef3dcd20ffea33c94ed83abcedf0f186871
Gerrit-Change-Number: 30168
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged