Clean up error checking and handling.

The TLV length includes the T and the V, so it must be at least 4; fail
if it's not.

If the IPv4 or IPv6 prefix length isn't valid, don't bother trying to
parse the rest of the addresses, just give up.

Don't bother returning offset values that aren't used.

Rename some routines to indicate that they process more than one item.

Add some comments while we're at it.

Change-Id: I6825216f0e7218e230d8d60d958b3c2453a3bb62
Reviewed-on: https://code.wireshark.org/review/13016
Reviewed-by: Guy Harris <guy@alum.mit.edu>

1 files changed, 55 insertions, 71 deletions

diff --git a/epan/dissectors/packet-eigrp.c b/epan/dissectors/packet-eigrp.c
index 8a9f6fac09..46399fc3c4 100644
--- a/epan/dissectors/packet-eigrp.c
+++ b/epan/dissectors/packet-eigrp.c
@@ -35,6 +35,12 @@
 #include "packet-ipx.h"
 #include "packet-atalk.h"
 
+/*
+ * Originally Cisco proprietary; now the subject of an I-D:
+ *
+ *    https://tools.ietf.org/html/draft-savage-eigrp-04
+ */
+
 /**
  * EIGRP Header size in bytes
  */
@@ -1081,21 +1087,19 @@ dissect_eigrp_metric_flags (proto_tree *tree, tvbuff_t *tvb, int offset, int lim
 }
 
 /**
- *@fn int dissect_eigrp_ipv4_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
- *                                 packet_info *pinfo, int offset, int unreachable)
+ *@fn void dissect_eigrp_ipv4_addrs (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *                                   packet_info *pinfo, int offset, int unreachable)
  *
  * @param[in,out] tree  detail dissection result
  * @param[in] tvb       packet data
  * @param[in] pinfo     general data about the protocol
  * @param[in] offset    current byte offset in packet being processed
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect all IPv4 address from offset though the end of the packet
  */
-static int
-dissect_eigrp_ipv4_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+static void
+dissect_eigrp_ipv4_addrs (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
                          packet_info *pinfo, int offset, int unreachable)
 {
     guint8 length;
@@ -1112,11 +1116,11 @@ dissect_eigrp_ipv4_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
         addr_len = ipv4_addr_and_mask(tvb, offset + 1, ip_addr.addr_bytes, length);
 
         if (addr_len < 0) {
+            /* Invalid prefix length, more than 32 bits */
             ti_prefixlen = proto_tree_add_item(tree, hf_eigrp_ipv4_prefixlen,
                                                tvb, offset, 1, ENC_BIG_ENDIAN);
             expert_add_info_format(pinfo, ti_prefixlen, &ei_eigrp_prefixlen, "Invalid prefix length %u, must be <= 32", length);
-            addr_len = 4; /* assure we can exit the loop */
-
+            break;  /* We don't know how long this address is */
         } else {
             address addr;
 
@@ -1136,26 +1140,23 @@ dissect_eigrp_ipv4_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
         }
         first = FALSE;
     }
-    return (offset);
 }
 
 /**
- *@fn int dissect_eigrp_ipv6_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
- *                                 packet_info *pinfo, int offset, int unreachable)
+ *@fn void dissect_eigrp_ipv6_addrs (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *                                   packet_info *pinfo, int offset, int unreachable)
  *
  * @param[in,out] tree  detail dissection result
  * @param[in] tvb       packet data
  * @param[in] pinfo     general data about the protocol
  * @param[in] offset    current byte offset in packet being processed
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect all IPv6 address from offset though the end of the packet
  */
-static int
-dissect_eigrp_ipv6_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
-                         packet_info *pinfo, int offset, int unreachable)
+static void
+dissect_eigrp_ipv6_addrs (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+                          packet_info *pinfo, int offset, int unreachable)
 {
     guint8             length;
     int                addr_len;
@@ -1169,10 +1170,11 @@ dissect_eigrp_ipv6_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
         addr_len = ipv6_addr_and_mask(tvb, offset + 1, &addr, length);
 
         if (addr_len < 0) {
+            /* Invalid prefix length, more than 128 bits */
             ti_prefixlen = proto_tree_add_item(tree, hf_eigrp_ipv6_prefixlen,
                                                tvb, offset, 1, ENC_BIG_ENDIAN);
             expert_add_info_format(pinfo, ti_prefixlen, &ei_eigrp_prefixlen, "Invalid prefix length %u, must be <= 128", length);
-            addr_len = 16; /* assure we can exit the loop */
+            break;  /* We don't know how long this address is */
         } else {
             proto_tree_add_item(tree, hf_eigrp_ipv6_prefixlen, tvb, offset, 1,
                                 ENC_BIG_ENDIAN);
@@ -1195,12 +1197,11 @@ dissect_eigrp_ipv6_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
         }
         first = FALSE;
     }
