bgpd: Regularise BGP NLRI sanity checks a bit

* bgp_route.h: (bgp_nlri_sanity_check) The bulk of the args are equivalent
  to a (struct bgp_nlri), consolidate.
* bgp_route.c: (bgp_nlri_sanity_check) Make this a frontend for all afi/safis.
  Including SAFI_MPLS_LABELED_VPN.
  (bgp_nlri_sanity_check_ip) Regular IP NLRI sanity check based on the
  existing code, and adjusted for (struct bgp_nlri *) arg.
* bgp_attr.c: (bgp_mp_reach_parse) Adjust for passing (struct bgp_nlri *)
  to bgp_nlri_sanity_check.
  Get rid of special-casing to not sanity check VPN.
  (bgp_mp_unreach_parse) Ditto.

* bgp_mplsvpn.c: Use the same VPN parsing code for both the sanity
  check and the actual parse.

  (bgp_nlri_parse_vpn) renamed to bgp_nlri_parse_vpn_body and made
  internal.

  (bgp_nlri_parse_vpn_body) Added (bool) argument to control whether it
  is sanity checking or whether it should update routing state for each
  NLRI.  Send a NOTIFY and reset the session, if there's a parsing
  error, as bgp_nlri_sanity_check_ip does, and as is required by the
  RFC.

  (bgp_nlri_parse_vpn) now a wrapper to call _body with update.

  (bgp_nlri_sanity_check_vpn) wrapper to call parser without
  updating.

* bgp_mplsvpn.h: (bgp_nlri_sanity_check_vpn) export for
  bgp_nlri_sanity_check.

* bgp_packet.c: (bgp_update_receive) Adjust for bgp_nlri_sanity_check
  argument changes.

* test/bgp_mp_attr_test.c: Extend to also test the NLRI parsing functions,
  if the initial MP-attr parsing has succeeded.  Fix the NLRI in the
  VPN cases.  Add further VPN tests.

* tests/bgpd.tests/testbgpmpattr.exp: Add the new test cases.

This commit a joint effort of:

Lou Berger <lberger@labn.net>
Donald Sharp <sharpd@cumulusnetworks.com>
Paul Jakma <paul.jakma@hpe.com> / <paul@jakma.org>

8 files changed, 390 insertions, 101 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 888f11a1..98571dab 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1725,23 +1725,20 @@ bgp_mp_reach_parse (struct bgp_attr_parser_args *args,
                  __func__, peer->host);
       return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
     }
- 
-  if (safi != SAFI_MPLS_LABELED_VPN)
-    {
-      ret = bgp_nlri_sanity_check (peer, afi, safi, stream_pnt (s), nlri_len);
-      if (ret < 0) 
-        {
-          zlog_info ("%s: (%s) NLRI doesn't pass sanity check",
-                     __func__, peer->host);
-	  return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
-	}
-    }
-
+  
   mp_update->afi = afi;
   mp_update->safi = safi;
   mp_update->nlri = stream_pnt (s);
   mp_update->length = nlri_len;
 
+  ret = bgp_nlri_sanity_check (peer, mp_update);
+  if (ret < 0) 
+    {
+      zlog_info ("%s: (%s) NLRI doesn't pass sanity check",
+                 __func__, peer->host);
+      return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
+    }
+
   stream_forward_getp (s, nlri_len);
 
   attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MP_REACH_NLRI);
