This makes GLib usage annoying as it makes GSourceFunc casts invalid.

Jakub Adam authored 7 years ago and Olivier Crête committed 6 years ago

Setting writable socket callbacks doesn't have to be limited to reliable
agents. TCP sockets need the callback in any case for correct operation
and calling nice_socket_set_writable_callback() on a NiceSocket that has
UDP as its base has no effect.

Differential Revision: https://phabricator.freedesktop.org/D1726

Jakub Adam authored 7 years ago and Olivier Crête committed 6 years ago

If main SfB TURN server sends our allocation request to an alternate
server, the response will have XOR_MAPPED_ADDRESS containing the IP
address of the turn server that proxied the message instead of our own
actual external IP.

Before we create server reflexive candidates upon receiving an allocate
response, check that the TURN port got assigned on the same server we
sent out allocate request to. Otherwise, the request was proxied and
XOR_MAPPED_ADDRESS contains a bogus value we should ignore.

Issue introduced by 59fcf95d.
Differential Revision: https://phabricator.freedesktop.org/D1949

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

With this patch we prevent the username and the password of a candidate
to be modified during a session, as required by the RFC, sect 9.1.2.
This is also needed from a memory management point of view, because the
password string pointer may be recorded in the components stun agent
sent_ids[] struct key member, and freeing these values there may cause
an use-after-free condition, when an inbound stun is received from this
candidate. This behavior has been observed with pidgin, xmpp, and
farstream when a same remote candidates are "updated" several times,
even if the credentials don't change in this case.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1917

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

The tcp server reflexive discovered local candidates must be ignored
when force_relay is set.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1899

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

Since commit 17f30e44, we may have a stream with an empty conncheck list,
and such a stream obviously should not be tested for failed components.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1895

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

Verify the compatibility of the socket domain with the stun server
IP address, before sending a request.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1894

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1893

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1892

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

With this patch, we put the pair in state failed if we cannot send
the connection check, for example due to missing local credentials.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1891

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

the first case of test-new-dribble (standard-test) is updated, by making
the credentials swap between the left and right agent asymmetric.
Previously, ragent started to receive stun requests without initially
knowing lagent candidates. Now, ragent also ignores lagent credentials.
This modification allows to test changes introduced by the previous
commit.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1890

Fabrice Bellet authored 7 years ago and Olivier Crête committed 6 years ago

With this patch we simplify the code used to handle the incoming stun
request when remote candidates or remote credentials have not been
received yet.

When the remote credentials is unknown, the stun request is stored
in a list of incoming_checks for later processing, and no further
processing is done, except responding to the request.

When the remote credentials are received, the triggered checks for these
incoming checks can now be queued, and the related pairs are created.

If the remote candidates have not been received when the stun request
on a valid local port arrives, a peer-reflexive remote candidate will be
created. This candidate may need to be updated later when remote
candidates are finally received, including candidate priority and
foundation, and also related pairs.

Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1889

Jozsef Vass authored 7 years ago and Olivier Crête committed 6 years ago

The variable tie is actually never read.

If a relay gives us an alternate-server, we need to cancel and reset
every candidate discovery attempt that uses the same server, to avoid
ending up with one component on one server and the other component on
another server (causing relay candidates with mismatched foundations).

The discovery_unsched_items is decremented every time a DiscoveryCandidate
goes from non-pending to pending. So if we restart a check by setting
pending to FALSE, we should re-increase the discovery_unsched_items.

The MS Office TURN servers will always return the MS_ALTERNATE_SERVER in
allocation responses, and if they are not handled, we end up using the
main turn server to send allocation requests that then get sent to the
alternate server which will return the XOR_MAPPED_ADDRESS containing
the IP address of the turn server that proxied the message instead of
our own actual external IP.

Miguel París Díaz authored 7 years ago and Olivier Crête committed 7 years ago

