NAK.
Crashes BGPd in my test case.

For my tests, I’ve applied the patch directly on top of quagga at git c99f3481a598, not the latest. (Latest code has more issues and makes it much more painful to make sure I can say for certain if the patch is good/bad). Patch applies cleanly on it.
Let me know if you think that this won’t/shouldn’t work.

Please let me know if you can’t reproduce and/or figure out the error and I’ll try
get you something to reproduce (or access to my testbed)

Crash detail:
Crash happens in the processing of the received BGP update:

2015/09/11 23:12:30 BGP: 192.168.1.1 rcvd UPDATE w/ attr: nexthop 192.168.1.1, origin i, path 501
2015/09/11 23:12:30 BGP: 192.168.1.1 rcvd 30.0.9.0/24
2015/09/11 23:12:30 ZEBRA: zebra message comes from socket [15]
2015/09/11 23:12:30 ZEBRA: zebra message received [ZEBRA_IPV4_NEXTHOP_LOOKUP] 4 in VRF 0 2015/09/11 23:12:30 ZEBRA: zread_ipv4_nexthop_lookup: looking up 192.168.1.1 2015/09/11 23:12:30 ZEBRA: zsend_ipv4_nexthop_lookup: Matching rib entry found. 2015/09/11 23:12:30 BGP: stream_get_ipv4: Attempt to get ipv4 out of bounds 2015/09/11 23:12:30 BGP: &(struct stream): 0x7f52b56eaba0, size: 4096, getp: 8, endp: 8