@@ -1759,7 +1756,6 @@ bgp_mp_unreach_parse (struct bgp_attr_parser_args *args,
   afi_t afi;
   safi_t safi;
   u_int16_t withdraw_len;
-  int ret;
   struct peer *const peer = args->peer;  
   struct attr *const attr = args->attr;
   const bgp_size_t length = args->length;
@@ -1775,18 +1771,14 @@ bgp_mp_unreach_parse (struct bgp_attr_parser_args *args,
   
   withdraw_len = length - BGP_MP_UNREACH_MIN_SIZE;
 
-  if (safi != SAFI_MPLS_LABELED_VPN)
-    {
-      ret = bgp_nlri_sanity_check (peer, afi, safi, stream_pnt (s), withdraw_len);
-      if (ret < 0)
-	return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
-    }
-
   mp_withdraw->afi = afi;
   mp_withdraw->safi = safi;
   mp_withdraw->nlri = stream_pnt (s);
   mp_withdraw->length = withdraw_len;
 
+  if (bgp_nlri_sanity_check (peer, mp_withdraw) < 0)
+    return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
+
   stream_forward_getp (s, withdraw_len);
 
   attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MP_UNREACH_NLRI);
diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c
index f8b43df8..900bc487 100644
--- a/bgpd/bgp_mplsvpn.c
+++ b/bgpd/bgp_mplsvpn.c
@@ -32,6 +32,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 #include "bgpd/bgp_route.h"
 #include "bgpd/bgp_attr.h"
 #include "bgpd/bgp_mplsvpn.h"
+#include "bgpd/bgp_packet.h"
 
 static u_int16_t
 decode_rd_type (u_char *pnt)
@@ -91,9 +92,9 @@ decode_rd_ip (u_char *pnt, struct rd_ip *rd_ip)
   rd_ip->val |= (u_int16_t) *pnt;
 }
 
-int
-bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr,
-                    struct bgp_nlri *packet)
+static int
+bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr, 
+                         struct bgp_nlri *packet, bool update)
 {
   u_char *pnt;
   u_char *lim;
@@ -129,30 +130,53 @@ bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr,
       psize = PSIZE (prefixlen);
       
       /* sanity check against packet data */
-      if (prefixlen < VPN_PREFIXLEN_MIN_BYTES*8 || (pnt + psize) > lim)
+      if (prefixlen < VPN_PREFIXLEN_MIN_BYTES*8)
+        {
+          plog_err (peer->log, 
+                    "%s [Error] Update packet error / VPNv4"
+                     " (prefix length %d less than VPNv4 min length)",
+                    peer->host, prefixlen);
+          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
+                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
+          return -1;
+        }
+      if ((pnt + psize) > lim)
         {
-          zlog_err ("prefix length (%d) is less than 88"
-                    " or larger than received (%u)",
+          plog_err (peer->log,
+                    "%s [Error] Update packet error / VPNv4"
+                    " (psize %u exceeds packet size (%u)",
+                    peer->host, 
                     prefixlen, (uint)(lim-pnt));
+          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
+                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
         }
       
       /* sanity check against storage for the IP address portion */
       if ((psize - VPN_PREFIXLEN_MIN_BYTES) > (ssize_t) sizeof(p.u))
         {
-          zlog_err ("prefix length (%d) exceeds prefix storage (%zu)",
+          plog_err (peer->log,
+                    "%s [Error] Update packet error / VPNv4"
+                    " (psize %u exceeds storage size (%zu)",
+                    peer->host,
                     prefixlen - VPN_PREFIXLEN_MIN_BYTES*8, sizeof(p.u));
+          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
+                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
         }
       
       /* Sanity check against max bitlen of the address family */
       if ((psize - VPN_PREFIXLEN_MIN_BYTES) > prefix_blen (&p))
         {
-          zlog_err ("prefix length (%d) exceeds family (%u) max byte length (%u)",
+          plog_err (peer->log,
+                    "%s [Error] Update packet error / VPNv4"
+                    " (psize %u exceeds family (%u) max byte len %u)",
+                    peer->host,
                     prefixlen - VPN_PREFIXLEN_MIN_BYTES*8, 
                     p.family, prefix_blen (&p));
+          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
+                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
-                  
         }
       
       /* Copyr label to prefix. */
@@ -187,22 +211,46 @@ bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr,
       memcpy (&p.u.prefix, pnt + VPN_PREFIXLEN_MIN_BYTES, 
               psize - VPN_PREFIXLEN_MIN_BYTES);
 
-      if (attr)
-        bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
-                    ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0);
-      else
-        bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
-                      ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt);
+      if (update)
+        {
+          if (attr)
+            bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
+                        ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0);
+          else
+            bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
+                          ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt);
+        }
     }
   /* Packet length consistency check. */
   if (pnt != lim)
