From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: tcpdump print-ppp: don't try to print text for arbitrary tags
To: tech <tech@openbsd.org>
Date: Wed, 30 Oct 2024 10:57:14 +0100

On Wed, Oct 30, 2024 at 09:52:23AM +0000, Stuart Henderson wrote:
> 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?

I like it. OK claudio@
 
> 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;
> 

-- 
:wq Claudio