igmp: fix igmp proxy group merge

When merging proxy groups in igmp_proxy_device_merge_group(), the call
to igmp_proxy_device_merge_src() can end up removing the current proxy
group via igmp_group_clear(). When that happens, it must returns NULL so
that igmp_proxy_device_merge_config() does not send a IGMPv3 report for
a dead proxy group.
Make igmp_group_clear() reset the group pointer to NULL to fix this bug
and to detect similar bugs more easily.

Type: fix

Change-Id: I229e55b5bfa71734d7844893f5209a66fa3cc8ae
Signed-off-by: Benoît Ganne <bganne@cisco.com>

diff --git a/src/plugins/igmp/igmp.c b/src/plugins/igmp/igmp.c
index 1e9f647..eea39d3 100644 (file)
--- a/src/plugins/igmp/igmp.c
+++ b/src/plugins/igmp/igmp.c
@@ -282,7 +282,7 @@ igmp_listen (vlib_main_t * vm,
          }
 
          if (0 == igmp_group_n_srcs (group, mode))
-           igmp_group_clear (group);
+           igmp_group_clear (&group);
 
          vec_free (added);
          vec_free (removed);

diff --git a/src/plugins/igmp/igmp_config.c b/src/plugins/igmp/igmp_config.c
index f9acc29..7637adb 100644 (file)
--- a/src/plugins/igmp/igmp_config.c
+++ b/src/plugins/igmp/igmp_config.c
@@ -31,7 +31,7 @@ igmp_clear_config (igmp_config_t * config)
   /* *INDENT-OFF* */
   FOR_EACH_GROUP (group, config,
     ({
-      igmp_group_clear (group);
+      igmp_group_clear (&group);
     }));
   /* *INDENT-ON* */
 

diff --git a/src/plugins/igmp/igmp_group.c b/src/plugins/igmp/igmp_group.c
index 51a44d2..eec4c9b 100644 (file)
--- a/src/plugins/igmp/igmp_group.c
+++ b/src/plugins/igmp/igmp_group.c
@@ -65,38 +65,39 @@ igmp_group_src_update (igmp_group_t * group,
 }
 
 void
-igmp_group_clear (igmp_group_t * group)
+igmp_group_clear (igmp_group_t ** group)
 {
   igmp_config_t *config;
   u32 ii;
 
-  ASSERT (group);
+  ASSERT (*group);
 
-  config = igmp_config_get (group->config);
+  config = igmp_config_get ((*group)->config);
 
   /* If interface is in ROUTER mode and IGMP proxy is enabled
    * remove mfib path.
    */
   if (config->mode == IGMP_MODE_ROUTER)
     {
-      igmp_proxy_device_mfib_path_add_del (group, /* add */ 0);
+      igmp_proxy_device_mfib_path_add_del (*group, /* add */ 0);
     }
 
   IGMP_DBG ("clear-group: %U %U",
-           format_igmp_key, group->key,
+           format_igmp_key, (*group)->key,
            format_vnet_sw_if_index_name,
            vnet_get_main (), config->sw_if_index);
 
-  igmp_group_free_all_srcs (group);
+  igmp_group_free_all_srcs (*group);
 
   for (ii = 0; ii < IGMP_GROUP_N_TIMERS; ii++)
     {
-      igmp_timer_retire (&group->timers[ii]);
+      igmp_timer_retire (&(*group)->timers[ii]);
     }
 
-  hash_unset_mem (config->igmp_group_by_key, group->key);
-  clib_mem_free (group->key);
-  pool_put (igmp_main.groups, group);
+  hash_unset_mem (config->igmp_group_by_key, (*group)->key);
+  clib_mem_free ((*group)->key);
+  pool_put (igmp_main.groups, *group);
+  *group = 0;
 }
 
 igmp_group_t *

