| Age | Commit message (Collapse) | Author |
|---|
|
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().
|
|
|
|
Use Ansi-C prototypes rather than old K&R method of declaring
function without arguments
|
|
* bgpd/bgp_community.c: (community_del_val) Fix bug in memcpy that was
using the wrong size on architectures where a pointer is not 32 bits.
|
|
* bgpd/bgp_aspath.c: (aspath_make_str_count) "assert (len < str_size)" was
getting hit under certain 4-byte ASN conditions. New realloc strategy.
* bgpd/bgp_aspath.c: (aspath_key_make) const warning fix.
"%d" -> "%u" 4-byte ASN corrections. Prevent negative number when ASN is
above 2^31.
|
|
2008-08-14 Stephen Hemminger <stephen.hemminger@vyatta.com>
* lib/hash.h: (struct hash) Hash comparator callback really
ought to treat storage behind arguments as constant - a compare
function with side-effects would be evil.
* */*.c: Adjust comparator functions similarly, thus fixing at least
a few compiler warnings about const qualifier being dropped.
Signed-off-by: Paul Jakma <paul@quagga.net>
|
|
2008-07-22 Paul Jakma <paul.jakma@sun.com>
* bgp_community.c: (community_str2com) assigns defaults to local
vars
* bgp_attr.c: (bgp_attr_parse) match format specifier to arg
* bgp_table.{c,h}: (bgp_table_top) can take a * to a const, quelling
warning in bgp_route.c
|
|
2007-09-07 Paul Jakma <paul.jakma@sun.com>
* (general) bgpd can be made crash by remote peers if debug
bgp updates is set, due to NULL pointer dereference.
Reported by "Mu Security Research Team",
<security@musecurity.com>.
* bgp_attr.c: (bgp_attr_community) If community length is 0,
don't set the community-present attribute bit, just return
early.
* bgp_debug.c: (community_str,community_com2str) Check com
pointer before dereferencing.
|
|
2006-05-08 Paul Jakma <paul.jakma@sun.com>
* bgp_community.c: (community_str2com) Coverity CID#62, fix
double-free, use-after-free.
|
|
2006-03-30 Paul Jakma <paul.jakma@sun.com>
* bgp_community.c: (community_gettoken) Unknown token should
return NULL, to give a strong indication to callers that
the token no longer can be parsed, otherwise callers looping
on this function may have a hard time ending their loop.
(community_str2com) While loop around community_gettoken appears
to have been coded thinking that break statement would break
from the while{}, hence it could never exit for unknown token
case. Fix it to do..while, so it can use the NULL result from
community_gettoken easily.
|
|
2006-01-19 Paul Jakma <paul.jakma@sun.com>
* (general) various miscellaneous compiler warning fixes.
Remove redundant break statements from switch clauses
which return.
return from main, not exit, cause it annoys SOS.
Remove stray semi-colons which cause empty-statement
warnings.
* zebra/main.c: (sighup) remove private declaration of external
function.
|
|
* (global) The great bgpd extern and static'ification.
* bgp_routemap.c: remove unused ROUTE_MATCH_ASPATH_OLD code
(route_set_metric_compile) fix u_int32_t to ULONG_MAX comparison
warnings.
* bgp_route.h: (bgp_process, bgp_withdraw, bgp_update) export these
used by various files which had their own private declarations,
in the case of mplsvpn - incorrect.
|
|
* (global) more const'ification and fixups of types to clean up code.
* bgp_mplsvpn.{c,h}: (str2tag) fix abuse. Still not perfect,
should use something like the VTY_GET_INTEGER macro, but without
the vty_out bits..
* bgp_routemap.c: (set_aggregator_as) use VTY_GET_INTEGER_RANGE
(no_set_aggregator_as) ditto.
* bgpd.c: (peer_uptime) fix unlikely bug, where no buffer is
returned, add comments about troublesome return value.
|
|
* type mismatch fixes
|
|
|
