[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().
diff --git a/lib/command.c b/lib/command.c
index 0bbd99e..31c067a 100644
--- a/lib/command.c
+++ b/lib/command.c
@@ -37,6 +37,9 @@
each daemon maintains each own cmdvec. */
vector cmdvec = NULL;
+struct desc desc_cr;
+char *command_cr = NULL;
+
/* Host information structure. */
struct host host;
@@ -199,8 +202,8 @@
static int
cmp_node (const void *p, const void *q)
{
- const struct cmd_element *a = *(struct cmd_element **)p;
- const struct cmd_element *b = *(struct cmd_element **)q;
+ const struct cmd_element *a = *(struct cmd_element * const *)p;
+ const struct cmd_element *b = *(struct cmd_element * const *)q;
return strcmp (a->string, b->string);
}
@@ -208,8 +211,8 @@
static int
cmp_desc (const void *p, const void *q)
{
- const struct desc *a = *(struct desc **)p;
- const struct desc *b = *(struct desc **)q;
+ const struct desc *a = *(struct desc * const *)p;
+ const struct desc *b = *(struct desc * const *)q;
return strcmp (a->cmd, b->cmd);
}
@@ -223,7 +226,7 @@
vector descvec;
struct cmd_element *cmd_element;
- for (i = 0; i < vector_active (cmdvec); i++)
+ for (i = 0; i < vector_active (cmdvec); i++)
if ((cnode = vector_slot (cmdvec, i)) != NULL)
{
vector cmd_vector = cnode->cmd_vector;
@@ -497,7 +500,9 @@
vector_set (cnode->cmd_vector, cmd);
- cmd->strvec = cmd_make_descvec (cmd->string, cmd->doc);
+ if (cmd->strvec == NULL)
+ cmd->strvec = cmd_make_descvec (cmd->string, cmd->doc);
+
cmd->cmdsize = cmd_cmdsize (cmd->strvec);
}
@@ -1588,7 +1593,6 @@
int ret;
enum match_type match;
char *command;
- static struct desc desc_cr = { "<cr>", "" };
/* Set index. */
if (vector_active (vline) == 0)
@@ -1665,7 +1669,6 @@
for (i = 0; i < vector_active (cmd_vector); i++)
if ((cmd_element = vector_slot (cmd_vector, i)) != NULL)
{
- const char *string = NULL;
vector strvec = cmd_element->strvec;
/* if command is NULL, index may be equal to vector_active */
@@ -1676,8 +1679,7 @@
/* Check if command is completed. */
if (command == NULL && index == vector_active (strvec))
{
- string = "<cr>";
- if (!desc_unique_string (matchvec, string))
+ if (!desc_unique_string (matchvec, command_cr))
vector_set (matchvec, &desc_cr);
}
else
@@ -1689,6 +1691,8 @@
for (j = 0; j < vector_active (descvec); j++)
if ((desc = vector_slot (descvec, j)))
{
+ const char *string;
+
string = cmd_entry_function_desc (command, desc->cmd);
if (string)
{
@@ -3506,6 +3510,8 @@
void
host_config_set (char *filename)
{
+ if (host.config)
+ XFREE (MTYPE_HOST, host.config);
host.config = XSTRDUP (MTYPE_HOST, filename);
}
@@ -3529,6 +3535,10 @@
void
cmd_init (int terminal)
{
+ command_cr = XSTRDUP(MTYPE_STRVEC, "<cr>");
+ desc_cr.cmd = command_cr;
+ desc_cr.str = XSTRDUP(MTYPE_STRVEC, "");
+
/* Allocate initial top vector of commands. */
cmdvec = vector_init (VECTOR_MIN_SIZE);
@@ -3645,3 +3655,74 @@
}
srand(time(NULL));
}
+
+void
+cmd_terminate ()
+{
+ unsigned int i, j, k, l;
+ struct cmd_node *cmd_node;
+ struct cmd_element *cmd_element;
+ struct desc *desc;
+ vector cmd_node_v, cmd_element_v, desc_v;
+
+ if (cmdvec)
+ {
+ for (i = 0; i < vector_active (cmdvec); i++)
+ if ((cmd_node = vector_slot (cmdvec, i)) != NULL)
+ {
+ cmd_node_v = cmd_node->cmd_vector;
+
+ for (j = 0; j < vector_active (cmd_node_v); j++)
+ if ((cmd_element = vector_slot (cmd_node_v, j)) != NULL &&
+ cmd_element->strvec != NULL)
+ {
+ cmd_element_v = cmd_element->strvec;
+
+ for (k = 0; k < vector_active (cmd_element_v); k++)
+ if ((desc_v = vector_slot (cmd_element_v, k)) != NULL)
+ {
+ for (l = 0; l < vector_active (desc_v); l++)
+ if ((desc = vector_slot (desc_v, l)) != NULL)
+ {
+ if (desc->cmd)
+ XFREE (MTYPE_STRVEC, desc->cmd);
+ if (desc->str)
+ XFREE (MTYPE_STRVEC, desc->str);
+
+ XFREE (MTYPE_DESC, desc);
+ }
+ vector_free (desc_v);
+ }
+
+ cmd_element->strvec = NULL;
+ vector_free (cmd_element_v);
+ }
+
+ vector_free (cmd_node_v);
+ }
+
+ vector_free (cmdvec);
+ cmdvec = NULL;
+ }
+
+ if (command_cr)
+ XFREE(MTYPE_STRVEC, command_cr);
+ if (desc_cr.str)
+ XFREE(MTYPE_STRVEC, desc_cr.str);
+ if (host.name)
+ XFREE (MTYPE_HOST, host.name);
+ if (host.password)
+ XFREE (MTYPE_HOST, host.password);
+ if (host.password_encrypt)
+ XFREE (MTYPE_HOST, host.password_encrypt);
+ if (host.enable)
+ XFREE (MTYPE_HOST, host.enable);
+ if (host.enable_encrypt)
+ XFREE (MTYPE_HOST, host.enable_encrypt);
+ if (host.logfile)
+ XFREE (MTYPE_HOST, host.logfile);
+ if (host.motdfile)
+ XFREE (MTYPE_HOST, host.motdfile);
+ if (host.config)
+ XFREE (MTYPE_HOST, host.config);
+}
diff --git a/lib/command.h b/lib/command.h
index d093df3..1275efe 100644
--- a/lib/command.h
+++ b/lib/command.h
@@ -147,8 +147,8 @@
/* Command description structure. */
struct desc
{
- const char *cmd; /* Command string. */
- const char *str; /* Command's description. */
+ char *cmd; /* Command string. */
+ char *str; /* Command's description. */
};
/* Return value of the commands. */
@@ -347,6 +347,7 @@
extern int cmd_execute_command_strict (vector, struct vty *, struct cmd_element **);
extern void config_replace_string (struct cmd_element *, char *, ...);
extern void cmd_init (int);
+extern void cmd_terminate (void);
/* Export typical functions. */
extern struct cmd_element config_end_cmd;
@@ -361,4 +362,7 @@
/* struct host global, ick */
extern struct host host;
+
+/* "<cr>" global */
+extern char *command_cr;
#endif /* _ZEBRA_COMMAND_H */
diff --git a/lib/log.c b/lib/log.c
index 8c3e2dd..0c2f655 100644
--- a/lib/log.c
+++ b/lib/log.c
@@ -649,7 +649,9 @@
closezlog (struct zlog *zl)
{
closelog();
- fclose (zl->fp);
+
+ if (zl->fp != NULL)
+ fclose (zl->fp);
XFREE (MTYPE_ZLOG, zl);
}
diff --git a/lib/memory.c b/lib/memory.c
index f5d0cba..dc09d8a 100644
--- a/lib/memory.c
+++ b/lib/memory.c
@@ -127,7 +127,7 @@
static struct
{
const char *name;
- unsigned long alloc;
+ long alloc;
unsigned long t_malloc;
unsigned long c_malloc;
unsigned long t_calloc;
@@ -214,9 +214,9 @@
static struct
{
char *name;
- unsigned long alloc;
+ long alloc;
} mstat [MTYPE_MAX];
-#endif /* MTPYE_LOG */
+#endif /* MEMORY_LOG */
/* Increment allocation counter. */
static void
@@ -253,6 +253,47 @@
}
}
+void
+log_memstats_stderr (const char *prefix)
+{
+ struct mlist *ml;
+ struct memory_list *m;
+ int i;
+ int j = 0;
+
+ for (ml = mlists; ml->list; ml++)
+ {
+ i = 0;
+
+ for (m = ml->list; m->index >= 0; m++)
+ if (m->index && mstat[m->index].alloc)
+ {
+ if (!i)
+ fprintf (stderr,
+ "%s: memstats: Current memory utilization in module %s:\n",
+ prefix,
+ ml->name);
+ fprintf (stderr,
+ "%s: memstats: %-30s: %10ld%s\n",
+ prefix,
+ m->format,
+ mstat[m->index].alloc,
+ mstat[m->index].alloc < 0 ? " (REPORT THIS BUG!)" : "");
+ i = j = 1;
+ }
+ }
+
+ if (j)
+ fprintf (stderr,
+ "%s: memstats: NOTE: If configuration exists, utilization may be "
+ "expected.\n",
+ prefix);
+ else
+ fprintf (stderr,
+ "%s: memstats: No remaining tracked memory utilization.\n",
+ prefix);
+}
+
static void
show_separator(struct vty *vty)
{
diff --git a/lib/memory.h b/lib/memory.h
index a23c278..42eb5ca 100644
--- a/lib/memory.h
+++ b/lib/memory.h
@@ -81,6 +81,7 @@
extern char *mtype_zstrdup (const char *file, int line, int type,
const char *str);
extern void memory_init (void);
+extern void log_memstats_stderr (const char *);
/* return number of allocations outstanding for the type */
extern unsigned long mtype_stats_alloc (int);
diff --git a/lib/routemap.c b/lib/routemap.c
index 5f7a318..4f4e6d6 100644
--- a/lib/routemap.c
+++ b/lib/routemap.c
@@ -889,6 +889,15 @@
route_match_vec = vector_init (1);
route_set_vec = vector_init (1);
}
+
+void
+route_map_finish (void)
+{
+ vector_free (route_match_vec);
+ route_match_vec = NULL;
+ vector_free (route_set_vec);
+ route_set_vec = NULL;
+}
/* VTY related functions. */
DEFUN (route_map,
diff --git a/lib/routemap.h b/lib/routemap.h
index 321e192..1402f5c 100644
--- a/lib/routemap.h
+++ b/lib/routemap.h
@@ -153,6 +153,7 @@
/* Prototypes. */
extern void route_map_init (void);
extern void route_map_init_vty (void);
+extern void route_map_finish (void);
/* Add match statement to route map. */
extern int route_map_add_match (struct route_map_index *index,
diff --git a/lib/thread.c b/lib/thread.c
index 47a9dc4..e89af54 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -239,6 +239,15 @@
return new;
}
+static void
+cpu_record_hash_free (void *a)
+{
+ struct cpu_thread_history *hist = a;
+
+ XFREE (MTYPE_THREAD_FUNCNAME, hist->funcname);
+ XFREE (MTYPE_THREAD_STATS, hist);
+}
+
static inline void
vty_out_cpu_thread_history(struct vty* vty,
struct cpu_thread_history *a)
@@ -485,7 +494,8 @@
for (t = list->head; t; t = next)
{
next = t->next;
- XFREE (MTYPE_THREAD_FUNCNAME, t->funcname);
+ if (t->funcname)
+ XFREE (MTYPE_THREAD_FUNCNAME, t->funcname);
XFREE (MTYPE_THREAD, t);
list->count--;
m->alloc--;
@@ -505,6 +515,13 @@
thread_list_free (m, &m->background);
XFREE (MTYPE_THREAD_MASTER, m);
+
+ if (cpu_record)
+ {
+ hash_clean (cpu_record, cpu_record_hash_free);
+ hash_free (cpu_record);
+ cpu_record = NULL;
+ }
}
/* Thread list is empty or not. */
@@ -836,6 +853,7 @@
{
*fetch = *thread;
thread->type = THREAD_UNUSED;
+ thread->funcname = NULL; /* thread_call will free fetch's copied pointer */
thread_add_unuse (m, thread);
return fetch;
}
@@ -1079,6 +1097,8 @@
realtime/1000, cputime/1000);
}
#endif /* CONSUMED_TIME_CHECK */
+
+ XFREE (MTYPE_THREAD_FUNCNAME, thread->funcname);
}
/* Execute thread */
diff --git a/lib/vty.c b/lib/vty.c
index 14a36c1..30a94e1 100644
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -1034,7 +1034,7 @@
if (desc->cmd[0] == '\0')
continue;
- if (strcmp (desc->cmd, "<cr>") == 0)
+ if (strcmp (desc->cmd, command_cr) == 0)
{
desc_cr = desc;
continue;
@@ -2988,3 +2988,17 @@
install_element (VTY_NODE, &no_vty_ipv6_access_class_cmd);
#endif /* HAVE_IPV6 */
}
+
+void
+vty_terminate (void)
+{
+ if (vty_cwd)
+ XFREE (MTYPE_TMP, vty_cwd);
+
+ if (vtyvec && Vvty_serv_thread)
+ {
+ vty_reset ();
+ vector_free (vtyvec);
+ vector_free (Vvty_serv_thread);
+ }
+}
diff --git a/lib/vty.h b/lib/vty.h
index 65ae620..7df04b5 100644
--- a/lib/vty.h
+++ b/lib/vty.h
@@ -203,6 +203,7 @@
/* Prototypes. */
extern void vty_init (struct thread_master *);
extern void vty_init_vtysh (void);
+extern void vty_terminate (void);
extern void vty_reset (void);
extern struct vty *vty_new (void);
extern int vty_out (struct vty *, const char *, ...) PRINTF_ATTRIBUTE(2, 3);
diff --git a/lib/zclient.c b/lib/zclient.c
index 4a716a6..d3d5322 100644
--- a/lib/zclient.c
+++ b/lib/zclient.c
@@ -58,13 +58,11 @@
return zclient;
}
-#if 0
-/* This function is never used. And it must not be used, because
+/* This function is only called when exiting, because
many parts of the code do not check for I/O errors, so they could
reference an invalid pointer if the structure was ever freed.
-*/
-/* Free zclient structure. */
+ Free zclient structure. */
void
zclient_free (struct zclient *zclient)
{
@@ -77,7 +75,6 @@
XFREE (MTYPE_ZCLIENT, zclient);
}
-#endif
/* Initialize zebra client. Argument redist_default is unwanted
redistribute route type. */
diff --git a/lib/zclient.h b/lib/zclient.h
index 69ada14..21786ab 100644
--- a/lib/zclient.h
+++ b/lib/zclient.h
@@ -125,6 +125,7 @@
extern int zclient_start (struct zclient *);
extern void zclient_stop (struct zclient *);
extern void zclient_reset (struct zclient *);
+extern void zclient_free (struct zclient *);
/* Get TCP socket connection to zebra daemon at loopback address. */
extern int zclient_socket (void);