One or more streams might not have any connection check list if the
number of streams differs from the peer agent.
Differential Revision: https://phabricator.freedesktop.org/D1880

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

Differential Revision: https://phabricator.freedesktop.org/D1888

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

With this patch, we stash the controlling mode property change, and
apply it safely, when it won't interfere with an ongoing conncheck
running. According to RFC5245, sect 5.2. "Determining Role", the role
is determined for a session, and persists unless an ICE is restarted.

Differential Revision: https://phabricator.freedesktop.org/D1887

https://phabricator.freedesktop.org/T7798



Signed-off-by: Philip Withnall <withnall@endlessm.com>
Reviewed-by: Olivier Crête <olivier.crete@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D1819

Spotted by Lukas Gradl on the mailing list.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

This code is not 1000 years old.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

When a pair is nominated while in state failed, we first move
back to state connecting, then we update the selected pair, and
finally we move to state connected.

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

When a new pair is created from an unknown remote candidate, it
should be enqueue for a triggered check, to allow it to be marked
as nominated on response arrival in priv_mark_pair_nominated().
Creating it in waiting state is not sufficient since the update
in priv_mark_pair_nominated() from previous commits.

Differential Revision: https://phabricator.freedesktop.org/D1763

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

With this patch, we fix an ambiguity of some parts of the spec, when
the document refers to in-progress pairs, that also concern pairs in
the triggered checks list.

The first cast is in section 7.1.2.5, "Updating the Nominated Flag",
when the in-progress pair will be nominated on response arrival. This is
handled in function priv_mark_pair_nominated(), when a pair is put to
the triggered check list in reaction to a matching inbound stun request.
Such a pair in priv_mark_pair_nominated() will _always_ be in the
triggered check list, from the previously called function
priv_schedule_triggered_check().

The second case is in section 8.1.2, "Updating State" when an in-progress
pair stops its retransmission when another pair of higher priority is
already nominated. This is handled by function priv_prune_pending_checks().

Until now, pairs enqueued in the triggered check list move transiently
to state waiting, according to 7.2.1.4. But this state causes wrong
decisions in the two previous cases, because such pairs should in fact
rather be considered "like in-progress", to avoid discarding them
inadvertantly.

This patch update the state of the triggered check list
pairs to in-progress. It allows to remove exception handling cited
above: the code is a bit more simple, and allows some refactoring
in priv_mark_pair_nominated() between RFC and compatibility modes.

Differential Revision: https://phabricator.freedesktop.org/D1762

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

This patch should improve the reliabily of the connection check by
keeping the record of several simultaneous ongoing stun requests per
pair. A new stun request on an in-progress pair typically is caused by
in inbound stun request from the peer on this same pair. This is named
"Triggered Checks" in the spec. When this situation arises, it is fair
to handle these two stun requests simultaneously, the triggered check,
and the initial ordinary check, since both can potentially succeed.

Differential Revision: https://phabricator.freedesktop.org/D1761

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

We try to use stun_timer_remainder() less frequently, particularily
in the debug messages, and favour of the next_tick value associated
to the pair.

Differential Revision: https://phabricator.freedesktop.org/D1760

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

We add a helper function to print the pair state in-extenso.

Differential Revision: https://phabricator.freedesktop.org/D1759

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

With this patch we simplify the levels of code indentation.

Differential Revision: https://phabricator.freedesktop.org/D1758

Fabrice Bellet authored 8 years ago and Olivier Crête committed 7 years ago

This patch displays explicitely the controlling or controlled
role of the agent.

Differential Revision: https://phabricator.freedesktop.org/D874

Creates useless warnings when other libraries change.

https://phabricator.freedesktop.org/T7770

GClosures are not that cheap to setup

This makes it easier to read and more extensible.

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

Differential Revision: https://phabricator.freedesktop.org/D1756

Fabrice Bellet authored 7 years ago and Olivier Crête committed 7 years ago

Differential Revision: https://phabricator.freedesktop.org/D1754