[bgpd] Stability fixes including bugs 397, 492

I've spent the last several weeks working on stability fixes to bgpd.
These patches fix all of the numerous crashes, assertion failures, memory
leaks and memory stomping I could find.  Valgrind was used extensively.

Added new function bgp_exit() to help catch problems.  If "debug bgp" is
configured and bgpd exits with status of 0, statistics on remaining
lib/memory.c allocations are printed to stderr.  It is my hope that other
developers will use this to stay on top of memory issues.

Example questionable exit:

  bgpd: memstats: Current memory utilization in module LIB:
  bgpd: memstats:  Link List                     :          6
  bgpd: memstats:  Link Node                     :          5
  bgpd: memstats:  Hash                          :          8
  bgpd: memstats:  Hash Bucket                   :          2
  bgpd: memstats:  Hash Index                    :          8
  bgpd: memstats:  Work queue                    :          3
  bgpd: memstats:  Work queue item               :          2
  bgpd: memstats:  Work queue name string        :          3
  bgpd: memstats: Current memory utilization in module BGP:
  bgpd: memstats:  BGP instance                  :          1
  bgpd: memstats:  BGP peer                      :          1
  bgpd: memstats:  BGP peer hostname             :          1
  bgpd: memstats:  BGP attribute                 :          1
  bgpd: memstats:  BGP extra attributes          :          1
  bgpd: memstats:  BGP aspath                    :          1
  bgpd: memstats:  BGP aspath str                :          1
  bgpd: memstats:  BGP table                     :         24
  bgpd: memstats:  BGP node                      :          1
  bgpd: memstats:  BGP route                     :          1
  bgpd: memstats:  BGP synchronise               :          8
  bgpd: memstats:  BGP Process queue             :          1
  bgpd: memstats:  BGP node clear queue          :          1
  bgpd: memstats: NOTE: If configuration exists, utilization may be expected.

Example clean exit:

  bgpd: memstats: No remaining tracked memory utilization.

This patch fixes bug #397: "Invalid free in bgp_announce_check()".

This patch fixes bug #492: "SIGBUS in bgpd/bgp_route.c:
bgp_clear_route_node()".

My apologies for not separating out these changes into individual patches.
The complexity of doing so boggled what is left of my brain.  I hope this
is all still useful to the community.

This code has been production tested, in non-route-server-client mode, on
a linux 32-bit box and a 64-bit box.

Release/reset functions, used by bgp_exit(), added to:

  bgpd/bgp_attr.c,h
  bgpd/bgp_community.c,h
  bgpd/bgp_dump.c,h
  bgpd/bgp_ecommunity.c,h
  bgpd/bgp_filter.c,h
  bgpd/bgp_nexthop.c,h
  bgpd/bgp_route.c,h
  lib/routemap.c,h

File by file analysis:

* bgpd/bgp_aspath.c: Prevent re-use of ashash after it is released.

* bgpd/bgp_attr.c: #if removed uncalled cluster_dup().

* bgpd/bgp_clist.c,h: Allow community_list_terminate() to be called from
  bgp_exit().

* bgpd/bgp_filter.c: Fix aslist->name use without allocation check, and
  also fix memory leak.

* bgpd/bgp_main.c: Created bgp_exit() exit routine.  This function frees
  allocations made as part of bgpd initialization and, to some extent,
  configuration.  If "debug bgp" is configured, memory stats are printed
  as described above.

* bgpd/bgp_nexthop.c: zclient_new() already allocates stream for
  ibuf/obuf, so bgp_scan_init() shouldn't do it too.  Also, made it so
  zlookup is global so bgp_exit() can use it.

* bgpd/bgp_packet.c: bgp_capability_msg_parse() call to bgp_clear_route()
  adjusted to use new BGP_CLEAR_ROUTE_NORMAL flag.

* bgpd/bgp_route.h: Correct reference counter "lock" to be signed.
  bgp_clear_route() now accepts a bgp_clear_route_type of either
  BGP_CLEAR_ROUTE_NORMAL or BGP_CLEAR_ROUTE_MY_RSCLIENT.

