BGP: Rework iteration of peer_af_array

While processing references to the macro PEERAF_FOREACH(), aggressive loop
optimization by gcc 4.9.x (probably 4.8 and greater) was resulting in the
generated code not checking on the index as well as eliminating some code.
This was leading to a dereference of invalid memory when a BGP peer came up.

The fix is to scrap this convoluted macro. Two other changes done are to
eliminate overloading of "afindex" and make the loop iterator an integer.

Signed-off-by: Vivek Venkatraman <vivek@cumulusnetworks.com>
Reviewed-by:   Dave Olson <olson@cumulusnetworks.com>
Reviewed-by:   Donald Sharp <sharpd@cumulusnetworks.com>
Reviewed-by:   Daniel Walton <dwalton@cumulusnetworks.com>
Ticket: CM-8889
Reviewed By: CCR-4018
Testing Done: Verified failure scenario

Note: This code was added as part of update-groups implementation; when
upstreaming update-groups, this patch should also be included.

diff --git a/bgpd/bgp_debug.c b/bgpd/bgp_debug.c
index 68106b449e675d248e13677e90d687e803b454ee..c86df535aa84ae4c88bba2d4e890cb6607546bec 100644 (file)
--- a/bgpd/bgp_debug.c
+++ b/bgpd/bgp_debug.c
@@ -1081,18 +1081,22 @@ DEFUN (debug_bgp_update_direct_peer,
     {
       struct peer *peer;
       struct peer_af *paf;
-      enum bgp_af_index af;
+      int afidx;
 
       bgp_debug_list_add_entry(bgp_debug_update_out_peers, host, NULL);
       peer = bgp_find_peer (vty, host);
 
       if (peer)
         {
-          PEERAF_FOREACH (peer, paf, af)
+          for (afidx = BGP_AF_START; afidx < BGP_AF_MAX; afidx++)
             {
-              if (PAF_SUBGRP (paf))
+              paf = peer->peer_af_array[afidx];
+              if (paf != NULL)
                 {
-                  UPDGRP_PEER_DBG_EN(PAF_SUBGRP(paf)->update_group);
+                  if (PAF_SUBGRP (paf))
+                    {
+                      UPDGRP_PEER_DBG_EN(PAF_SUBGRP(paf)->update_group);
+                    }
                 }
             }
         }
@@ -1220,16 +1224,20 @@ DEFUN (no_debug_bgp_update_direct_peer,
 
       struct peer *peer;
       struct peer_af *paf;
-      enum bgp_af_index af;
+      int afidx;
       peer = bgp_find_peer (vty, host);
 
       if (peer)
         {
-          PEERAF_FOREACH (peer, paf, af)
+          for (afidx = BGP_AF_START; afidx < BGP_AF_MAX; afidx++)
             {
-              if (PAF_SUBGRP (paf))
+              paf = peer->peer_af_array[afidx];
+              if (paf != NULL)
                 {
-                  UPDGRP_PEER_DBG_DIS(PAF_SUBGRP(paf)->update_group);
+                  if (PAF_SUBGRP (paf))
+                    {
+                      UPDGRP_PEER_DBG_DIS(PAF_SUBGRP(paf)->update_group);
+                    }
                 }
             }
         }

diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h
index b94519ef00e87ce2b0083dee06a6815e09451472..aab2458b30a83147d574bf0e8976264b40478bf4 100644 (file)
--- a/bgpd/bgp_updgrp.h
+++ b/bgpd/bgp_updgrp.h
@@ -528,9 +528,14 @@ static inline void
 update_group_adjust_peer_afs (struct peer *peer)
 {
   struct peer_af *paf;
-  enum bgp_af_index afi;
+  int afidx;
 
-  PEERAF_FOREACH (peer, paf, afi) update_group_adjust_peer (paf);
+  for (afidx = BGP_AF_START; afidx < BGP_AF_MAX; afidx++)
+    {
+      paf = peer->peer_af_array[afidx];
+      if (paf != NULL)
+        update_group_adjust_peer (paf);
+    }
 }
 
 /*
@@ -542,10 +547,14 @@ static inline void
 update_group_remove_peer_afs (struct peer *peer)
 {
   struct peer_af *paf;
-  enum bgp_af_index afi;
+  int afidx;
 
-  PEERAF_FOREACH (peer, paf, afi)
-    update_subgroup_remove_peer (PAF_SUBGRP (paf), paf);
+  for (afidx = BGP_AF_START; afidx < BGP_AF_MAX; afidx++)
+    {
+      paf = peer->peer_af_array[afidx];
+      if (paf != NULL)
+        update_subgroup_remove_peer (PAF_SUBGRP (paf), paf);
+    }
 }
 
 /*
@@ -592,9 +601,14 @@ static inline void
 bgp_announce_peer (struct peer *peer)
 {
   struct peer_af *paf;
-  enum bgp_af_index af;
+  int afidx;
 
-  PEERAF_FOREACH (peer, paf, af) subgroup_announce_all (PAF_SUBGRP (paf));
+  for (afidx = BGP_AF_START; afidx < BGP_AF_MAX; afidx++)
+    {
+      paf = peer->peer_af_array[afidx];
+      if (paf != NULL)
+        subgroup_announce_all (PAF_SUBGRP (paf));
+    }
 }
 
 /**

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index af2f284dab50698ad20c6a9026741dd959edc528..c7f9232a89b7dba117bd2a4c2122a712399a25a5 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -1142,7 +1142,7 @@ peer_xfer_config (struct peer *peer_dst, struct peer *peer_src)
   struct peer_af *paf;
   afi_t afi;
   safi_t safi;
-  enum bgp_af_index afindex;
+  int afidx;
 
   assert(peer_src);
   assert(peer_dst);
@@ -1185,8 +1185,12 @@ peer_xfer_config (struct peer *peer_dst, struct peer *peer_src)
        peer_dst->allowas_in[afi][safi] = peer_src->allowas_in[afi][safi];
     }
 
-  PEERAF_FOREACH(peer_src, paf, afindex)
-    peer_af_create(peer_dst, paf->afi, paf->safi);
+  for (afidx = BGP_AF_START; afidx < BGP_AF_MAX; afidx++)
+    {
+      paf = peer_src->peer_af_array[afidx];
+      if (paf != NULL)
+        peer_af_create(peer_dst, paf->afi, paf->safi);
+    }
 
   /* update-source apply */
   if (peer_src->update_source)

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 14ebf840ab24942d1fb7f7d4ea771c45f40a14d5..7af51f304c5655fe0e230c467d9673d363741239 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -840,12 +840,6 @@ struct bgp_nlri
   bgp_size_t length;
 };
 
-#define PEERAF_FOREACH(peer, paf, afi)                                 \
-  for ((afi) = BGP_AF_START, (paf) = (peer)->peer_af_array[(afi)];     \
-       (afi) < BGP_AF_MAX;                                             \
-       (afi)++, (paf) = (peer)->peer_af_array[(afi)])                  \
-    if ((paf) != NULL)                                                 \
-
 /* BGP versions.  */
 #define BGP_VERSION_4                           4