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

3 files changed, 74 insertions, 3 deletions

diff --git a/agent/agent-priv.h b/agent/agent-priv.h
index 714ecff..7269be0 100644
--- a/agent/agent-priv.h
+++ b/agent/agent-priv.h
@@ -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 */
 };
 
diff --git a/agent/agent.c b/agent/agent.c
index a4dcc0c..0773c53 100644
--- a/agent/agent.c
+++ b/agent/agent.c
@@ -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",
@@ -1107,6 +1114,47 @@ static void priv_generate_tie_breaker (NiceAgent *agent)
 }
 
 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)
 {
   agent->next_candidate_id = 1;
@@ -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;
 
diff --git a/tests/test-restart.c b/tests/test-restart.c
index c2cbe9a..afc51b6 100644
--- a/tests/test-restart.c
+++ b/tests/test-restart.c
@@ -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...");