Hello,
We have found the following bug in the procedure for dumping BGP routes in
MRT format (bgp_dump_routes_func in bgp_dump.c). If the RIB entry
corresponding to some prefix is large enough then bgpd crashes with SIGABRT.
The reason of such behavior is as follows. The function
bgp_dump_routes_func does not perform any size checks when writing data to
bgp_dump_obuf. For each bgp_node table record it tries to dump all data to
bgp_dump_obuf stream which is of limited size. If there is no free space in
this stream, the assertion fails and bgpd crashes.
The problem seems to be quite serious since it may occur if bgpd has many
BGP peers announcing the same prefix (e.g. if bgpd is used as BGP
reflector, on Internet Exchanges etc), and regular dumping is enabled. In
our case "many" was equal to 20.
The easiest way to reproduce the problem:
1) add 150 BGP neighbors announcing the same prefix
2) write "dump bgp routes-mrt bview.dat" command to the telnet console.
The easiest way to eliminate the problem is to create multiple MRT records
if there is too much data for a prefix. Please see the attached file
dump_fix.patch implementing such solution.
Finally, we have noticed a typo in the description of "dump bgp" command.
Please see the attached file comment_fix.patch.
--
| Evgeny Uskov | HLL l QRATOR
| mob.: +7 916 319 33 20
| skype: evgeny_uskov
| mailto: [email protected]
| visit: www.qrator.net
diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c
index 227fc7a..d5e9c1c 100644
--- a/bgpd/bgp_dump.c
+++ b/bgpd/bgp_dump.c
@@ -705,8 +705,8 @@ DEFUN (dump_bgp_all,
"dump bgp (all|all-et|updates|updates-et|routes-mrt) PATH [INTERVAL]",
"Dump packet\n"
"BGP packet dump\n"
- "Dump all BGP packets\nDump all BGP packets (Extended Tiemstamp Header)\n"
- "Dump BGP updates only\nDump BGP updates only (Extended Tiemstamp Header)\n"
+ "Dump all BGP packets\nDump all BGP packets (Extended Timestamp Header)\n"
+ "Dump BGP updates only\nDump BGP updates only (Extended Timestamp Header)\n"
"Dump whole BGP routing table\n"
"Output filename\n"
"Interval of output\n")
diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c
index 227fc7a..dd3bcaa 100644
--- a/bgpd/bgp_dump.c
+++ b/bgpd/bgp_dump.c
@@ -296,11 +296,96 @@ bgp_dump_routes_index_table(struct bgp *bgp)
}
+static struct bgp_info *
+bgp_dump_route_node_record (int afi, struct bgp_node *rn, struct bgp_info *info, unsigned int seq)
+{
+ struct stream *obuf;
+ size_t sizep;
+ size_t endp;
+
+ obuf = bgp_dump_obuf;
+ stream_reset(obuf);
+
+ /* MRT header */
+ if (afi == AFI_IP)
+ bgp_dump_header (obuf, MSG_TABLE_DUMP_V2, TABLE_DUMP_V2_RIB_IPV4_UNICAST,
+ BGP_DUMP_ROUTES);
+ else if (afi == AFI_IP6)
+ bgp_dump_header (obuf, MSG_TABLE_DUMP_V2, TABLE_DUMP_V2_RIB_IPV6_UNICAST,
+ BGP_DUMP_ROUTES);
+
+ /* Sequence number */
+ stream_putl(obuf, seq);
+
+ /* Prefix length */
+ stream_putc (obuf, rn->p.prefixlen);
+
+ /* Prefix */
+ if (afi == AFI_IP)
+ {
+ /* We'll dump only the useful bits (those not 0), but have to align on 8 bits */
+ stream_write(obuf, (u_char *)&rn->p.u.prefix4, (rn->p.prefixlen+7)/8);
+ }
+ else if (afi == AFI_IP6)
+ {
+ /* We'll dump only the useful bits (those not 0), but have to align on 8 bits */
+ stream_write (obuf, (u_char *)&rn->p.u.prefix6, (rn->p.prefixlen+7)/8);
+ }
+
+ /* Save where we are now, so we can overwride the entry count later */
+ sizep = stream_get_endp(obuf);
+
+ /* Entry count */
+ uint16_t entry_count = 0;
+
+ /* Entry count, note that this is overwritten later */
+ stream_putw(obuf, 0);
+
+ endp = stream_get_endp(obuf);
+ for (; info; info = info->next)
+ {
+ size_t cur_endp;
+
+ /* Peer index */
+ stream_putw(obuf, info->peer->table_dump_index);
+
+ /* Originated */
+#ifdef HAVE_CLOCK_MONOTONIC
+ stream_putl (obuf, time(NULL) - (bgp_clock() - info->uptime));
+#else
+ stream_putl (obuf, info->uptime);
+#endif /* HAVE_CLOCK_MONOTONIC */
+
+ /* Dump attribute. */
+ /* Skip prefix & AFI/SAFI for MP_NLRI */
+ bgp_dump_routes_attr (obuf, info->attr, &rn->p);
+
+ cur_endp = stream_get_endp(obuf);
+ if (cur_endp > BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER
+ + BGP_DUMP_HEADER_SIZE)
+ {
+ stream_set_endp(obuf, endp);
+ break;
+ }
+
+ entry_count++;
+ endp = cur_endp;
+ }
+
+ /* Overwrite the entry count, now that we know the right number */
+ stream_putw_at (obuf, sizep, entry_count);
+
+ bgp_dump_set_size(obuf, MSG_TABLE_DUMP_V2);
+ fwrite (STREAM_DATA (obuf), stream_get_endp (obuf), 1, bgp_dump_routes.fp);
+
+ return info;
+}
+
+
/* Runs under child process. */
static unsigned int
bgp_dump_routes_func (int afi, int first_run, unsigned int seq)
{
- struct stream *obuf;
struct bgp_info *info;
struct bgp_node *rn;
struct bgp *bgp;
@@ -319,81 +404,17 @@ bgp_dump_routes_func (int afi, int first_run, unsigned int seq)
if(first_run)
bgp_dump_routes_index_table(bgp);
- obuf = bgp_dump_obuf;
- stream_reset(obuf);
-
/* Walk down each BGP route. */
table = bgp->rib[afi][SAFI_UNICAST];
for (rn = bgp_table_top (table); rn; rn = bgp_route_next (rn))
{
- if(!rn->info)
- continue;
-
- stream_reset(obuf);
-
- /* MRT header */
- if (afi == AFI_IP)
- bgp_dump_header (obuf, MSG_TABLE_DUMP_V2, TABLE_DUMP_V2_RIB_IPV4_UNICAST,
- BGP_DUMP_ROUTES);
- else if (afi == AFI_IP6)
- bgp_dump_header (obuf, MSG_TABLE_DUMP_V2, TABLE_DUMP_V2_RIB_IPV6_UNICAST,
- BGP_DUMP_ROUTES);
-
- /* Sequence number */
- stream_putl(obuf, seq);
-
- /* Prefix length */
- stream_putc (obuf, rn->p.prefixlen);
-
- /* Prefix */
- if (afi == AFI_IP)
- {
- /* We'll dump only the useful bits (those not 0), but have to align on 8 bits */
- stream_write(obuf, (u_char *)&rn->p.u.prefix4, (rn->p.prefixlen+7)/8);
- }
- else if (afi == AFI_IP6)
- {
- /* We'll dump only the useful bits (those not 0), but have to align on 8 bits */
- stream_write (obuf, (u_char *)&rn->p.u.prefix6, (rn->p.prefixlen+7)/8);
- }
-
- /* Save where we are now, so we can overwride the entry count later */
- int sizep = stream_get_endp(obuf);
-
- /* Entry count */
- uint16_t entry_count = 0;
-
- /* Entry count, note that this is overwritten later */
- stream_putw(obuf, 0);
-
- for (info = rn->info; info; info = info->next)
- {
- entry_count++;
-
- /* Peer index */
- stream_putw(obuf, info->peer->table_dump_index);
-
- /* Originated */
-#ifdef HAVE_CLOCK_MONOTONIC
- stream_putl (obuf, time(NULL) - (bgp_clock() - info->uptime));
-#else
- stream_putl (obuf, info->uptime);
-#endif /* HAVE_CLOCK_MONOTONIC */
-
- /* Dump attribute. */
- /* Skip prefix & AFI/SAFI for MP_NLRI */
- bgp_dump_routes_attr (obuf, info->attr, &rn->p);
- }
-
- /* Overwrite the entry count, now that we know the right number */
- stream_putw_at (obuf, sizep, entry_count);
-
- seq++;
-
- bgp_dump_set_size(obuf, MSG_TABLE_DUMP_V2);
- fwrite (STREAM_DATA (obuf), stream_get_endp (obuf), 1, bgp_dump_routes.fp);
-
+ info = rn->info;
+ while (info)
+ {
+ info = bgp_dump_route_node_record(afi, rn, info, seq);
+ seq++;
+ }
}
fflush (bgp_dump_routes.fp);
@@ -840,8 +861,7 @@ bgp_dump_init (void)
memset (&bgp_dump_updates, 0, sizeof (struct bgp_dump));
memset (&bgp_dump_routes, 0, sizeof (struct bgp_dump));
- bgp_dump_obuf = stream_new (BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER
- + BGP_DUMP_HEADER_SIZE);
+ bgp_dump_obuf = stream_new (0x4000);
install_node (&bgp_dump_node, config_write_bgp_dump);
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
