Paul, Please see the attached files created with "git format-patch".
Regarding the chunk with bgp_dump_obuf creation, it was done intentionally with the following purpose. The key idea of the patch is that if the portion of data (i.e. the RIB entry corresponding to the prefix) is greater than the remaining space in the current MRT record, then we finalize the current record and put this portion of data to the next record. The function writing the portion of data (bgp_dump_routes_attr) does not perform size checking, and hence we decided to implement the logic above in the following way. 1) We increased the max size of bgp_dump_obuf; 2) after each portion of data we compare the current data size with BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE. If the data size is greater than this value, then we finalize the record with all the RIB entries except this last one (which does not fit to max packet size), and the last RIB entry goes to the next data portion. The size of bgp_dump_obuf in this case should be at least BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE + (max size of the RIB entry). In the patches attached to this letter we removed the magic constant 0x4000, set the size in the following way: - bgp_dump_obuf = stream_new (BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER - + BGP_DUMP_HEADER_SIZE); + bgp_dump_obuf = stream_new ((BGP_MAX_PACKET_SIZE << 1) + + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE); I.e. we added one extra BGP_MAX_PACKET_SIZE which should be enough to contain any possible RIB entry. -- | Evgeny Uskov | HLL l QRATOR | mob.: +7 916 319 33 20 | skype: evgeny_uskov | mailto: [email protected] | visit: www.qrator.net On Mon, Jan 25, 2016 at 4:16 PM, Paul Jakma <[email protected]> wrote: > Hi, > > On Mon, 25 Jan 2016, Evgeny Uskov wrote: > > 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. >> > > Nice. Thanks! > > One thing, could you supply it as a git commit, or otherwise just supply a > commit message. > > Also, did you intend to submit this chunk? If yes, what's the purpose? > > - bgp_dump_obuf = stream_new (BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER > - + BGP_DUMP_HEADER_SIZE); > + bgp_dump_obuf = stream_new (0x4000); > > regards, > -- > Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A > Fortune: > Our country has plenty of good five-cent cigars, but the trouble is > they charge fifteen cents for them. >
From cb331797f82407876b279ebaa34d989fb5752b19 Mon Sep 17 00:00:00 2001 From: Evgeny Uskov <[email protected]> Date: Mon, 25 Jan 2016 17:06:38 +0300 Subject: [PATCH] bgpd: Fix description of the command "dump bgp ..." --- bgpd/bgp_dump.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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") -- 2.1.1
From e26b44d79aba373a3f911c18c63b5deef28dd47a Mon Sep 17 00:00:00 2001 From: Evgeny Uskov <[email protected]> Date: Wed, 13 Jan 2016 13:58:00 +0300 Subject: [PATCH] bgpd: Fix buffer overflow error in bgp_dump_routes_func Now if the number of entries for some prefix is too large, multiple TABLE_DUMP_V2 records are created. In the previous version in such situation bgpd crashed with SIGABRT. --- bgpd/bgp_dump.c | 167 +++++++++++++++++++++++++++++++------------------------- 1 file changed, 94 insertions(+), 73 deletions(-) diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c index 227fc7a..af5bf87 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,8 @@ 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 ((BGP_MAX_PACKET_SIZE << 1) + + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE); install_node (&bgp_dump_node, config_write_bgp_dump); -- 2.1.1
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