-    return -1;
+    {
+      plog_err (peer->log,
+                "%s [Error] Update packet error / VPNv4"
+                " (%zu data remaining after parsing)",
+                peer->host, lim - pnt);
+      bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
+                       BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
+      return -1;
+    }
   
   return 0;
 #undef VPN_PREFIXLEN_MIN_BYTES
 }
 
 int
+bgp_nlri_sanity_check_vpn (struct peer *peer, struct bgp_nlri *nlri)
+{
+  return bgp_nlri_parse_vpn_body (peer, NULL, nlri, false);
+}
+
+int
+bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr, 
+                    struct bgp_nlri *packet)
+{
+  return bgp_nlri_parse_vpn_body (peer, attr, packet, true);
+}
+
+int
 str2prefix_rd (const char *str, struct prefix_rd *prd)
 {
   int ret; /* ret of called functions */
diff --git a/bgpd/bgp_mplsvpn.h b/bgpd/bgp_mplsvpn.h
index 3299b9cb..b89b6d8a 100644
--- a/bgpd/bgp_mplsvpn.h
+++ b/bgpd/bgp_mplsvpn.h
@@ -43,6 +43,7 @@ struct rd_ip
 
 extern void bgp_mplsvpn_init (void);
 extern int bgp_nlri_parse_vpn (struct peer *, struct attr *, struct bgp_nlri *);
+extern int bgp_nlri_sanity_check_vpn (struct peer *, struct bgp_nlri *);
 extern u_int32_t decode_label (u_char *);
 extern int str2prefix_rd (const char *, struct prefix_rd *);
 extern int str2tag (const char *, u_char *);
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index b8a38fa5..d40c24c2 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -1660,17 +1660,17 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
   /* Unfeasible Route packet format check. */
   if (withdraw_len > 0)
     {
-      ret = bgp_nlri_sanity_check (peer, AFI_IP, SAFI_UNICAST, stream_pnt (s), withdraw_len);
+      withdraw.afi = AFI_IP;
+      withdraw.safi = SAFI_UNICAST;
+      withdraw.nlri = stream_pnt (s);
+      withdraw.length = withdraw_len;
+      ret = bgp_nlri_sanity_check (peer, &withdraw);
       if (ret < 0)
 	return -1;
 
       if (BGP_DEBUG (packet, PACKET_RECV))
 	zlog_debug ("%s [Update:RECV] Unfeasible NLRI received", peer->host);
 
-      withdraw.afi = AFI_IP;
-      withdraw.safi = SAFI_UNICAST;
-      withdraw.nlri = stream_pnt (s);
-      withdraw.length = withdraw_len;
       stream_forward_getp (s, withdraw_len);
     }
   
@@ -1751,8 +1751,14 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
 
   if (update_len)
     {
+      /* Set NLRI portion to structure. */
+      update.afi = AFI_IP;
+      update.safi = SAFI_UNICAST;
+      update.nlri = stream_pnt (s);
+      update.length = update_len;
+      
       /* Check NLRI packet format and prefix length. */
-      ret = bgp_nlri_sanity_check (peer, AFI_IP, SAFI_UNICAST, stream_pnt (s), update_len);
+      ret = bgp_nlri_sanity_check (peer, &update);
       if (ret < 0)
         {
           bgp_attr_unintern_sub (&attr);
@@ -1760,11 +1766,6 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
 	  return -1;
 	}
 
-      /* Set NLRI portion to structure. */
-      update.afi = AFI_IP;
-      update.safi = SAFI_UNICAST;
-      update.nlri = stream_pnt (s);
-      update.length = update_len;
       stream_forward_getp (s, update_len);
     }
 
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 0dcae61e..c1e5e59b 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -3322,16 +3322,16 @@ bgp_nlri_parse (struct peer *peer, struct attr *attr, struct bgp_nlri *packet)
   return 0;
 }
 
