GitLab

To try and avoid NULL NiceCandidates entering our internal state.

This makes helgrind slightly happier.

This could result in leaking the stream’s un-closed state otherwise, in
any case where a NiceAgent is finalised without
nice_agent_remove_stream() being called on all its streams.

To try and avoid NULL NiceCandidates entering our internal state.

Fix regressions introduced by the following patch:
Clear existing GSource timeouts before adding new ones

Rather than passing them through to the application. Invalid STUN
packets are still passed through, but it causes unnecessary noise and
corruption for higher-level applications to receive STUN packets they
weren’t expecting.

This is permitted by RFC 5389, §7.3.0:
    If any errors are detected, the message is silently discarded.
    In the case when STUN is being multiplexed with another protocol,
    an error may indicate that this is not really a STUN message; in
    this case, the agent should try to parse the message as a different
    protocol.
where I interpret ‘error’ to mean ‘validation error’ rather than (e.g.)
‘unrecognised attribute’ or ‘unmatched response’ where the STUN packet
is otherwise perfectly formed.

Modify agent_timeout_add_with_context() to force destroying and freeing
of an existing GSource before overwriting it with a new one (probably
with an updated timeout period).

This fixes a case in priv_map_reply_to_relay_refresh() where the TURN
candidate refresh timer was being overwritten with a new one, without
the old one being destroyed. This lead to two timeouts existing, only
one of which would be destroyed when the CandidateRefresh struct was
freed, leaking the other one (in the main context) and allowing it to be
later dispatched with a dangling CandidateRefresh pointer.

The modification to agent_timeout_add_with_context() should prevent this
happening in new code in future.

Modify the agent_timeout_add_with_context() utility function to
automatically add names to the timer GSources it creates. This makes
them a little easier to identify when debugging.

Otherwise we risk running out of space in the space limited saved ids
list. So the easiest way to do that is to put one StunAgent in each
component. It may be advisable to just give up on not allocating
memory inside libstun and just use a regular hash table in there
instead.

The agent can reach EOS after performing one or more non-empty reads in
its read loop, and then hitting EOS on the underlying stream. That means
reached_eos is TRUE, but n_valid_messages is non-zero.

Weaken the postcondition to reflect this.

As mentioned in the previous commit message, component_free() requires
the agent lock to be released, but component_close() requires it to be
held. Fix the locking in nice_agent_remove_stream() to do that.

Closing a component and freeing it are slightly conceptually different.
As it happens, freeing it can result in the disposal of the component's
NiceIOStream, which requires the agent lock to be released — but closing
the rest of the Component requires the agent lock to be held. Splitting
the function up simplifies locking handling in other parts of the code
(see next commit).

The mutex is now non-recursive. This is left over from commit 1deee693.

Bad merge in commit 67179ffd.

This got lost in rebasing the FIN–ACK work. component->tcp is now only
NULL before the pseudo-TCP stream is initially created — afterwards, it
is always non-NULL, but pseudo_tcp_stream_is_closed() returns TRUE if
the stream has closed (gracefully or on errors). This allows
differentiation between the states: TCP support was never initialised;
and TCP support was initialised but is now closed.

As with send(), pseudo_tcp_socket_send() may return EPIPE if the local
side of the connection has been closed using close() or shutdown(). It
currently doesn’t, but will do once pseudo-TCP FIN–ACK support has been
implemented.

Instead, keep the closed object around. This allows differentiation
between the states: TCP support was never initialised; and TCP support
was initialised but is now closed.

This simplifies the code a little. No functional changes.

Previously, an EWOULDBLOCK return value from the low-level socket calls
(including PseudoTcpSocket) would be represented by a zero number of
bytes (or messages) read by the agent. This conflicts with the use of
zero to represent end of stream (EOS) for pseudo-TCP connections, where
the sender has indicated that they are not going to send any more bytes.

So, now use GError (G_IO_ERROR_WOULD_BLOCK) to represent EWOULDBLOCK,
just like the GSocket functions. Zero is reserved exclusively for if:
 • the number of requested bytes/messages is zero; or
 • reliable mode is enabled and EOS is reached.

This does change the documented behaviour of the NiceAgent send/recv
API, but only by allowing a new behaviour (returning zero) rather than
by changing an existing one, so it should be OK.

This is to use keepalives to check if the connection is still ongoing. If
they don't get a reply, then we can assume that the connection has died.

This needs to not happen in the case of ICE-TCP.

This reverts commit 18e5dff4.

The pseudo_tcp_socket_get_next_clock returns the next clock in terms
of g_get_monotonic_clock / 1000, however, it gets cropped to guint32.
When we try to find the interval, by substracting the current time as
a guint64, it will return a negative value, which prevents an integer
overflow by setting the timeout to 0. This causes the notify_clock
timeout to be called all the time by the mainloop, taking 100% of CPU.

When calling component_emit_io_callback, the agent mutex will be unlocked before
the callback is called, this can cause another thread to remove the
stream and thus destroying the component. We must verify that the
source was not destroyed after calling component_emit_io_callback
and avoid accessing the component in that case.
Bug report by Stephan Thamm in the mailing list.

We want ICE-TCP to not have reliable behavior unless the agent
is reliable otherwise it will confuse existing VoIP applications.

PseudoTCP is only used if both the pseudotcp socket exists and the
underlying transport is unreliable.

This signal include the whole candidate as the foundation is ambiguous.

These signals include the actual candidate instead of just the foundation
which is not unique.

If the socket can't be bound, then trying another port makes sense,
otherwise it doesn't.

If we don't call check_connect_result on an outbound connection,
then is_connected will always return FALSE. That's why we check
both conditions to make sure g_socket_is_connected returns the
correct result, otherwise we end up closing valid connections