* bgpd/bgp_route.c:
  - bgp_process_rsclient(): attr was being zero'ed and then
    bgp_attr_extra_free() was being called with it, even though it was
    never filled with valid data.

  - bgp_process_rsclient(): Make sure rsclient->group is not NULL before
    use.

  - bgp_processq_del(): Add call to bgp_table_unlock().

  - bgp_process(): Add call to bgp_table_lock().

  - bgp_update_rsclient(): memset clearing of new_attr not needed since
    declarationw with "= { 0 }" does it.  memset was already commented
    out.

  - bgp_update_rsclient(): Fix screwed up misleading indentation.

  - bgp_withdraw_rsclient(): Fix screwed up misleading indentation.

  - bgp_clear_route_node(): Support BGP_CLEAR_ROUTE_MY_RSCLIENT.

  - bgp_clear_node_queue_del(): Add call to bgp_table_unlock() and also
    free struct bgp_clear_node_queue used for work item.

  - bgp_clear_node_complete(): Do peer_unlock() after BGP_EVENT_ADD() in
    case peer is released by peer_unlock() call.

  - bgp_clear_route_table(): Support BGP_CLEAR_ROUTE_MY_RSCLIENT.  Use
    struct bgp_clear_node_queue to supply data to worker.  Add call to
    bgp_table_lock().

  - bgp_clear_route(): Add support for BGP_CLEAR_ROUTE_NORMAL or
    BGP_CLEAR_ROUTE_MY_RSCLIENT.

  - bgp_clear_route_all(): Use BGP_CLEAR_ROUTE_NORMAL.

  Bug 397 fixes:

    - bgp_default_originate()
    - bgp_announce_table()

* bgpd/bgp_table.h:
  - struct bgp_table: Added reference count.  Changed type of owner to be
    "struct peer *" rather than "void *".

  - struct bgp_node: Correct reference counter "lock" to be signed.

* bgpd/bgp_table.c:
  - Added bgp_table reference counting.

  - bgp_table_free(): Fixed cleanup code.  Call peer_unlock() on owner if
    set.

  - bgp_unlock_node(): Added assertion.

  - bgp_node_get(): Added call to bgp_lock_node() to code path that it was
    missing from.

* bgpd/bgp_vty.c:
  - peer_rsclient_set_vty(): Call peer_lock() as part of peer assignment
    to owner.  Handle failure gracefully.

  - peer_rsclient_unset_vty(): Add call to bgp_clear_route() with
    BGP_CLEAR_ROUTE_MY_RSCLIENT purpose.

* bgpd/bgp_zebra.c: Made it so zclient is global so bgp_exit() can use it.

* bgpd/bgpd.c:
  - peer_lock(): Allow to be called when status is "Deleted".

  - peer_deactivate(): Supply BGP_CLEAR_ROUTE_NORMAL purpose to
    bgp_clear_route() call.

  - peer_delete(): Common variable listnode pn.  Fix bug in which rsclient
    was only dealt with if not part of a peer group.  Call
    bgp_clear_route() for rsclient, if appropriate, and do so with
    BGP_CLEAR_ROUTE_MY_RSCLIENT purpose.

  - peer_group_get(): Use XSTRDUP() instead of strdup() for conf->host.

  - peer_group_bind(): Call bgp_clear_route() for rsclient, and do so with
    BGP_CLEAR_ROUTE_MY_RSCLIENT purpose.

  - bgp_create(): Use XSTRDUP() instead of strdup() for peer_self->host.

  - bgp_delete(): Delete peers before groups, rather than after.  And then
    rather than deleting rsclients, verify that there are none at this
    point.

  - bgp_unlock(): Add assertion.

  - bgp_free(): Call bgp_table_finish() rather than doing XFREE() itself.

* lib/command.c,h: Compiler warning fixes.  Add cmd_terminate().  Fixed
  massive leak in install_element() in which cmd_make_descvec() was being
  called more than once for the same cmd->strvec/string/doc.

* lib/log.c: Make closezlog() check fp before calling fclose().

* lib/memory.c: Catch when alloc count goes negative by using signed
  counts.  Correct #endif comment.  Add log_memstats_stderr().