-    return(offset);
 }
 
 /**
- *@fn int dissect_eigrp_ipx_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
- *                                packet_info *pinfo, int offset, int unreachable)
+ *@fn int dissect_eigrp_ipx_addrs (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *                                 packet_info *pinfo, int offset, int unreachable)
  *
  * @param[in,out] tree  detail dissection result
  * @param[in] tvb       packet data
@@ -1213,8 +1214,8 @@ dissect_eigrp_ipv6_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  * Dissect all IPX address from offset though the end of the packet
  */
 static int
-dissect_eigrp_ipx_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
-                        packet_info *pinfo, int offset, int unreachable)
+dissect_eigrp_ipx_addrs (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+                         packet_info *pinfo, int offset, int unreachable)
 {
     proto_item *ti_dst;
 
@@ -1233,8 +1234,8 @@ dissect_eigrp_ipx_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
 }
 
 /**
- *@fn int dissect_eigrp_service (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
- *                               packet_info *pinfo, int offset)
+ *@fn void dissect_eigrp_services (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *                                 packet_info *pinfo, int offset)
  *
  * @param[in,out] tree  detail dissection result
  * @param[in] tvb       packet data
@@ -1242,8 +1243,6 @@ dissect_eigrp_ipx_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  * @param[in] ti        protocol item
  * @param[in] offset    current byte offset in packet being processed
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect all SAF Services from offset though the end of the packet. The
  * following represents the format of  a SAF Service:
@@ -1279,8 +1278,8 @@ dissect_eigrp_ipx_addr (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  *
  */
-static int
-dissect_eigrp_service (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+static void
+dissect_eigrp_services (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
                        packet_info *pinfo, int offset)
 {
     int         afi, length, remaining;
@@ -1387,9 +1386,6 @@ dissect_eigrp_service (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
         }
         sub_offset += length;
     }
-
-    offset += sub_offset;
-    return(offset);
 }
 
 /**
@@ -1687,7 +1683,7 @@ dissect_eigrp_general_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
 }
 
 /**
- *@fn int dissect_eigrp_ipv4_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *@fn void dissect_eigrp_ipv4_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *                                 packet_info *pinfo, guint16 tlv)
  *
  * @param[in,out] tree  detail dissection result
@@ -1695,8 +1691,6 @@ dissect_eigrp_general_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  * @param[in] pinfo     general data about the protocol
  * @param[in] tlv       Specific TLV in to be dissected
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect the Legacy IPv4 route TLV; handles both the internal and external
  * TLV types; This packet format is being deprecated and replaced with the
@@ -1718,7 +1712,7 @@ dissect_eigrp_general_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *   | Reliability  |      Load     |  Internal Tag   |   Flag       |
  *   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  */
-static int
+static void
 dissect_eigrp_ipv4_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
                         packet_info *pinfo, guint16 tlv)
 {
@@ -1738,9 +1732,7 @@ dissect_eigrp_ipv4_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
     offset = dissect_eigrp_legacy_metric(tree, tvb, offset);
 
     /* dissect addresses */
-    offset = dissect_eigrp_ipv4_addr(ti, tree, tvb, pinfo, offset, unreachable);
-
-    return offset;
+    dissect_eigrp_ipv4_addrs(ti, tree, tvb, pinfo, offset, unreachable);
 }
 
 /**
@@ -1787,7 +1779,6 @@ dissect_eigrp_atalk_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
         proto_item_append_text(ti, ": %u-%u",
                                tvb_get_ntohs(tvb, 36), tvb_get_ntohs(tvb, 38));
     }
-    return;
 }
 
 /**
@@ -1826,12 +1817,11 @@ dissect_eigrp_ipv6_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
     offset = dissect_eigrp_legacy_metric(tree, tvb, offset);
 
     /* dissect addresses */