diff --git a/src/plugins/igmp/igmp_group.h b/src/plugins/igmp/igmp_group.h
index 7d4dfb6..0eb4f43 100644 (file)
--- a/src/plugins/igmp/igmp_group.h
+++ b/src/plugins/igmp/igmp_group.h
@@ -103,7 +103,7 @@ do {                                                                    \
  */
 struct igmp_config_t_;
 
-extern void igmp_group_clear (igmp_group_t * group);
+extern void igmp_group_clear (igmp_group_t ** group);
 extern void igmp_group_free_all_srcs (igmp_group_t * group);
 
 extern igmp_group_t *igmp_group_alloc (struct igmp_config_t_ *config,

diff --git a/src/plugins/igmp/igmp_proxy.c b/src/plugins/igmp/igmp_proxy.c
index dc2b4c1..2167740 100644 (file)
--- a/src/plugins/igmp/igmp_proxy.c
+++ b/src/plugins/igmp/igmp_proxy.c
@@ -246,18 +246,18 @@ igmp_proxy_device_block_src (igmp_config_t * config, igmp_group_t * group,
     {
       igmp_proxy_device_mfib_path_add_del (proxy_group, 0);
       igmp_proxy_device_mfib_path_add_del (group, 0);
-      igmp_group_clear (proxy_group);
+      igmp_group_clear (&proxy_group);
     }
 }
 
 always_inline void
-igmp_proxy_device_merge_src (igmp_group_t * proxy_group, igmp_src_t * src,
+igmp_proxy_device_merge_src (igmp_group_t ** proxy_group, igmp_src_t * src,
                             ip46_address_t ** srcaddrs, u8 block)
 {
   igmp_src_t *proxy_src;
   u32 d_config;
 
-  proxy_src = igmp_src_lookup (proxy_group, src->key);
+  proxy_src = igmp_src_lookup (*proxy_group, src->key);
 
   if (proxy_src == NULL)
     {
@@ -267,11 +267,11 @@ igmp_proxy_device_merge_src (igmp_group_t * proxy_group, igmp_src_t * src,
       d_config = igmp_group_get (src->group)->config;
 
       proxy_src =
-       igmp_src_alloc (igmp_group_index (proxy_group), src->key,
+       igmp_src_alloc (igmp_group_index (*proxy_group), src->key,
                        IGMP_MODE_HOST);
 
-      hash_set_mem (proxy_group->igmp_src_by_key
-                   [proxy_group->router_filter_mode], proxy_src->key,
+      hash_set_mem ((*proxy_group)->igmp_src_by_key
+                   [(*proxy_group)->router_filter_mode], proxy_src->key,
                    igmp_src_index (proxy_src));
 
       vec_validate_init_empty (proxy_src->referance_by_config_index, d_config,
@@ -299,12 +299,12 @@ igmp_proxy_device_merge_src (igmp_group_t * proxy_group, igmp_src_t * src,
 
          vec_add1 (*srcaddrs, *proxy_src->key);
 
-         igmp_group_src_remove (proxy_group, proxy_src);
+         igmp_group_src_remove (*proxy_group, proxy_src);
          igmp_src_free (proxy_src);
 
-         if (igmp_group_n_srcs (proxy_group, IGMP_FILTER_MODE_INCLUDE) == 0)
+         if (igmp_group_n_srcs (*proxy_group, IGMP_FILTER_MODE_INCLUDE) == 0)
            {
-             igmp_proxy_device_mfib_path_add_del (proxy_group, 0);
+             igmp_proxy_device_mfib_path_add_del (*proxy_group, 0);
              igmp_group_clear (proxy_group);
            }
          return;
@@ -348,7 +348,7 @@ igmp_proxy_device_merge_group (igmp_proxy_device_t * proxy_device,
   /* *INDENT-OFF* */
   FOR_EACH_SRC (src, group, group->router_filter_mode,
     ({
-      igmp_proxy_device_merge_src (proxy_group, src, srcaddrs, block);
+      igmp_proxy_device_merge_src (&proxy_group, src, srcaddrs, block);
     }));
   /* *INDENT-ON* */
   return proxy_group;

diff --git a/src/plugins/igmp/igmp_src.c b/src/plugins/igmp/igmp_src.c
index 64768ab..bec46f8 100644 (file)
--- a/src/plugins/igmp/igmp_src.c
+++ b/src/plugins/igmp/igmp_src.c
@@ -66,7 +66,7 @@ igmp_src_exp (u32 obj, void *dat)
   igmp_src_free (src);
 
   if (0 == igmp_group_n_srcs (group, IGMP_FILTER_MODE_INCLUDE))
-    igmp_group_clear (group);
+    igmp_group_clear (&group);
 }
 
 igmp_src_t *