* lib/memory.h: Add log_memstats_stderr().

* lib/thread.c: thread->funcname was being accessed in thread_call() after
  it had been freed.  Rearranged things so that thread_call() frees
  funcname.  Also made it so thread_master_free() cleans up cpu_record.

* lib/vty.c,h: Use global command_cr.  Add vty_terminate().

* lib/zclient.c,h: Re-enable zclient_free().

bgpd/bgp_aspath.c

@@ -1799,6 +1799,7 @@ void
 aspath_finish (void)
 {
   hash_free (ashash);
+  ashash = NULL;
 
   if (snmp_stream)
     stream_free (snmp_stream);

bgpd/bgp_attr.c

@@ -148,6 +148,7 @@ cluster_free (struct cluster_list *cluster)
   XFREE (MTYPE_CLUSTER, cluster);
 }
 
+#if 0
 static struct cluster_list *
 cluster_dup (struct cluster_list *cluster)
 {
@@ -166,6 +167,7 @@ cluster_dup (struct cluster_list *cluster)
 
   return new;
 }
+#endif
 
 static struct cluster_list *
 cluster_intern (struct cluster_list *cluster)
@@ -198,6 +200,13 @@ cluster_init (void)
 {
   cluster_hash = hash_create (cluster_hash_key_make, cluster_hash_cmp);
 }
+
+static void
+cluster_finish (void)
+{
+  hash_free (cluster_hash);
+  cluster_hash = NULL;
+}
 
 /* Unknown transit attribute. */
 static struct hash *transit_hash;
@@ -278,6 +287,13 @@ transit_init (void)
 {
   transit_hash = hash_create (transit_hash_key_make, transit_hash_cmp);
 }
+
+static void
+transit_finish (void)
+{
+  hash_free (transit_hash);
+  transit_hash = NULL;
+}
 
 /* Attribute hash routines. */
 static struct hash *attrhash;
@@ -435,6 +451,13 @@ attrhash_init (void)
   attrhash = hash_create (attrhash_key_make, attrhash_cmp);
 }
 
+static void
+attrhash_finish (void)
+{
+  hash_free (attrhash);
+  attrhash = NULL;
+}
+
 static void
 attr_show_all_iterator (struct hash_backet *backet, struct vty *vty)
 {
@@ -2302,6 +2325,17 @@ bgp_attr_init (void)
   transit_init ();
 }
 
+void
+bgp_attr_finish (void)
+{
+  aspath_finish ();
+  attrhash_finish ();
+  community_finish ();
+  ecommunity_finish ();
+  cluster_finish ();
+  transit_finish ();
+}
+
 /* Make attribute packet. */
 void
 bgp_dump_routes_attr (struct stream *s, struct attr *attr, 

bgpd/bgp_attr.h

@@ -140,6 +140,7 @@ struct transit
 
 /* Prototypes. */
 extern void bgp_attr_init (void);
+extern void bgp_attr_finish (void);
 extern int bgp_attr_parse (struct peer *, struct attr *, bgp_size_t,
 		    struct bgp_nlri *, struct bgp_nlri *);
 extern int bgp_attr_check (struct peer *, struct attr *);

bgpd/bgp_clist.c

@@ -829,7 +829,7 @@ community_list_init (void)
 }
 
 /* Terminate community-list.  */
-static void
+void
 community_list_terminate (struct community_list_handler *ch)
 {
   struct community_list_master *cm;

bgpd/bgp_clist.h

@@ -125,6 +125,7 @@ extern struct community_list_handler *bgp_clist;
 
 /* Prototypes.  */
 extern struct community_list_handler *community_list_init (void);
+extern void community_list_terminate (struct community_list_handler *);
 
 extern int community_list_set (struct community_list_handler *ch,
 			       const char *name, const char *str, int direct,

bgpd/bgp_community.c

@@ -636,3 +636,10 @@ community_init (void)
   comhash = hash_create ((unsigned int (*) (void *))community_hash_make,
 			 (int (*) (const void *, const void *))community_cmp);
 }
+
+void
+community_finish (void)
+{
+  hash_free (comhash);
+  comhash = NULL;
+}

bgpd/bgp_community.h

@@ -52,6 +52,7 @@ struct community
 
 /* Prototypes of communities attribute functions.  */
 extern void community_init (void);
+extern void community_finish (void);
 extern void community_free (struct community *);
 extern struct community *community_uniq_sort (struct community *);
 extern struct community *community_parse (u_int32_t *, u_short);

bgpd/bgp_dump.c

@@ -865,3 +865,10 @@ bgp_dump_init (void)
   install_element (CONFIG_NODE, &dump_bgp_routes_interval_cmd);
   install_element (CONFIG_NODE, &no_dump_bgp_routes_cmd);
 }