-    dissect_eigrp_ipv6_addr(ti, tree, tvb, pinfo, offset, unreachable);
-    return;
+    dissect_eigrp_ipv6_addrs(ti, tree, tvb, pinfo, offset, unreachable);
 }
 
 /**
- *@fn int dissect_eigrp_ipx_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *@fn void dissect_eigrp_ipx_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *                                packet_info *pinfo, guint16 tlv)
  *
  * @param[in,out] tree  detail dissection result
@@ -1839,8 +1829,6 @@ dissect_eigrp_ipv6_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  * @param[in] pinfo     general data about the protocol
  * @param[in] tlv       Specific TLV in to be dissected
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect the legacy IPX route TLV; handles both the internal and external
  * TLV type.  The following represents the format
@@ -1884,7 +1872,7 @@ dissect_eigrp_ipv6_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *
  *
  */
-static int
+static void
 dissect_eigrp_ipx_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
                        packet_info *pinfo, guint16 tlv)
 {
@@ -1903,14 +1891,12 @@ dissect_eigrp_ipx_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
     offset = dissect_eigrp_legacy_metric(tree, tvb, offset);
 
     /* dissect addresses */
-    offset = dissect_eigrp_ipx_addr(ti, tree, tvb, pinfo, offset, unreachable);
-
-    return offset;
+    dissect_eigrp_ipx_addrs(ti, tree, tvb, pinfo, offset, unreachable);
 }
 
 /**
- *@fn void dissect_eigrp_ipv4_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
- *                                 packet_info *pinfo, proto_item *ti, guint16 tlv)
+ *@fn void dissect_eigrp_multi_topology_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *                                           packet_info *pinfo, proto_item *ti, guint16 tlv)
  *
  * @param[in,out] tree  detail dissection result
  * @param[in] tvb       packet data
@@ -1918,8 +1904,6 @@ dissect_eigrp_ipx_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  * @param[in] ti        protocol item
  * @param[in] tlv       Specific TLV in to be dissected
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect the Multi-Topology route TLV; This packet format has been deprecated
  * and replaced with the Multi-Protocol packet formats as of EIGRP Release-8. Of
@@ -1953,7 +1937,7 @@ dissect_eigrp_ipx_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  *
  */
-static int
+static void
 dissect_eigrp_multi_topology_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
                                   packet_info *pinfo, guint16 tlv)
 {
@@ -1996,26 +1980,24 @@ dissect_eigrp_multi_topology_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tv
     /* dissect dest information */
     switch (afi) {
     case EIGRP_AF_IPv4:
-        offset = dissect_eigrp_ipv4_addr(ti, tree, tvb, pinfo, offset, unreachable);
+        dissect_eigrp_ipv4_addrs(ti, tree, tvb, pinfo, offset, unreachable);
         break;
     case EIGRP_AF_IPv6:
-        offset = dissect_eigrp_ipv6_addr(ti, tree, tvb, pinfo, offset, unreachable);
+        dissect_eigrp_ipv6_addrs(ti, tree, tvb, pinfo, offset, unreachable);
         break;
     case EIGRP_AF_IPX:
-        offset = dissect_eigrp_ipx_addr(ti, tree, tvb, pinfo, offset, unreachable);
+        dissect_eigrp_ipx_addrs(ti, tree, tvb, pinfo, offset, unreachable);
         break;
 
     case EIGRP_SF_COMMON:
     case EIGRP_SF_IPv4:
     case EIGRP_SF_IPv6:
-        offset = dissect_eigrp_service(ti, tree, tvb, pinfo, offset);
+        dissect_eigrp_services(ti, tree, tvb, pinfo, offset);
         break;
 
     default:
         proto_tree_add_expert(tree, pinfo, &ei_eigrp_afi, tvb, offset, -1);
     }
-
-    return offset;
 }
 
 /**
@@ -2316,7 +2298,7 @@ dissect_eigrp_wide_metric (proto_tree *tree, tvbuff_t *tvb, int offset)
 }
 
 /**
- *@fn int dissect_eigrp_multi_protocol_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
+ *@fn void dissect_eigrp_multi_protocol_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
  *                                           packet_info *pinfo, guint16 tlv)
 
  *
@@ -2325,8 +2307,6 @@ dissect_eigrp_wide_metric (proto_tree *tree, tvbuff_t *tvb, int offset)
  * @param[in] ti        protocol item
  * @param[in] pinfo     general data about the protocol
  *
- * @return int          number of bytes process
- *
  * @par
  * Dissect the Multi-Protocol (TLV Version 2.0) TLV format definition. The following
  * represents the format
@@ -2349,7 +2329,7 @@ dissect_eigrp_wide_metric (proto_tree *tree, tvbuff_t *tvb, int offset)
  *   |\/\/\/       Destination (Family Specific Length)        \/\/\/|
  *   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  */