-/* NLRI encode syntax check routine. */
-int
-bgp_nlri_sanity_check (struct peer *peer, int afi, safi_t safi,
-                       u_char *pnt, bgp_size_t length)
+static int
+bgp_nlri_sanity_check_ip (struct peer *peer, struct bgp_nlri *nlri)
 {
   u_char *end;
   u_char prefixlen;
   int psize;
-
-  end = pnt + length;
+  u_char *pnt = nlri->nlri;
+  afi_t afi = nlri->afi;
+  safi_t safi = nlri->safi;
+  end = pnt + nlri->length;
 
   /* RFC1771 6.3 The NLRI field in the UPDATE message is checked for
      syntactic validity.  If the field is syntactically incorrect,
@@ -3395,6 +3395,20 @@ bgp_nlri_sanity_check (struct peer *peer, int afi, safi_t safi,
   return 0;
 }
 
+int
+bgp_nlri_sanity_check (struct peer *peer, struct bgp_nlri *nlri)
+{
+   switch (nlri->safi)
+     {
+       case SAFI_MPLS_LABELED_VPN:
+         return bgp_nlri_sanity_check_vpn (peer, nlri);
+       case SAFI_UNICAST:
+       case SAFI_MULTICAST:
+         return bgp_nlri_sanity_check_ip (peer, nlri);
+       default: return -1;
+     }
+}
+
 static struct bgp_static *
 bgp_static_new (void)
 {
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index 8e9bcbd1..c16eca7b 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -196,7 +196,7 @@ extern struct bgp_info_extra *bgp_info_extra_get (struct bgp_info *);
 extern void bgp_info_set_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
 extern void bgp_info_unset_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
 
-extern int bgp_nlri_sanity_check (struct peer *, int, safi_t, u_char *, bgp_size_t);
+extern int bgp_nlri_sanity_check (struct peer *, struct bgp_nlri *);
 extern int bgp_nlri_parse (struct peer *, struct attr *, struct bgp_nlri *);
 
 extern int bgp_maximum_prefix_overflow (struct peer *, afi_t, safi_t, int);
diff --git a/tests/bgp_mp_attr_test.c b/tests/bgp_mp_attr_test.c
index 6dc6757c..39f3b0da 100644
--- a/tests/bgp_mp_attr_test.c
+++ b/tests/bgp_mp_attr_test.c
@@ -31,6 +31,9 @@
 #include "bgpd/bgp_attr.h"
 #include "bgpd/bgp_open.h"
 #include "bgpd/bgp_debug.h"
+#include "bgpd/bgp_route.h"
+#include "bgpd/bgp_mplsvpn.h"
+#include "bgpd/bgp_nexthop.h"
 
 #define VT100_RESET "\x1b[0m"
 #define VT100_RED "\x1b[31m"
@@ -308,8 +311,34 @@ static struct test_segment {
     SHOULD_ERR,
     AFI_IP, SAFI_UNICAST, VALID_AFI,
   },
-  { "IPv4-MLVPN",
-    "IPv4/MPLS-labeled VPN MP Reach, RD, Nexthop, 3 NLRIs", 
+  { "IPv4-VPNv4",
+    "IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 16,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3)),
+    SHOULD_PARSE,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+  { "IPv4-VPNv4-bogus-plen",
+    "IPv4/MPLS-labeled VPN MP Reach, RD, Nexthop, NLRI / bogus p'len", 
     {
       /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
       /* nexthop bytes */	12,
@@ -321,10 +350,169 @@ static struct test_segment {
                                 17, 10, 2, 3,  /* 10.2.3/17 */
                                 0, /* 0/0 */
     },
-    (4 + 12 + 1 + 3 + 4 + 1),
+    (3 + 1 + 3*4 + 1 + 3 + 4 + 1),
+    SHOULD_ERR,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+  { "IPv4-VPNv4-plen1-short",
+    "IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs, 1st plen short", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 1,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3)),
+    SHOULD_ERR,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+  { "IPv4-VPNv4-plen1-long",
+    "IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs, 1st plen long", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 32,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3)),
+    SHOULD_ERR,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+  { "IPv4-VPNv4-plenn-long",
+    "IPv4/VPNv4 MP Reach, RD, Nexthop, 3 NLRIs, last plen long", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 16,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+                                88 + 1, /* bogus */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3) + 1),
+    SHOULD_ERR,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+  { "IPv4-VPNv4-plenn-short",
+    "IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs, last plen short", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 16,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 2,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3)),
+    SHOULD_ERR,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+  { "IPv4-VPNv4-bogus-rd-type",
+    "IPv4/VPNv4 MP Reach, RD, NH, 2 NLRI, unknown RD in 1st (log, but parse)", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 16,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0xff, 0, /* Bogus RD */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3)),
     SHOULD_PARSE,