+
+void
+bgp_dump_finish (void)
+{
+  stream_free (bgp_dump_obuf);
+  bgp_dump_obuf = NULL;
+}

bgpd/bgp_dump.h

@@ -48,6 +48,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 #define TABLE_DUMP_V2_PEER_INDEX_TABLE_AS4 2
 
 extern void bgp_dump_init (void);
+extern void bgp_dump_finish (void);
 extern void bgp_dump_state (struct peer *, int, int);
 extern void bgp_dump_packet (struct peer *, int, struct stream *);
 

bgpd/bgp_ecommunity.c

@@ -262,6 +262,13 @@ ecommunity_init (void)
 {
   ecomhash = hash_create (ecommunity_hash_make, ecommunity_cmp);
 }
+
+void
+ecommunity_finish (void)
+{
+  hash_free (ecomhash);
+  ecomhash = NULL;
+}
 
 /* Extended Communities token enum. */
 enum ecommunity_token

bgpd/bgp_ecommunity.h

@@ -66,6 +66,7 @@ struct ecommunity_val
 #define ecom_length(X)    ((X)->size * ECOMMUNITY_SIZE)
 
 extern void ecommunity_init (void);
+extern void ecommunity_finish (void);
 extern void ecommunity_free (struct ecommunity *);
 extern struct ecommunity *ecommunity_parse (u_int8_t *, u_short);
 extern struct ecommunity *ecommunity_dup (struct ecommunity *);

bgpd/bgp_filter.c

@@ -181,6 +181,11 @@ as_list_new (void)
 static void
 as_list_free (struct as_list *aslist)
 {
+  if (aslist->name)
+    {
+      free (aslist->name);
+      aslist->name = NULL;
+    }
   XFREE (MTYPE_AS_LIST, aslist);
 }
 
@@ -198,6 +203,7 @@ as_list_insert (const char *name)
   /* Allocate new access_list and copy given name. */
   aslist = as_list_new ();
   aslist->name = strdup (name);
+  assert (aslist->name);
 
   /* If name is made by all digit character.  We treat it as
      number. */
@@ -693,3 +699,28 @@ bgp_filter_init (void)
   install_element (ENABLE_NODE, &show_ip_as_path_access_list_cmd);
   install_element (ENABLE_NODE, &show_ip_as_path_access_list_all_cmd);
 }
+
+void
+bgp_filter_reset (void)
+{
+  struct as_list *aslist;
+  struct as_list *next;
+
+  for (aslist = as_list_master.num.head; aslist; aslist = next)
+    {
+      next = aslist->next;
+      as_list_delete (aslist);
+    }
+
+  for (aslist = as_list_master.str.head; aslist; aslist = next)
+    {
+      next = aslist->next;
+      as_list_delete (aslist);
+    }
+
+  assert (as_list_master.num.head == NULL);
+  assert (as_list_master.num.tail == NULL);
+
+  assert (as_list_master.str.head == NULL);
+  assert (as_list_master.str.tail == NULL);
+}

bgpd/bgp_filter.h

@@ -28,6 +28,7 @@ enum as_filter_type
 };
 
 extern void bgp_filter_init (void);
+extern void bgp_filter_reset (void);
 
 extern enum as_filter_type as_list_apply (struct as_list *, void *);
 

bgpd/bgp_main.c

@@ -31,10 +31,22 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 #include "log.h"
 #include "privs.h"
 #include "sigevent.h"