-static int
+static void
 dissect_eigrp_multi_protocol_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tvb,
                                   packet_info *pinfo, guint16 tlv)
 {
@@ -2388,28 +2368,26 @@ dissect_eigrp_multi_protocol_tlv (proto_item *ti, proto_tree *tree, tvbuff_t *tv
     /* dissect dest information */
     switch (afi) {
     case EIGRP_AF_IPv4:
-        offset = dissect_eigrp_ipv4_addr(ti, tree, tvb, pinfo, offset, unreachable);
+        dissect_eigrp_ipv4_addrs(ti, tree, tvb, pinfo, offset, unreachable);
         break;
 
     case EIGRP_AF_IPv6:
-        offset = dissect_eigrp_ipv6_addr(ti, tree, tvb, pinfo, offset, unreachable);
+        dissect_eigrp_ipv6_addrs(ti, tree, tvb, pinfo, offset, unreachable);
         break;
 
     case EIGRP_AF_IPX:
-        offset = dissect_eigrp_ipx_addr(ti, tree, tvb, pinfo, offset, unreachable);
+        dissect_eigrp_ipx_addrs(ti, tree, tvb, pinfo, offset, unreachable);
         break;
 
     case EIGRP_SF_COMMON:
     case EIGRP_SF_IPv4:
     case EIGRP_SF_IPv6:
-        offset = dissect_eigrp_service(ti, tree, tvb, pinfo, offset);
+        dissect_eigrp_services(ti, tree, tvb, pinfo, offset);
         break;
 
     default:
         proto_tree_add_expert(tree, pinfo, &ei_eigrp_afi, tvb, offset, -1);
     }
-
-    return offset;
 }
 
 /**
@@ -2530,7 +2508,13 @@ dissect_eigrp (tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *data _
             }
 
             size =  tvb_get_ntohs(tvb, offset + 2);
-            if (size == 0) {
+            if (size < 4) {
+                /*
+                 * As the draft says, in section 6.6.2 "Length Field Encoding",
+                 * "The value does includes[sic] the Type and Length fields".
+                 *
+                 * Therefore, it must be at least 4.
+                 */
                 proto_tree_add_expert(eigrp_tree, pinfo, &ei_eigrp_tlv_len, tvb, offset, -1);
                 return(tvb_captured_length(tvb));
             }
@@ -3372,7 +3356,7 @@ proto_register_eigrp(void)
         { &ei_eigrp_tlv_type, { "eigrp.tlv_type.unknown", PI_PROTOCOL, PI_WARN, "Unknown TLV", EXPFILL }},
         { &ei_eigrp_afi, { "eigrp.afi.unknown", PI_PROTOCOL, PI_WARN, "Unknown AFI", EXPFILL }},
         { &ei_eigrp_checksum_bad, { "eigrp.checksum.bad", PI_CHECKSUM, PI_WARN, "Bad Checksum", EXPFILL }},
-        { &ei_eigrp_tlv_len, { "eigrp.tlv.len.invalid", PI_MALFORMED, PI_ERROR, "Corrupt TLV (Length field set to 0)", EXPFILL }},
+        { &ei_eigrp_tlv_len, { "eigrp.tlv.len.invalid", PI_MALFORMED, PI_ERROR, "Corrupt TLV (Length field less than 4)", EXPFILL }},
         { &ei_eigrp_tlv_trunc, { "eigrp.tlv.truncated", PI_MALFORMED, PI_ERROR, "Corrupt TLV (Truncated prematurely)", EXPFILL }},
     };