2015/09/11 23:12:30 BGP: Assertion `0' failed in file stream.c, line 486, function stream_get_ipv4
2015/09/11 23:12:30 BGP: Backtrace for 15 stack frames:
2015/09/11 23:12:30 BGP: [bt 0] /usr/local/test/lib/libzebra.so.0(zlog_backtrace+0x25) [0x7f52b3e4809b] 2015/09/11 23:12:30 BGP: [bt 1] /usr/local/test/lib/libzebra.so.0(_zlog_assert_failed+0xdc) [0x7f52b3e48ac0] 2015/09/11 23:12:30 BGP: [bt 2] /usr/local/test/lib/libzebra.so.0(stream_get_ipv4+0x15e) [0x7f52b3e43db5]
2015/09/11 23:12:30 BGP: [bt 3] bgpd(+0x6f8bc) [0x7f52b43178bc]
2015/09/11 23:12:30 BGP: [bt 4] bgpd(+0x6fb13) [0x7f52b4317b13]
2015/09/11 23:12:30 BGP: [bt 5] bgpd(+0x6e797) [0x7f52b4316797]
2015/09/11 23:12:30 BGP: [bt 6] bgpd(+0x3f543) [0x7f52b42e7543]
2015/09/11 23:12:30 BGP: [bt 7] bgpd(+0x3f7c2) [0x7f52b42e77c2]
2015/09/11 23:12:30 BGP: [bt 8] bgpd(+0x41bdf) [0x7f52b42e9bdf]
2015/09/11 23:12:30 BGP: [bt 9] bgpd(+0x62f71) [0x7f52b430af71]
2015/09/11 23:12:30 BGP: [bt 10] bgpd(+0x65314) [0x7f52b430d314]
2015/09/11 23:12:30 BGP: [bt 11] /usr/local/test/lib/libzebra.so.0(thread_call+0xc2) [0x7f52b3e3467e]
2015/09/11 23:12:30 BGP: [bt 12] bgpd(+0x1aee3) [0x7f52b42c2ee3]
2015/09/11 23:12:30 BGP: [bt 13] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7f52b375eec5]
2015/09/11 23:12:30 BGP: [bt 14] bgpd(+0x1a3d9) [0x7f52b42c23d9]
2015/09/11 23:12:30 BGP: Current thread function bgp_read, scheduled from file bgp_packet.c, line 2498

Looking at core dump:

Program terminated with signal SIGABRT, Aborted.

(gdb) bt
#0  0x00007f52b3773cc9 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007f52b37770d8 in __GI_abort () at abort.c:89
#2 0x00007f52b3e48acf in _zlog_assert_failed (assertion=0x7f52b3e63de8 "0",
    file=0x7f52b3e63c30 "stream.c", line=486,
function=0x7f52b3e64000 <__func__.8779> "stream_get_ipv4") at log.c:665 #3 0x00007f52b3e43db5 in stream_get_ipv4 (s=0x7f52b56eaba0) at stream.c:486
#4  0x00007f52b43178bc in zlookup_read () at bgp_nexthop.c:820
#5  0x00007f52b4317b13 in zlookup_query (addr=...) at bgp_nexthop.c:894
#6 0x00007f52b4316797 in bgp_nexthop_lookup (afi=1, peer=0x7f52b5720110, ri=0x7f52b573b090, changed=0x0, metricchanged=0x0) at bgp_nexthop.c:349
#7  0x00007f52b42e7543 in bgp_update_main (peer=0x7f52b5720110,
p=0x7ffd27c02060, attr=0x7ffd27c04170, afi=1, safi=1 '\001', type=9,
    sub_type=0, prd=0x0, tag=0x0, soft_reconfig=0) at bgp_route.c:2349
#8 0x00007f52b42e77c2 in bgp_update (peer=0x7f52b5720110, p=0x7ffd27c02060, attr=0x7ffd27c04170, afi=1, safi=1 '\001', type=9, sub_type=0, prd=0x0,
    tag=0x0, soft_reconfig=0) at bgp_route.c:2404
#9  0x00007f52b42e9bdf in bgp_nlri_parse (peer=0x7f52b5720110,
    attr=0x7ffd27c04170, packet=0x7ffd27c040f0) at bgp_route.c:3182
#10 0x00007f52b430af71 in bgp_update_receive (peer=0x7f52b5720110, size=26)
    at bgp_packet.c:1783
#11 0x00007f52b430d314 in bgp_read (thread=0x7ffd27c06420) at bgp_packet.c:2601 #12 0x00007f52b3e3467e in thread_call (thread=0x7ffd27c06420) at thread.c:1252 #13 0x00007f52b42c2ee3 in main (argc=2, argv=0x7ffd27c06588) at bgp_main.c:463



#3 0x00007f52b3e43db5 in stream_get_ipv4 (s=0x7f52b56eaba0) at stream.c:486
486           STREAM_BOUND_WARN (s, "get ipv4");


476     /* Get next long word from the stream. */
477     u_int32_t
478     stream_get_ipv4 (struct stream *s)
479     {
480       u_int32_t l;
481     
482       STREAM_VERIFY_SANE(s);
483     
484       if (STREAM_READABLE (s) < sizeof(u_int32_t))
485         {
486           STREAM_BOUND_WARN (s, "get ipv4");
487           return 0;
488         }
489     
490       memcpy (&l, s->data + s->getp, sizeof(u_int32_t));
491       s->getp += sizeof(u_int32_t);
492     
493       return l;
494     }

(gdb) p s
$1 = (struct stream *) 0x7f52b56eaba0
(gdb) p *s
$2 = {next = 0x0, getp = 8, endp = 8, size = 4096, data = 0x7f52b56eabd0 ""}


(gdb) up
#4  0x00007f52b43178bc in zlookup_read () at bgp_nexthop.c:820
820       raddr.s_addr = stream_get_ipv4 (s);

(gdb) list
812       if (version != ZSERV_VERSION || marker != ZEBRA_HEADER_MARKER)
813         {
814 zlog_err("%s: socket %d version mismatch, marker %d, version %d",
815                    __func__, zlookup->sock, marker, version);
816           return NULL;
817         }
818     
819       /* XXX: not doing anything with raddr */
820       raddr.s_addr = stream_get_ipv4 (s);
821       metric = stream_getl (s);
822       nexthop_num = stream_getc (s);
823     
824       if (nexthop_num)


(just removing the unused radar won’t help - it just crashes at the next line,
trying to read the metric)

Please be aware that I’m sending a VERY simple BGP Update with NO metric and
for a single 30.0.9.0/24 network

- Martin Winter
  [email protected]



On 10 Sep 2015, at 17:13, Martin Winter wrote:

Will test the v2 version (BGP tests for IPv4/IPv6 only, sorry no PIM tests yet)
Expect results in approx 48hrs.

- Martin

On 7 Sep 2015, at 6:56, Nicolas Dichtel wrote:

Commit c99f3481a598 has changed the API. Now, the vrfid has been added in the header, thus we must read it before parsing the rest of the message.

To ease code maintenance, let's add a new function to read a zAPI header.

Fixes: c99f3481a598 ("*: add VRF ID in the API message header")
Signed-off-by: Nicolas Dichtel <[email protected]>
---

Martin, could you test this patch? At least it fixes the ebgp multihop bug.

v2: add zclient_read_header()
fix also bgpd/zlookup_read_ipv6(), bgpd/bgp_import_check() and
pimd/zclient_read_nexthop()

bgpd/bgp_nexthop.c | 68 ++++++++++++++++++++++--------------------------------
lib/zclient.c      | 15 ++++++++++++
lib/zclient.h      |  3 +++
pimd/pim_zlookup.c | 51 +++++++++++++++++++---------------------
4 files changed, 69 insertions(+), 68 deletions(-)

diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c
index 7336793e01e6..c74bebad86e3 100644
--- a/bgpd/bgp_nexthop.c
+++ b/bgpd/bgp_nexthop.c
@@ -789,8 +789,9 @@ zlookup_read (void)
uint16_t length;
u_char marker;
u_char version;
-  uint16_t command __attribute__((unused));
-  int nbytes __attribute__((unused));
+  uint16_t vrf_id;
+  uint16_t command;
+  int err;
struct in_addr raddr __attribute__((unused));
uint32_t metric;
int i;
@@ -801,14 +802,13 @@ zlookup_read (void)
s = zlookup->ibuf;
stream_reset (s);

-  /* nbytes not being checked */
-  nbytes = stream_read (s, zlookup->sock, 2);
-  length = stream_getw (s);
-
-  nbytes = stream_read (s, zlookup->sock, length - 2);
-  marker = stream_getc (s);
-  version = stream_getc (s);
-
+ err = zclient_read_header (s, zlookup->sock, &length, &marker, &version,
+                             &vrf_id, &command);
+  if (err < 0)
+    {
+      zlog_err("%s: zserv_read_header() failed", __func__);
+      return NULL;
+    }
if (version != ZSERV_VERSION || marker != ZEBRA_HEADER_MARKER)
{
zlog_err("%s: socket %d version mismatch, marker %d, version %d",
@@ -816,9 +816,6 @@ zlookup_read (void)
return NULL;
}

-  /* XXX: not checking command */
-  command = stream_getw (s);
-
/* XXX: not doing anything with raddr */
raddr.s_addr = stream_get_ipv4 (s);
metric = stream_getl (s);
@@ -902,11 +899,11 @@ static struct bgp_nexthop_cache *
zlookup_read_ipv6 (void)
{
struct stream *s;
-  uint16_t length;
+  uint16_t length, vrf_id, cmd;
u_char version, marker;
struct in6_addr raddr;
uint32_t metric;
-  int i;
+  int i, err;
u_char nexthop_num;
struct nexthop *nexthop;
struct bgp_nexthop_cache *bnc;
@@ -914,14 +911,13 @@ zlookup_read_ipv6 (void)
s = zlookup->ibuf;
stream_reset (s);

-  /* XXX: ignoring nbytes, see also zread_lookup */
-  stream_read (s, zlookup->sock, 2);
-  length = stream_getw (s);
-
-  stream_read (s, zlookup->sock, length - 2);
-  marker = stream_getc (s);
-  version = stream_getc (s);
-
+ err = zclient_read_header (s, zlookup->sock, &length, &marker, &version,
+                             &vrf_id, &cmd);
+  if (err < 0)
+    {
+      zlog_err("%s: zserv_read_header() failed", __func__);
+      return NULL;
+    }
if (version != ZSERV_VERSION || marker != ZEBRA_HEADER_MARKER)
{
zlog_err("%s: socket %d version mismatch, marker %d, version %d",
@@ -929,9 +925,6 @@ zlookup_read_ipv6 (void)
return NULL;
}

-  /* XXX: ignoring command */
-  stream_getw (s);
-
/* XXX: not actually doing anything with raddr */
stream_get (&raddr, s, 16);

@@ -1018,9 +1011,8 @@ bgp_import_check (struct prefix *p, u_int32_t *igpmetric,
{
struct stream *s;
int ret;
-  u_int16_t length, command __attribute__((unused));
+  u_int16_t length, vrf_id, command;
u_char version, marker;
-  int nbytes __attribute__((unused));
struct in_addr addr __attribute__((unused));
struct in_addr nexthop;
u_int32_t metric = 0;
@@ -1066,16 +1058,13 @@ bgp_import_check (struct prefix *p, u_int32_t *igpmetric,
/* Get result. */
stream_reset (s);

-  /* Fetch length. */
-  /* XXX: not using nbytes */
-  nbytes = stream_read (s, zlookup->sock, 2);
-  length = stream_getw (s);
-
-  /* Fetch whole data. */
-  nbytes = stream_read (s, zlookup->sock, length - 2);
-  marker = stream_getc (s);
-  version = stream_getc (s);
-
+ ret = zclient_read_header (s, zlookup->sock, &length, &marker, &version,
+                             &vrf_id, &command);
+  if (ret < 0)
+    {
+      zlog_err("%s: zserv_read_header() failed", __func__);
+      return 0;
+    }
if (version != ZSERV_VERSION || marker != ZEBRA_HEADER_MARKER)
{
zlog_err("%s: socket %d version mismatch, marker %d, version %d",
@@ -1083,9 +1072,6 @@ bgp_import_check (struct prefix *p, u_int32_t *igpmetric,
return 0;
}

-  /* XXX: not using command */
-  command = stream_getw (s);
-
/* XXX: not using addr */
addr.s_addr = stream_get_ipv4 (s);
metric = stream_getl (s);
diff --git a/lib/zclient.c b/lib/zclient.c
index 8e443e28d26f..c1bdd8ff4f2d 100644
--- a/lib/zclient.c
+++ b/lib/zclient.c
@@ -302,6 +302,21 @@ zclient_create_header (struct stream *s, uint16_t command, vrf_id_t vrf_id)
stream_putw (s, command);
}

+int
+zclient_read_header (struct stream *s, int sock, u_int16_t *size, u_char *marker, + u_char *version, u_int16_t *vrf_id, u_int16_t *cmd)
+{
+  if (stream_read (s, sock, ZEBRA_HEADER_SIZE) != ZEBRA_HEADER_SIZE)
+    return -1;
+
+  *size = stream_getw (s) - ZEBRA_HEADER_SIZE;
+  *marker = stream_getc (s);
+  *version = stream_getc (s);
+  *vrf_id = stream_getw (s);
+  *cmd = stream_getw (s);
+  return 0;
+}
+
/* Send simple Zebra message. */
static int
zebra_message_send (struct zclient *zclient, int command, vrf_id_t vrf_id)
diff --git a/lib/zclient.h b/lib/zclient.h
index 19b4f0ead6cd..3490b32092b2 100644
--- a/lib/zclient.h
+++ b/lib/zclient.h
@@ -163,6 +163,9 @@ extern int zclient_send_message(struct zclient *);

/* create header for command, length to be filled in by user later */
extern void zclient_create_header (struct stream *, uint16_t, vrf_id_t); +extern int zclient_read_header (struct stream *s, int sock, u_int16_t *size,
+                               u_char *marker, u_char *version,
+                               u_int16_t *vrf_id, u_int16_t *cmd);

extern struct interface *zebra_interface_add_read (struct stream *,
vrf_id_t);
diff --git a/pimd/pim_zlookup.c b/pimd/pim_zlookup.c
index 60003670cf6d..fae8f81ca1b0 100644
--- a/pimd/pim_zlookup.c
+++ b/pimd/pim_zlookup.c
@@ -149,17 +149,18 @@ static int zclient_read_nexthop(struct zclient *zlookup,
{
int num_ifindex = 0;
struct stream *s;
- const uint16_t MIN_LEN = 14; /* getc=1 getc=1 getw=2 getipv4=4 getc=1 getl=4 getc=1 */
-  uint16_t length, len;
+  const uint16_t MIN_LEN = 10; /* getipv4=4 getc=1 getl=4 getc=1 */
+  uint16_t length;
u_char marker;
u_char version;
+  uint16_t vrf_id;
uint16_t command;
int nbytes;
struct in_addr raddr;
uint8_t distance;
uint32_t metric;
int nexthop_num;
-  int i;
+  int i, err;

if (PIM_DEBUG_ZEBRA) {
char addr_str[100];
@@ -172,41 +173,37 @@ static int zclient_read_nexthop(struct zclient *zlookup,
s = zlookup->ibuf;
stream_reset(s);

-  nbytes = stream_read(s, zlookup->sock, 2);
-  if (nbytes < 2) {
- zlog_err("%s %s: failure reading zclient lookup socket: nbytes=%d",
-            __FILE__, __PRETTY_FUNCTION__, nbytes);
+ err = zclient_read_header (s, zlookup->sock, &length, &marker, &version,
+                             &vrf_id, &command);
+  if (err < 0) {
+    zlog_err("%s %s: zclient_read_header() failed",
+            __FILE__, __PRETTY_FUNCTION__);
zclient_lookup_failed(zlookup);
return -1;
}
-  length = stream_getw(s);
-
-  len = length - 2;

-  if (len < MIN_LEN) {
+  if (length < MIN_LEN) {
zlog_err("%s %s: failure reading zclient lookup socket: len=%d < MIN_LEN=%d",
-            __FILE__, __PRETTY_FUNCTION__, len, MIN_LEN);
+            __FILE__, __PRETTY_FUNCTION__, length, MIN_LEN);
zclient_lookup_failed(zlookup);
return -2;
}
-
-  nbytes = stream_read(s, zlookup->sock, len);
-  if (nbytes < (length - 2)) {
+
+  nbytes = stream_read(s, zlookup->sock, length);
+  if (nbytes < length) {
zlog_err("%s %s: failure reading zclient lookup socket: nbytes=%d < len=%d",
-            __FILE__, __PRETTY_FUNCTION__, nbytes, len);
+            __FILE__, __PRETTY_FUNCTION__, nbytes, length);
zclient_lookup_failed(zlookup);
return -3;
}
-  marker = stream_getc(s);
-  version = stream_getc(s);

if (version != ZSERV_VERSION || marker != ZEBRA_HEADER_MARKER) {
zlog_err("%s: socket %d version mismatch, marker %d, version %d",
             __func__, zlookup->sock, marker, version);
+    zclient_lookup_failed(zlookup);
return -4;
}

-  command = stream_getw(s);
if (command != ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB) {
zlog_err("%s: socket %d command mismatch: %d",
      __func__, zlookup->sock, command);
@@ -236,19 +233,19 @@ static int zclient_read_nexthop(struct zclient *zlookup,
return -6;
}

-  len -= MIN_LEN;
+  length -= MIN_LEN;

for (i = 0; i < nexthop_num; ++i) {
enum nexthop_types_t nexthop_type;

-    if (len < 1) {
+    if (length < 1) {
zlog_err("%s: socket %d empty input expecting nexthop_type: len=%d",
-              __func__, zlookup->sock, len);
+              __func__, zlookup->sock, length);
return -7;
}

nexthop_type = stream_getc(s);
-    --len;
+    --length;

switch (nexthop_type) {
case ZEBRA_NEXTHOP_IFINDEX:
@@ -263,13 +260,13 @@ static int zclient_read_nexthop(struct zclient *zlookup,
        return num_ifindex;
}
if (nexthop_type == ZEBRA_NEXTHOP_IPV4_IFINDEX) {
-       if (len < 4) {
+       if (length < 4) {
zlog_err("%s: socket %d short input expecting nexthop IPv4-addr: len=%d",
-                  __func__, zlookup->sock, len);
+                  __func__, zlookup->sock, length);
          return -8;
        }
        nexthop_tab[num_ifindex].nexthop_addr.s_addr = stream_get_ipv4(s);
-       len -= 4;
+       length -= 4;
}
else {
        nexthop_tab[num_ifindex].nexthop_addr.s_addr = PIM_NET_INADDR_ANY;
@@ -289,7 +286,7 @@ static int zclient_read_nexthop(struct zclient *zlookup,
        return num_ifindex;
}
nexthop_tab[num_ifindex].nexthop_addr.s_addr = stream_get_ipv4(s);
-      len -= 4;
+      length -= 4;
nexthop_tab[num_ifindex].ifindex             = 0;
nexthop_tab[num_ifindex].protocol_distance   = distance;
nexthop_tab[num_ifindex].route_metric        = metric;
--
2.4.2

_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev

Reply via email to