Commit 02216a67 authored by Fabrice Bellet's avatar Fabrice Bellet Committed by Olivier Crête

agent: prevent external role change while conncheck is running

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
parent fbdccf0c
......@@ -146,7 +146,7 @@ struct _NiceAgent
NiceProxyType proxy_type; /* property: Proxy type */
gchar *proxy_username; /* property: Proxy username */
gchar *proxy_password; /* property: Proxy password */
gboolean controlling_mode; /* property: controlling-mode */
gboolean saved_controlling_mode;/* property: controlling-mode */
guint timer_ta; /* property: timer Ta */
guint max_conn_checks; /* property: max connectivity checks */
gboolean force_relay; /* property: force relay */
......@@ -190,6 +190,8 @@ struct _NiceAgent
gboolean use_ice_tcp;
guint conncheck_timer_grace_period; /* ongoing delay before timer stop */
gboolean controlling_mode; /* controlling mode used by the
conncheck */
/* XXX: add pointer to internal data struct for ABI-safe extensions */
};
......
......@@ -405,6 +405,13 @@ nice_agent_class_init (NiceAgentClass *klass)
1, /* not a construct property, ignored */
G_PARAM_READWRITE));
/**
* NiceAgent:controlling-mode:
*
* Whether the agent has the controlling role. This property should
* be modified before gathering candidates, any modification occuring
* later will be hold until ICE is restarted.
*/
g_object_class_install_property (gobject_class, PROP_CONTROLLING_MODE,
g_param_spec_boolean (
"controlling-mode",
......@@ -1106,6 +1113,47 @@ static void priv_generate_tie_breaker (NiceAgent *agent)
nice_rng_generate_bytes (agent->rng, 8, (gchar*)&agent->tie_breaker);
}
static void
priv_update_controlling_mode (NiceAgent *agent, gboolean value)
{
gboolean update_controlling_mode;
GSList *i, *j;
agent->saved_controlling_mode = value;
/* It is safe to update the agent controlling mode when all
* components are still in state disconnected. When we leave
* this state, the role must stay under the control of the
* conncheck algorithm exclusively, until the conncheck is
* eventually restarted. See RFC5245, sect 5.2. Determining Role
*/
if (agent->controlling_mode != agent->saved_controlling_mode) {
update_controlling_mode = TRUE;
for (i = agent->streams;
i && update_controlling_mode; i = i->next) {
NiceStream *stream = i->data;
for (j = stream->components;
j && update_controlling_mode; j = j->next) {
NiceComponent *component = j->data;
if (component->state > NICE_COMPONENT_STATE_DISCONNECTED)
update_controlling_mode = FALSE;
}
}
if (update_controlling_mode) {
agent->controlling_mode = agent->saved_controlling_mode;
nice_debug ("Agent %p : Property set, changing role to \"%s\".",
agent, agent->controlling_mode ? "controlling" : "controlled");
} else {
nice_debug ("Agent %p : Property set, role switch requested "
"but conncheck already started.", agent);
nice_debug ("Agent %p : Property set, staying with role \"%s\" "
"until restart.", agent,
agent->controlling_mode ? "controlling" : "controlled");
}
} else
nice_debug ("Agent %p : Property set, role is already \"%s\".", agent,
agent->controlling_mode ? "controlling" : "controlled");
}
static void
nice_agent_init (NiceAgent *agent)
{
......@@ -1115,6 +1163,7 @@ nice_agent_init (NiceAgent *agent)
/* set defaults; not construct params, so set here */
agent->stun_server_port = DEFAULT_STUN_PORT;
agent->controlling_mode = TRUE;
agent->saved_controlling_mode = TRUE;
agent->max_conn_checks = NICE_AGENT_MAX_CONNECTIVITY_CHECKS_DEFAULT;
agent->nomination_mode = NICE_NOMINATION_MODE_AGGRESSIVE;
......@@ -1213,7 +1262,7 @@ nice_agent_get_property (
break;
case PROP_CONTROLLING_MODE:
g_value_set_boolean (value, agent->controlling_mode);
g_value_set_boolean (value, agent->saved_controlling_mode);
break;
case PROP_FULL_MODE:
......@@ -1422,7 +1471,7 @@ nice_agent_set_property (
break;
case PROP_CONTROLLING_MODE:
agent->controlling_mode = g_value_get_boolean (value);
priv_update_controlling_mode (agent, g_value_get_boolean (value));
break;
case PROP_FULL_MODE:
......@@ -4930,6 +4979,11 @@ nice_agent_restart (
/* step: regenerate tie-breaker value */
priv_generate_tie_breaker (agent);
/* step: reset controlling mode from the property value */
agent->controlling_mode = agent->saved_controlling_mode;
nice_debug ("Agent %p : ICE restart, reset role to \"%s\".",
agent, agent->controlling_mode ? "controlling" : "controlled");
for (i = agent->streams; i; i = i->next) {
NiceStream *stream = i->data;
......
......@@ -301,6 +301,11 @@ static int run_restart_test (NiceAgent *lagent, NiceAgent *ragent, NiceAddress *
nice_agent_set_remote_candidates (lagent, ls_id, NICE_COMPONENT_TYPE_RTCP, cands);
cdes.addr = laddr_rtcp;
nice_agent_set_remote_candidates (ragent, rs_id, NICE_COMPONENT_TYPE_RTCP, cands);
/* This role switch request will be effective after restart. We test
* here that the role cannot be externally modified after conncheck
* has started. */
g_object_set (G_OBJECT (ragent), "controlling-mode", TRUE, NULL);
g_assert (ragent->controlling_mode == FALSE);
g_debug ("test-restart: Set properties, next running mainloop until connectivity checks succeed...");
......@@ -329,10 +334,18 @@ static int run_restart_test (NiceAgent *lagent, NiceAgent *ragent, NiceAddress *
global_ragent_read = 0;
g_assert (nice_agent_send (lagent, ls_id, 1, 16, "1234567812345678") == 16);
/* Both agent have a distinct role at the end of the conncheck */
g_assert (lagent->controlling_mode == TRUE);
g_assert (ragent->controlling_mode == FALSE);
/* step: restart agents, exchange updated credentials */
tie_breaker = ragent->tie_breaker;
nice_agent_restart (ragent);
g_assert (tie_breaker != ragent->tie_breaker);
/* This role switch of ragent should be done now, and both agents
* have now the same role, which should generate a role conflict
* resolution situation */
g_assert (lagent->controlling_mode == TRUE);
g_assert (ragent->controlling_mode == TRUE);
nice_agent_restart (lagent);
{
gchar *ufrag = NULL, *password = NULL;
......@@ -375,6 +388,8 @@ static int run_restart_test (NiceAgent *lagent, NiceAgent *ragent, NiceAddress *
/* note: verify binding requests were resent after restart */
g_assert (global_lagent_ibr_received == TRUE);
g_assert (global_ragent_ibr_received == TRUE);
/* note: verify that a role switch occured for one of the agents */
g_assert (ragent->controlling_mode != lagent->controlling_mode);
g_debug ("test-restart: Ran mainloop, removing streams...");
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment