From 228da42898c4f7bd72d9c1ee4135108e8d40d860 Mon Sep 17 00:00:00 2001 From: Chris Caputo Date: Sat, 18 Jul 2009 05:44:03 +0000 Subject: [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(). --- lib/command.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++------ lib/command.h | 8 +++-- lib/log.c | 4 ++- lib/memory.c | 47 +++++++++++++++++++++++++-- lib/memory.h | 1 + lib/routemap.c | 9 +++++ lib/routemap.h | 1 + lib/thread.c | 22 ++++++++++++- lib/vty.c | 16 ++++++++- lib/vty.h | 1 + lib/zclient.c | 7 ++-- lib/zclient.h | 1 + 12 files changed, 195 insertions(+), 23 deletions(-) (limited to 'lib') diff --git a/lib/command.c b/lib/command.c index 0bbd99e5..31c067a3 100644 --- a/lib/command.c +++ b/lib/command.c @@ -37,6 +37,9 @@ Boston, MA 02111-1307, USA. */ 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 @@ install_node (struct cmd_node *node, 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 @@ cmp_node (const void *p, const void *q) 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 @@ sort_node () 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 @@ install_element (enum node_type ntype, struct cmd_element *cmd) 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 @@ cmd_describe_command_real (vector vline, struct vty *vty, int *status) int ret; enum match_type match; char *command; - static struct desc desc_cr = { "", "" }; /* Set index. */ if (vector_active (vline) == 0) @@ -1665,7 +1669,6 @@ cmd_describe_command_real (vector vline, struct vty *vty, int *status) 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 @@ cmd_describe_command_real (vector vline, struct vty *vty, int *status) /* Check if command is completed. */ if (command == NULL && index == vector_active (strvec)) { - string = ""; - if (!desc_unique_string (matchvec, string)) + if (!desc_unique_string (matchvec, command_cr)) vector_set (matchvec, &desc_cr); } else @@ -1689,6 +1691,8 @@ cmd_describe_command_real (vector vline, struct vty *vty, int *status) 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 @@ DEFUN (no_banner_motd, void host_config_set (char *filename) { + if (host.config) + XFREE (MTYPE_HOST, host.config); host.config = XSTRDUP (MTYPE_HOST, filename); } @@ -3529,6 +3535,10 @@ install_default (enum node_type node) void cmd_init (int terminal) { + command_cr = XSTRDUP(MTYPE_STRVEC, ""); + 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 @@ cmd_init (int terminal) } 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 d093df3c..1275efee 100644 --- a/lib/command.h +++ b/lib/command.h @@ -147,8 +147,8 @@ struct cmd_element /* 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 (vector, struct vty *, struct cmd_element **, int 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 @@ extern void print_version (const char *); /* struct host global, ick */ extern struct host host; + +/* "" global */ +extern char *command_cr; #endif /* _ZEBRA_COMMAND_H */ diff --git a/lib/log.c b/lib/log.c index 8c3e2ddc..0c2f655b 100644 --- a/lib/log.c +++ b/lib/log.c @@ -649,7 +649,9 @@ void 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 f5d0cba6..dc09d8a6 100644 --- a/lib/memory.c +++ b/lib/memory.c @@ -127,7 +127,7 @@ zstrdup (int type, const char *str) 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 @@ mtype_zstrdup (const char *file, int line, int type, const char *str) 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 @@ log_memstats(int pri) } } +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 a23c2787..42eb5cae 100644 --- a/lib/memory.h +++ b/lib/memory.h @@ -81,6 +81,7 @@ extern void mtype_zfree (const char *file, int line, int type, 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 5f7a3182..4f4e6d62 100644 --- a/lib/routemap.c +++ b/lib/routemap.c @@ -889,6 +889,15 @@ route_map_init (void) 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 321e1927..1402f5c8 100644 --- a/lib/routemap.h +++ b/lib/routemap.h @@ -153,6 +153,7 @@ struct route_map /* 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 47a9dc43..e89af541 100644 --- a/lib/thread.c +++ b/lib/thread.c @@ -239,6 +239,15 @@ cpu_record_hash_alloc (struct cpu_thread_history *a) 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 @@ thread_list_free (struct thread_master *m, struct thread_list *list) 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_master_free (struct thread_master *m) 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 @@ thread_run (struct thread_master *m, struct thread *thread, { *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 @@ thread_call (struct thread *thread) 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 14a36c16..30a94e11 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -1034,7 +1034,7 @@ vty_describe_command (struct vty *vty) if (desc->cmd[0] == '\0') continue; - if (strcmp (desc->cmd, "") == 0) + if (strcmp (desc->cmd, command_cr) == 0) { desc_cr = desc; continue; @@ -2988,3 +2988,17 @@ vty_init (struct thread_master *master_thread) 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 65ae6201..7df04b5f 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -203,6 +203,7 @@ extern char integrate_default[]; /* 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 4a716a66..d3d53227 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -58,13 +58,11 @@ zclient_new () 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 @@ zclient_free (struct zclient *zclient) 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 69ada144..21786ab8 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -125,6 +125,7 @@ extern void zclient_init (struct zclient *, int); 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); -- cgit v1.2.1