-    AFI_IP, SAFI_UNICAST, VALID_AFI,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
   },
+  { "IPv4-VPNv4-0-nlri",
+    "IPv4/VPNv4 MP Reach, RD, Nexthop, 3 NLRI, 3rd 0 bogus", 
+    {
+      /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
+      /* nexthop bytes */	12,
+      /* RD */			0, 0, 0, 0, /* RD defined to be 0 */
+                                0, 0, 0, 0,
+      /* Nexthop */		192, 168,   0,  1, 
+      /* SNPA (defunct, MBZ) */	0x0,
+      /* NLRI tuples */		88 + 16,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
+                                0 /* 0/0, bogus for vpnv4 ?? */
+    },
+    (4 + 12 + 1 + (1+3+8+2) + (1+3+8+3) + 1),
+    SHOULD_ERR,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
+  },
+
   /* From bug #385 */
   { "IPv6-bug",
     "IPv6, global nexthop, 1 default NLRI", 
@@ -431,33 +619,79 @@ static struct test_segment mp_unreach_segments [] =
     SHOULD_ERR,
     AFI_IP, SAFI_UNICAST, VALID_AFI,
   },
-  { "IPv4-unreach-MLVPN",
+  { "IPv4-unreach-VPNv4",
     "IPv4/MPLS-labeled VPN MP Unreach, RD, 3 NLRIs", 
     {
       /* AFI / SAFI */		0x0, AFI_IP, SAFI_MPLS_LABELED_VPN,
-      /* nexthop bytes */	12,
-      /* RD */			0, 0, 1, 2,
-                                0, 0xff, 3, 4,
-      /* Nexthop */		192, 168,   0,  1, 
-      /* SNPA (defunct, MBZ) */	0x0,
-      /* NLRI tuples */		16, 10, 1,    /* 10.1/16 */
-                                17, 10, 2, 3,  /* 10.2.3/17 */
-                                0, /* 0/0 */
+      /* NLRI tuples */		88 + 16,
+                                  0, 1, 2,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_AS */
+                                    0, 2, 0, 0xff, 3, 4, /* AS(2):val(4) */
+                                  10, 1,    /* 10.1/16 */
+                                88 + 17,
+                                  0xff, 0, 0,   /* tag */
+                                  /* rd, 8 octets */
+                                    0, 0, /* RD_TYPE_IP */
+                                    192, 168, 0, 1, /* IPv4 */
+                                  10, 2, 3,  /* 10.2.3/17 */
     },
-    (3 + 3 + 4 + 1),
+    (3 + (1+3+8+2) + (1+3+8+3)),
     SHOULD_PARSE,
-    AFI_IP, SAFI_UNICAST, VALID_AFI,
+    AFI_IP, SAFI_MPLS_LABELED_VPN, VALID_AFI,
   },
   { NULL, NULL, {0}, 0, 0}
 };
 
