This commit should fix a bug present if for instance batch_factor < 8
and osmux_batch_enqueue is called from osmux_replay_lost_packets and
enough packets were lost from last received packet.
Change-Id: I5d643810949aeca4762f0cad05eed534d35087f7
Use this function in osmux_batch_del_circuit() since msgs are stored in a list
per circuit. After the circuit is free()d the msgs are lost.
Before this patch any messages enqueued inside a batch when the circiut is
deleted were leaking.
Change-Id: Ib0311652183332d0475bf7347023d518d38487ef
Ticket: OS#1733
Reviewed-on: https://gerrit.osmocom.org/120
Tested-by: Jenkins Builder
Reviewed-by: Holger Freyther <holger@freyther.de>
This new function allows you to create a circuit on an existing input handle.
We don't create the circuit anymore from the first packet seen, instead the
client application is in full control of opening and closing the circuit.
This change includes a new feature to pad a circuit until we see the first
packet that contains voice data. This is useful to preallocate bandwidth on
satellite links such as Iridium/OpenPort.
Add this new function to explicitly remove an existing circuit. Thus, the
client application (openbsc) is in full control to release circuits.
Before this patch, the circuit object was added when the first RTP messages was
seen, and it was removed when transforming the list of pending RTP messages to
the Osmux message (once the timer expired).
Moreover, check circuit->nmsgs to account bytes that are consumed by the osmux
header, given that !circuit doesn't mean "this is the first packet" anymore.
Add a new field to count the number of messages in the batch that are pending
to be transformed to osmux. Use this new field to check when to enable the
osmux timer.
The follow up patch keeps circuit objects in the batch until they are closed,
so we won't be able to rely on this to know when to enable the timer anymore.
I think this is a better name for this object. Basically, an input handle
represents a batch that is composed of one or more circuit objects.
Each circuit object contains a list of RTP messages that are pending to be
converted to the osmux format in one single batch.
This new structure serves as container of the internal osmux state during
transformation from RTP AMR to the compressed osmux format.
This reduces the footprint of several functions and it makes them easier to
extend if we need to pass new information between functions.
Make sure we don't release a osmux handle with an armed batch timer.
==9753== Invalid read of size 4
==9753== at 0x4043CA2: rb_first (rbtree.c:293)
==9753== by 0x403E172: osmo_timers_check (timer.c:256)
==9753== by 0x403E69E: osmo_select_main (select.c:124)
==9753== by 0x804EBD3: main (bsc_nat.c:1613)
==9753== Address 0x4302670 is 56 bytes inside a block of size 108 free'd
==9753== at 0x4023B6A: free (vg_replace_malloc.c:366)
==9753== by 0x40494CF: talloc_free (talloc.c:609)
==9753== by 0x40AB279: osmux_xfrm_input_fini (osmux.c:620)
==9753== by 0x80651FC: osmux_disable_endpoint (mgcp_osmux.c:96)
==9753== by 0x805CAFF: mgcp_release_endp (mgcp_protocol.c:1540)
==9753== by 0x805DD35: handle_delete_con (mgcp_protocol.c:1274)
==9753== by 0x805E998: mgcp_handle_message (mgcp_protocol.c:415)
==9753== by 0x804CFF1: mgcp_do_read (bsc_mgcp_utils.c:972)
==9753== by 0x403F96D: osmo_wqueue_bfd_cb (write_queue.c:48)
==9753== by 0x403E724: osmo_select_main (select.c:158)
==9753== by 0x804EBD3: main (bsc_nat.c:1613)
If it is not possible to put the RTP message into the batch in case that:
1) The message is malformed.
2) The message is duplicated.
3) OOM.
4) The batch is already full.
Osmux releases the messages and gets back to the upper layer with an OK
to give another chance to follow up RTP messages.
This patch also fixes a use-after-free that was possible when the batch
was full. The message was released and osmux_batch_add() was returning 0
osmux_xfrm_input(), which resulted in accessing the released message
when updating the statistics.
This is ugly, with many ifdefs, but they don't want any debug message
that can be spamming. I don't want to remove these because there is
no dissector for osmux, and this can be useful for troubleshooting.
Should be enabled only in case you consider that Osmux should
is lagging when reconstructing RTP messages. I haven't seen
any issue regarding this so far. Let's disable it by default.
This patch adds a new field to the struct osmux_in_handle that allows
you to specify the osmux frame size. If not specified, the default
size assumes your nic uses a mtu of 1500 bytes.
Remove message that is printed twice and reword the messages to
make it more clear the traffic flow when compressing and
decompressing the RTP AMR traffic.
(dc898ab osmux: don't trust AMR FT field) was not correctly
validating the AMR FT field as it was comparing the same
value twice calculated in different ways.
Use osmo_amr_bytes(amrh->ft) to obtain the expected length
and check if it is what we got. Use the output of
osmo_rtp_get_payload() as it also includes the RTP payload
stripping possible extensions.
The ctr field of the osmux header is 3 bits long, make sure we
don't run over that boundary. This should not happen in practise
unless we have to deal with network congestion or broken RTP
stacks, but osmux should not crash in that case.
With this patch, osmux_xfrm_input() returns 0 (means "message has been
processed") instead of 1 (means "retry") if the RTP message is too big
to fit into one osmux batch. This fixes a likely infinite loop in the
caller, which will retry forever for a message does not fit into the
batch.
Unlikely to happen in normal scenario, as RTP+AMR messages are way
smaller than the interface MTU.
The AMR FT field is used to infer the length of the payload, if
a value higher than 8 (SID) is received, skip it.
This fixes a possible crash in osmux_snprintf() in case we receive
a malformed osmux header.
This is also addresses the crash described in c733ae5b6e.
valgrind reports the following crash backtrace:
!<001c> osmux.c:687 No room for OSMUX payload: only 49 bytes
==12800==
==12800== Process terminating with default action of signal 11 (SIGSEGV)
==12800== Access not within mapped region at address 0xDFA8E473
==12800== at 0x4073FD2: osmux_snprintf (osmux.c:628)
==12800== by 0x80524F1: osmux_deliver (osmux.c:50)
==12800== by 0x407371C: osmux_xfrm_input_deliver (osmux.c:302)
==12800== by 0x4073792: osmux_batch_timer_expired (osmux.c:312)
==12800== by 0x405A4A0: osmo_timers_update (timer.c:243)
==12800== by 0x405A79A: osmo_select_main (select.c:133)
==12800== by 0x8049A53: main (mgcp_main.c:307)
The problem is that osmux_snprintf() is not handling multi-batch
messages (ie. messages that contain several osmux batches). More
specifically, the offset to print the osmux batches was reset
when parsing every osmux batch.
The problem also manifested with wrong outputs.
Reported by Mattias Lundstrom.
Valgrind complains about a possible use after free:
==12800== Invalid read of size 4
==12800== at 0x4073DF6: osmux_tx_sched (linuxlist.h:119)
==12800== by 0x8052B0F: osmux_read_from_bsc_nat_cb (osmux.c:261)
==12800== by 0x453F967: ???
==12800== Address 0x453f710 is 48 bytes inside a block of size 145
+free'd
==12800== at 0x402750C: free (vg_replace_malloc.c:427)
==12800== by 0x4064ADE: talloc_free (talloc.c:609)
==12800== by 0x405AAAA: msgb_free (msgb.c:72)
==12800== by 0x8052492: scheduled_tx_bts_cb (osmux.c:196)
==12800== by 0x4072CF8: osmux_tx_cb (osmux.c:554)
==12800== by 0x4073F03: osmux_tx_sched (osmux.c:582)
==12800== by 0x8052B0F: osmux_read_from_bsc_nat_cb (osmux.c:261)
==12800== by 0x453F967: ???
The problem is that osmux_tx_sched may immediately call osmux_tx_cb for
the first extracted RTP message from the osmux batch, which releases the
message after that.
Remove the message from our list of messages to be transmitted before
the message is passed to the tx callback.
Reported by Mattias Lundstrom.
According to RFC3267, AMR FT upper 9 should be discarded. This patch
adds extra validation to make sure that input RTP traffic encapsulating
AMR payload and OSMUX amr_ft field are OK with regards to that
restriction.
osmux.c:622:20: warning: Value stored to 'osmuxh' during its
initialization is never read
struct osmux_hdr *osmuxh = (struct osmux_hdr *)msg->data;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Reported by Holger Hans Peter Freyther.
If osmux notices a gap between two RTP packets, fill it with
the last RTP packet seen. Without this patch, 10% of packet loss
is enough to get garbage, with it we get glitches in the conversation
with 30%, and pretty much broken conversation with 40% of it.
Pau Espin
Harald Welte
Max
Daniel Willmann