+#include "zclient.h"
+#include "routemap.h"
+#include "filter.h"
+#include "plist.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_attr.h"
 #include "bgpd/bgp_mplsvpn.h"
+#include "bgpd/bgp_aspath.h"
+#include "bgpd/bgp_dump.h"
+#include "bgpd/bgp_route.h"
+#include "bgpd/bgp_nexthop.h"
+#include "bgpd/bgp_regex.h"
+#include "bgpd/bgp_clist.h"
+#include "bgpd/bgp_debug.h"
+#include "bgpd/bgp_filter.h"
 
 /* bgpd options, we use GNU getopt library. */
 static const struct option longopts[] = 
@@ -61,6 +73,8 @@ void sighup (void);
 void sigint (void);
 void sigusr1 (void);
 
+static void bgp_exit (int);
+
 static struct quagga_signal_t bgp_signals[] = 
 {
   { 
@@ -182,7 +196,7 @@ sigint (void)
   if (! retain_mode)
     bgp_terminate ();
 
-  exit (0);
+  bgp_exit (0);
 }
 
 /* SIGUSR1 handler. */
@@ -191,6 +205,99 @@ sigusr1 (void)
 {
   zlog_rotate (NULL);
 }
+
+/*
+  Try to free up allocations we know about so that diagnostic tools such as
+  valgrind are able to better illuminate leaks.
+
+  Zebra route removal and protocol teardown are not meant to be done here.
+  For example, "retain_mode" may be set.
+*/
+static void
+bgp_exit (int status)
+{
+  struct bgp *bgp;
+  struct listnode *node, *nnode;
+  int *socket;
+  struct interface *ifp;
+  extern struct zclient *zclient;
+  extern struct zclient *zlookup;
+
+  /* it only makes sense for this to be called on a clean exit */
+  assert (status == 0);
+
+  /* reverse bgp_master_init */
+  for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp))
+    bgp_delete (bgp);
+  list_free (bm->bgp);
+
+  /* reverse bgp_master_init */
+  for (ALL_LIST_ELEMENTS_RO(bm->listen_sockets, node, socket))
+    {
+      if (close ((int)(long)socket) == -1)
+        zlog_err ("close (%d): %s", (int)(long)socket, safe_strerror (errno));
+    }
+  list_delete (bm->listen_sockets);
+
+  /* reverse bgp_zebra_init/if_init */
+  if (retain_mode)
+    if_add_hook (IF_DELETE_HOOK, NULL);
+  for (ALL_LIST_ELEMENTS (iflist, node, nnode, ifp))
+    if_delete (ifp);
+  list_free (iflist);
+
+  /* reverse bgp_attr_init */
+  bgp_attr_finish ();
+
+  /* reverse bgp_dump_init */
+  bgp_dump_finish ();
+
+  /* reverse bgp_route_init */
+  bgp_route_finish ();
+
+  /* reverse bgp_route_map_init/route_map_init */
+  route_map_finish ();
+
+  /* reverse bgp_scan_init */
+  bgp_scan_finish ();
+
+  /* reverse access_list_init */
+  access_list_add_hook (NULL);
+  access_list_delete_hook (NULL);
+  access_list_reset ();
+
+  /* reverse bgp_filter_init */
+  as_list_add_hook (NULL);
+  as_list_delete_hook (NULL);
+  bgp_filter_reset ();
+
+  /* reverse prefix_list_init */
+  prefix_list_add_hook (NULL);
+  prefix_list_delete_hook (NULL);
+  prefix_list_reset ();
+
+  /* reverse community_list_init */
+  community_list_terminate (bgp_clist);
+
+  cmd_terminate ();
+  vty_terminate ();
+  if (zclient)
+    zclient_free (zclient);
+  if (zlookup)
+    zclient_free (zlookup);
+
+  /* reverse bgp_master_init */
+  if (master)
+    thread_master_free (master);
+
+  if (zlog_default)
+    closezlog (zlog_default);
+
+  if (CONF_BGP_DEBUG (normal, NORMAL))
+    log_memstats_stderr ("bgpd");
+
+  exit (status);
+}
 
 /* Main routine of bgpd. Treatment of argument and start bgp finite
    state machine is handled at here. */