+/* nlri_parse indicates 0 on successful parse, and -1 otherwise.
+ * attr_parse indicates BGP_ATTR_PARSE_PROCEED/0 on success,
+ * and BGP_ATTR_PARSE_ERROR/-1 or lower negative ret on err.
+ */
+static void
+handle_result (struct peer *peer, struct test_segment *t,
+               int parse_ret, int nlri_ret)
+{
+  int oldfailed = failed;
+  
+  if (!parse_ret)
+    {
+      safi_t safi = t->safi;
+      
+      if (bgp_afi_safi_valid_indices (t->afi, &safi) != t->afi_valid)
+        failed++;
+      
+      printf ("MP: %u/%u (%u): recv %u, nego %u\n",
+              t->afi, t->safi, safi,
+              peer->afc_recv[t->afi][safi],
+              peer->afc_nego[t->afi][safi]);
+    }
+  
+  printf ("mp attr parsed?: %s\n", parse_ret ? "no" : "yes");
+  if (!parse_ret)
+    printf ("nrli parsed?:  %s\n", nlri_ret ? "no" : "yes");
+  printf ("should parse?:  %s\n", t->parses ? "no" : "yes");
+  
+  if ((parse_ret != 0 || nlri_ret != 0) != (t->parses != 0))
+    failed++;
+  
+    
+  if (tty)
+    printf ("%s", (failed > oldfailed) ? VT100_RED "failed!" VT100_RESET 
+                                         : VT100_GREEN "OK" VT100_RESET);
+  else
+    printf ("%s", (failed > oldfailed) ? "failed!" : "OK" );
+  
+  if (failed)
+    printf (" (%u)", failed);
+  
+  printf ("\n\n");
+}
 
 /* basic parsing test */
 static void
 parse_test (struct peer *peer, struct test_segment *t, int type)
 {
-  int ret;
-  int oldfailed = failed;
+  int parse_ret = 0, nlri_ret = 0;
   struct attr attr = { };
   struct bgp_nlri nlri = { };
   struct bgp_attr_parser_args attr_args = {
@@ -465,7 +699,7 @@ parse_test (struct peer *peer, struct test_segment *t, int type)
     .length = t->len,
     .total = 1,
     .attr = &attr,
-    .type = BGP_ATTR_MP_REACH_NLRI,
+    .type = type,
     .flags = BGP_ATTR_FLAG_OPTIONAL, 
     .startp = BGP_INPUT_PNT (peer),
   };
@@ -479,40 +713,29 @@ parse_test (struct peer *peer, struct test_segment *t, int type)
   
   printf ("%s: %s\n", t->name, t->desc);
   
-  
   if (type == BGP_ATTR_MP_REACH_NLRI)
-    ret = bgp_mp_reach_parse (&attr_args, &nlri);
+    parse_ret = bgp_mp_reach_parse (&attr_args, &nlri);
   else
-    ret = bgp_mp_unreach_parse (&attr_args, &nlri);
-
-  if (!ret)
+    parse_ret = bgp_mp_unreach_parse (&attr_args, &nlri);
+  
+  if (parse_ret == 0 && t->afi_valid == VALID_AFI)
+    assert (nlri.afi == t->afi && nlri.safi == t->safi);
+  
+  if (!parse_ret)
     {
-      safi_t safi = t->safi;
+      int (*f) (struct peer *, struct attr *, struct bgp_nlri *)
+        = bgp_nlri_parse;
       
-      if (bgp_afi_safi_valid_indices (t->afi, &safi) != t->afi_valid)
-        failed++;
+      if (t->safi == SAFI_MPLS_LABELED_VPN)
+        f = bgp_nlri_parse_vpn;
       
-      printf ("MP: %u/%u (%u): recv %u, nego %u\n",
-              t->afi, t->safi, safi,
-              peer->afc_recv[t->afi][safi],
-              peer->afc_nego[t->afi][safi]);
+      if (type == BGP_ATTR_MP_REACH_NLRI)
+        nlri_ret = f (peer, &attr, &nlri);
+      else
+        nlri_ret = f (peer, NULL, &nlri);
     }
   
-  printf ("parsed?: %s\n", ret ? "no" : "yes");
-  
-  if ((ret == 0) != (t->parses == 0))
-    failed++;
-  
-  if (tty)
-    printf ("%s", (failed > oldfailed) ? VT100_RED "failed!" VT100_RESET 
-                                         : VT100_GREEN "OK" VT100_RESET);
-  else
-    printf ("%s", (failed > oldfailed) ? "failed!" : "OK" );
-  
-  if (failed)
-    printf (" (%u)", failed);
-  
-  printf ("\n\n");
+  handle_result (peer, t, parse_ret, nlri_ret);
 }
 
 static struct bgp *bgp;
