Set width of hex output according to bitmask

Output from BASE_HEX et al has always been zero-padded to according to
the field type - 8 digits for FT_UINT32, etc.

When the field has a bitmask, this is definitely not appropriate - the
field type is used to indicate the size of the bitfield container, and
tells us nothing about the size of this field.

Instead, determine the actual size of the field by inspecting the
bitmask, and output the corresponding number of hex digits.

Change-Id: I10ec4e93e1e40e8b1354d5368cc8945cf671a617
Reviewed-on: https://code.wireshark.org/review/13225
Reviewed-by: João Valverde <j@v6e.pt>
Petri-Dish: João Valverde <j@v6e.pt>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Michael Mann <mmann78@netscape.net>

4 files changed, 106 insertions, 71 deletions

diff --git a/epan/dissectors/packet-ipv6.c b/epan/dissectors/packet-ipv6.c
index 66f13b427e..e7280e0568 100644
--- a/epan/dissectors/packet-ipv6.c
+++ b/epan/dissectors/packet-ipv6.c
@@ -2067,9 +2067,9 @@ dissect_ipv6(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void* data _U_
     if (tree) {
         /* !!! warning: (4-bit) version, (6-bit) DSCP, (2-bit) ECN and (20-bit) Flow */
 
-        /* _format_value() used to elide leading zeros from hex representation for IPv6 Traffic Class */
-        ti = proto_tree_add_uint_format_value(ipv6_tree, hf_ipv6_tclass, tvb,
-                            offset + IP6H_CTL_FLOW, 4, g_ntohl(ipv6->ip6_flow), "0x%02x", tfc);
+
+        ti = proto_tree_add_item(ipv6_tree, hf_ipv6_tclass, tvb,
+                            offset + IP6H_CTL_FLOW, 4, ENC_BIG_ENDIAN);
         proto_item_append_text(ti, " (DSCP: %s, ECN: %s)",
                             val_to_str_ext_const(IPDSFIELD_DSCP(tfc), &dscp_short_vals_ext, "Unknown"),
                             val_to_str_ext_const(IPDSFIELD_ECN(tfc), &ecn_short_vals_ext, "Unknown"));
diff --git a/epan/proto.c b/epan/proto.c
index f3b4b891b2..0100c06cba 100644
--- a/epan/proto.c
+++ b/epan/proto.c
@@ -168,6 +168,7 @@ struct ptvcursor {
 
 static const char *hf_try_val_to_str(guint32 value, const header_field_info *hfinfo);
 static const char *hf_try_val64_to_str(guint64 value, const header_field_info *hfinfo);
+static int hfinfo_container_bitwidth(const header_field_info *hfinfo);
 
 static void label_mark_truncated(char *label_str, gsize name_pos);
 #define LABEL_MARK_TRUNCATED_START(label_str) label_mark_truncated(label_str, 0)
@@ -4640,7 +4641,7 @@ proto_tree_set_representation_value(proto_item *pi, const char *format, va_list
 
 			val <<= hfinfo_bitshift(hf);
 
-			p = decode_bitfield_value(fi->rep->representation, val, hf->bitmask, hfinfo_bitwidth(hf));
+			p = decode_bitfield_value(fi->rep->representation, val, hf->bitmask, hfinfo_container_bitwidth(hf));
 			ret = (int) (p - fi->rep->representation);
 		}
 
@@ -7193,7 +7194,7 @@ fill_label_boolean(field_info *fi, gchar *label_str)
 	value = fvalue_get_uinteger64(&fi->value);
 	if (hfinfo->bitmask) {
 		/* Figure out the bit width */
-		bitwidth = hfinfo_bitwidth(hfinfo);
+		bitwidth = hfinfo_container_bitwidth(hfinfo);
 
 		/* Un-shift bits */
 		unshifted_value = value;
@@ -7270,15 +7271,15 @@ fill_label_bitfield(field_info *fi, gchar *label_str, gboolean is_signed)
 	header_field_info *hfinfo = fi->hfinfo;
 
 	/* Figure out the bit width */
-	bitwidth = hfinfo_bitwidth(hfinfo);
+	bitwidth = hfinfo_container_bitwidth(hfinfo);
 
 	/* Un-shift bits */
 	if (is_signed)
-		unshifted_value = fvalue_get_sinteger(&fi->value);
+		value = fvalue_get_sinteger(&fi->value);
 	else
-		unshifted_value = fvalue_get_uinteger(&fi->value);
+		value = fvalue_get_uinteger(&fi->value);
 
-	value = unshifted_value;
+	unshifted_value = value;
 	if (hfinfo->bitmask) {
 		unshifted_value <<= hfinfo_bitshift(hfinfo);
 	}
@@ -7326,17 +7327,17 @@ fill_label_bitfield64(field_info *fi, gchar *label_str, gboolean is_signed)
 	header_field_info *hfinfo = fi->hfinfo;
 
 	/* Figure out the bit width */
-	bitwidth = hfinfo_bitwidth(hfinfo);
+	bitwidth = hfinfo_container_bitwidth(hfinfo);
 
 	/* Un-shift bits */
 	if (is_signed)
-		unshifted_value = fvalue_get_sinteger64(&fi->value);
+		value = fvalue_get_sinteger64(&fi->value);
 	else
-		unshifted_value = fvalue_get_uinteger64(&fi->value);
+		value = fvalue_get_uinteger64(&fi->value);
 
-	value = unshifted_value;
+	unshifted_value = value;
 	if (hfinfo->bitmask) {
-		unshifted_value <<= hfinfo_bitshift(hfinfo); /* XXX 64-bit function needed? */
+		unshifted_value <<= hfinfo_bitshift(hfinfo);
 	}
 
 	/* Create the bitfield first */
@@ -7459,16 +7460,23 @@ hfinfo_bitshift(const header_field_info *hfinfo)
 	return ws_ctz(hfinfo->bitmask);
 }
 
-int
-hfinfo_bitwidth(const header_field_info *hfinfo)
+static int
+hfinfo_mask_bitwidth(const header_field_info *hfinfo)
 {
-	int bitwidth = 0;
+    if (!hfinfo->bitmask) {
+        return 0;
+    }
 
-	if (!hfinfo->bitmask) {
-		return 0;
-	}
+    /* ilog2 = first set bit, ctz = last set bit */
+    return ws_ilog2(hfinfo->bitmask) - ws_ctz(hfinfo->bitmask) + 1;
+}
 
-	switch (hfinfo->type) {
+static int
+hfinfo_type_bitwidth(enum ftenum type)
+{
+	int bitwidth = 0;
+
+	switch (type) {
 		case FT_UINT8:
 		case FT_INT8:
 			bitwidth = 8;
@@ -7501,9 +7509,6 @@ hfinfo_bitwidth(const header_field_info *hfinfo)
 		case FT_INT64:
 			bitwidth = 64;
 			break;
-		case FT_BOOLEAN:
-			bitwidth = hfinfo->display; /* hacky? :) */
-			break;
 		default:
 			DISSECTOR_ASSERT_NOT_REACHED();
 			;
@@ -7511,47 +7516,38 @@ hfinfo_bitwidth(const header_field_info *hfinfo)
 	return bitwidth;
 }
 
+
 static int
-_hfinfo_type_hex_octet(int type)
+hfinfo_container_bitwidth(const header_field_info *hfinfo)
 {
-	switch (type) {
-		case FT_INT8:
-		case FT_UINT8:
-			return 2;
-
-		case FT_UINT16:
-		case FT_INT16:
-			return 4;
-
-		case FT_UINT24:
-		case FT_INT24:
-			return 6;
-
-		case FT_UINT32:
-		case FT_INT32:
-			return 8;
-
-		case FT_UINT40:
-		case FT_INT40:
-			return 10;
+	if (!hfinfo->bitmask) {
+		return 0;
+	}
 
-		case FT_UINT48:
-		case FT_INT48:
-			return 12;
+	if (hfinfo->type == FT_BOOLEAN) {
+		return hfinfo->display; /* hacky? :) */
+	}
 
-		case FT_UINT56:
-		case FT_INT56:
-			return 14;
+	return hfinfo_type_bitwidth(hfinfo->type);
+}
 
-		case FT_UINT64:
-		case FT_INT64:
-			return 16;
+static int
+hfinfo_hex_digits(const header_field_info *hfinfo)
+{
+	int bitwidth;
 
-		default:
-			DISSECTOR_ASSERT_NOT_REACHED();
-			;
+	/* If we have a bitmask, hfinfo->type is the width of the container, so not
+	 * appropriate to determine the number of hex digits for the field.
+	 * So instead, we compute it from the bitmask.
+	 */
+	if (hfinfo->bitmask != 0) {
+		bitwidth = hfinfo_mask_bitwidth(hfinfo);
+	} else {
+		bitwidth = hfinfo_type_bitwidth(hfinfo->type);
 	}
-	return -1;
+
+	/* Divide by 4, rounding up, to get number of hex digits. */
+	return (bitwidth + 3) / 4;
 }
 
 static const char *
@@ -7568,7 +7564,7 @@ hfinfo_number_value_format_display(const header_field_info *hfinfo, int display,
 
 		case BASE_DEC_HEX:
 			*(--ptr) = ')';
-			ptr = hex_to_str_back(ptr, _hfinfo_type_hex_octet(hfinfo->type), value);
+			ptr = hex_to_str_back(ptr, hfinfo_hex_digits(hfinfo), value);
 			*(--ptr) = '(';
 			*(--ptr) = ' ';
 			ptr = isint ? int_to_str_back(ptr, (gint32) value) : uint_to_str_back(ptr, value);
@@ -7578,14 +7574,14 @@ hfinfo_number_value_format_display(const header_field_info *hfinfo, int display,
 			return oct_to_str_back(ptr, value);
 
 		case BASE_HEX:
-			return hex_to_str_back(ptr, _hfinfo_type_hex_octet(hfinfo->type), value);
+			return hex_to_str_back(ptr, hfinfo_hex_digits(hfinfo), value);
 
 		case BASE_HEX_DEC:
 			*(--ptr) = ')';
 			ptr = isint ? int_to_str_back(ptr, (gint32) value) : uint_to_str_back(ptr, value);
 			*(--ptr) = '(';
 			*(--ptr) = ' ';
-			ptr = hex_to_str_back(ptr, _hfinfo_type_hex_octet(hfinfo->type), value);
+			ptr = hex_to_str_back(ptr, hfinfo_hex_digits(hfinfo), value);
 			return ptr;
 
 		case BASE_PT_UDP:
@@ -7616,7 +7612,7 @@ hfinfo_number_value_format_display64(const header_field_info *hfinfo, int displa
 
 			case BASE_DEC_HEX:
 				*(--ptr) = ')';
-				ptr = hex64_to_str_back(ptr, _hfinfo_type_hex_octet(hfinfo->type), value);
+				ptr = hex64_to_str_back(ptr, hfinfo_hex_digits(hfinfo), value);
 				*(--ptr) = '(';
 				*(--ptr) = ' ';
 				ptr = isint ? int64_to_str_back(ptr, (gint64) value) : uint64_to_str_back(ptr, value);
@@ -7626,14 +7622,14 @@ hfinfo_number_value_format_display64(const header_field_info *hfinfo, int displa
 				return oct64_to_str_back(ptr, value);
 
 			case BASE_HEX:
-				return hex64_to_str_back(ptr, _hfinfo_type_hex_octet(hfinfo->type), value);
+				return hex64_to_str_back(ptr, hfinfo_hex_digits(hfinfo), value);
 
 			case BASE_HEX_DEC:
 				*(--ptr) = ')';
 				ptr = isint ? int64_to_str_back(ptr, (gint64) value) : uint64_to_str_back(ptr, value);
 				*(--ptr) = '(';
 				*(--ptr) = ' ';
-				ptr = hex64_to_str_back(ptr, _hfinfo_type_hex_octet(hfinfo->type), value);
+				ptr = hex64_to_str_back(ptr, hfinfo_hex_digits(hfinfo), value);
 				return ptr;
 
 			default:
diff --git a/epan/proto.h b/epan/proto.h
index 002bc4295c..b31cf23a8f 100644
--- a/epan/proto.h
+++ b/epan/proto.h
@@ -2311,12 +2311,6 @@ WS_DLL_PUBLIC void tree_expanded_set(int tree_type, gboolean value);
 #define g_ptr_array_len(a)      ((a)?(a)->len:0)
 #endif
 
-/** Get number of bits of a header_field.
- @param hfinfo header_field
- @return the bitwidth */
-extern int
-hfinfo_bitwidth(const header_field_info *hfinfo);
-
 WS_DLL_PUBLIC int
 hfinfo_bitshift(const header_field_info *hfinfo);
 
diff --git a/wsutil/bits_ctz.h b/wsutil/bits_ctz.h
index dfdf24bf93..08125c04ab 100644
--- a/wsutil/bits_ctz.h
+++ b/wsutil/bits_ctz.h
@@ -25,18 +25,32 @@
 
 #include <glib.h>
 
+/* ws_ctz == trailing zeros == position of lowest set bit [0..63] */
+/* ws_ilog2 == position of highest set bit == 63 - leading zeros [0..63] */
+
+/* The return value of both ws_ctz and ws_ilog2 is undefined for x == 0 */
+
 #if defined(__GNUC__) && ((__GNUC__ > 3) || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4))
+
 static inline int
 ws_ctz(guint64 x)
 {
 	return __builtin_ctzll(x);
 }
+
+static inline int
+ws_ilog2(guint64 x)
+{
+	return 63 - __builtin_clzll(x);
+}
+
 #else
+
 static inline int
 __ws_ctz32(guint32 x)
 {
 	/* From http://graphics.stanford.edu/~seander/bithacks.html#ZerosOnRightMultLookup */
-	static const int table[32] = {
+	static const guint8 table[32] = {
 		0,   1, 28,  2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17,  4, 8,
 		31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18,  6, 11,  5, 10, 9
 	};
@@ -55,6 +69,37 @@ ws_ctz(guint64 x)
 	else
 		return __ws_ctz32(lo);
 }
+
+static inline int
+__ws_ilog2_32(guint32 x)
+{
+	/* From http://graphics.stanford.edu/~seander/bithacks.html#IntegerLogDeBruijn */
+	static const guint8 table[32] =	{
+		0,  9,  1, 10, 13, 21,  2, 29, 11, 14, 16, 18, 22, 25,  3, 30,
+		8, 12, 20, 28, 15, 17, 24,  7, 19, 27, 23,  6, 26,  5,  4, 31
+	};
+
+	x |= x >> 1;
+	x |= x >> 2;
+	x |= x >> 4;
+	x |= x >> 8;
+	x |= x >> 16;
+
+	return table[((guint32)(x * 0x07C4ACDDU)) >> 27];
+}
+
+static inline int
+ws_ilog2(guint64 x)
+{
+	guint32 hi = x >> 32;
+	guint32 lo = (guint32) x;
+
+	if (hi == 0)
+		return __ws_ilog2_32(lo);
+	else
+		return 32 + __ws_ilog2_32(hi);
+}
+
 #endif
 
 #endif /* __WSUTIL_BITS_CTZ_H__ */