From: Stuart Henderson <stu@spacehopper.org>
Subject: tcpdump print-ppp: don't try to print text for arbitrary tags
To: tech <tech@openbsd.org>
Date: Wed, 30 Oct 2024 09:52:23 +0000

tcpdump, when pointed at an interface carrying pppoe traffic, tries to
print all tags as text with some handrolled strvis-a-like:

09:37:08.807684 xx:xx:xx:xx:xx:xx xx:xx:xx:xx:xx:xx 8863 78: PPPoE-Discovery
	code Offer, version 1, type 1, id 0x0000, length 58
	tag AC-Name, length 16 BNG1.IXN-LON-RE0
	tag Host-Uniq, length 4 \240~\376\377
	tag PPP-Max-Payload, length 2 \005\334
	tag Service-Name, length 0
	tag AC-Cookie, length 16 \260\222\010\264\2106x\336w^e\023\315\375\226\321

Most of these are just arbitrary data (random IDs etc) or in some cases
numeric (like in PPP-Max-Payload).

Diff below changes to only attempt to print text for tags where this
might make sense, and otherwise print as hex, e.g.

09:44:43.417577 xx:xx:xx:xx:xx:xx xx:xx:xx:xx:xx:xx 8863 78: PPPoE-Discovery
	code Offer, version 1, type 1, id 0x0000, length 58
	tag AC-Name, length 16 BNG1.IXN-LON-RE0
	tag Host-Uniq, length 4 0xa07efeff
	tag PPP-Max-Payload, length 2 0x05dc
	tag Service-Name, length 0
	tag AC-Cookie, length 16 0xb09208b4883678de775e6513cdfd96d1

This seems a lot more readable, and makes visual comparison easier.
It would generate a solid block of numbers for anything with long tags,
but that's not worse than at present.

Any comments? ok?

Index: print-ppp.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-ppp.c,v
diff -u -p -r1.36 print-ppp.c
--- print-ppp.c	1 Dec 2021 18:28:46 -0000	1.36
+++ print-ppp.c	30 Oct 2024 09:49:30 -0000
@@ -1291,6 +1291,7 @@ pppoe_if_print(u_short ethertype, const 
 	if (ethertype == ETHERTYPE_PPPOEDISC) {
 		while (l > 0) {
 			u_int16_t t_type, t_len;
+			int text = 0;
 
 			if (l < 4)
 				goto trunc;
@@ -1310,9 +1311,11 @@ pppoe_if_print(u_short ethertype, const 
 				break;
 			case PPPOE_TAG_SERVICE_NAME:
 				printf("Service-Name");
+				text = 1;
 				break;
 			case PPPOE_TAG_AC_NAME:
 				printf("AC-Name");
+				text = 1;
 				break;
 			case PPPOE_TAG_HOST_UNIQ:
 				printf("Host-Uniq");
@@ -1331,25 +1334,32 @@ pppoe_if_print(u_short ethertype, const 
 				break;
 			case PPPOE_TAG_SERVICE_NAME_ERROR:
 				printf("Service-Name-Error");
+				text = 1;
 				break;
 			case PPPOE_TAG_AC_SYSTEM_ERROR:
 				printf("AC-System-Error");
+				text = 1;
 				break;
 			case PPPOE_TAG_GENERIC_ERROR:
 				printf("Generic-Error");
+				text = 1;
 				break;
 			default:
 				printf("Unknown(0x%04x)", t_type);
 			}
 			printf(", length %u%s", t_len, t_len ? " " : "");
 
-			if (t_len) {
+			if (t_len && text == 1) {
 				for (t_type = 0; t_type < t_len; t_type++) {
 					if (isprint(p[t_type]))
 						printf("%c", p[t_type]);
 					else
 						printf("\\%03o", p[t_type]);
 				}
+			} else if (t_len) {
+				printf("0x");
+				for (t_type = 0; t_type < t_len; t_type++)
+					printf("%02x", p[t_type]);
 			}
 			p += t_len;
 			l -= t_len;