@@ -538,6 +761,8 @@ main (void)
   master = thread_master_create ();
   bgp_master_init ();
   bgp_option_set (BGP_OPT_NO_LISTEN);
+  bgp_attr_init ();
+  bgp_address_init ();
   
   if (fileno (stdout) >= 0) 
     tty = isatty (fileno (stdout));
@@ -547,6 +772,7 @@ main (void)
   
   peer = peer_create_accept (bgp);
   peer->host = (char *)"foo";
+  peer->status = Established;
   
   for (i = AFI_IP; i < AFI_MAX; i++)
     for (j = SAFI_UNICAST; j < SAFI_MAX; j++)
diff --git a/tests/bgpd.tests/testbgpmpattr.exp b/tests/bgpd.tests/testbgpmpattr.exp
index 646bbe50..e6d7305a 100644
--- a/tests/bgpd.tests/testbgpmpattr.exp
+++ b/tests/bgpd.tests/testbgpmpattr.exp
@@ -19,7 +19,14 @@ simpletest "IPv6-nlri: IPV6 MP Reach, NLRI bitlen overflow"
 simpletest "IPv4: IPv4 MP Reach, 2 NLRIs + default"
 simpletest "IPv4-nhlen: IPv4 MP Reach, nexthop lenth overflow"
 simpletest "IPv4-nlrilen: IPv4 MP Reach, nlri lenth overflow"
-simpletest "IPv4-MLVPN: IPv4/MPLS-labeled VPN MP Reach, RD, Nexthop, 3 NLRIs"
+simpletest "IPv4-VPNv4: IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs"
+simpletest "IPv4-VPNv4-bogus-plen: IPv4/MPLS-labeled VPN MP Reach, RD, Nexthop, NLRI / bogus p'len"
+simpletest "IPv4-VPNv4-plen1-short: IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs, 1st plen short"
+simpletest "IPv4-VPNv4-plen1-long: IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs, 1st plen long"
+simpletest "IPv4-VPNv4-plenn-long: IPv4/VPNv4 MP Reach, RD, Nexthop, 3 NLRIs, last plen long"
+simpletest "IPv4-VPNv4-plenn-short: IPv4/VPNv4 MP Reach, RD, Nexthop, 2 NLRIs, last plen short"
+simpletest "IPv4-VPNv4-bogus-rd-type: IPv4/VPNv4 MP Reach, RD, NH, 2 NLRI, unknown RD in 1st (log, but parse)"
+simpletest "IPv4-VPNv4-0-nlri: IPv4/VPNv4 MP Reach, RD, Nexthop, 3 NLRI, 3rd 0 bogus"
 simpletest "IPv6-bug: IPv6, global nexthop, 1 default NLRI"
 simpletest "IPv6-unreach: IPV6 MP Unreach, 1 NLRI"
 simpletest "IPv6-unreach2: IPV6 MP Unreach, 2 NLRIs"
@@ -27,5 +34,5 @@ simpletest "IPv6-unreach-default: IPV6 MP Unreach, 2 NLRIs + default"
 simpletest "IPv6-unreach-nlri: IPV6 MP Unreach, NLRI bitlen overflow"
 simpletest "IPv4-unreach: IPv4 MP Unreach, 2 NLRIs + default"
 simpletest "IPv4-unreach-nlrilen: IPv4 MP Unreach, nlri length overflow"
-simpletest "IPv4-unreach-MLVPN: IPv4/MPLS-labeled VPN MP Unreach, RD, 3 NLRIs"
+simpletest "IPv4-unreach-VPNv4: IPv4/MPLS-labeled VPN MP Unreach, RD, 3 NLRIs"