isotpsniffer: option for no quitting on invalid message received #550

maziu · 2024-07-19T09:47:55Z

Hello,

I was working with isotpsniffer, and found out that if invalid / incomplete / malformed iso tp message is received, isotpsniffer immediately quits:

┌──(kali㉿kali)-[~/can-utils]
└─$ ./isotpsniffer -s 1D9 -d 481 vcan0
read socket t: Invalid or incomplete multibyte or wide character
                                                                                                                    
┌──(kali㉿kali)-[~/can-utils]
└─$

This wasn't desired behaviour, as I had to use malformed frames for target debugging purposes

This PR address this issue and adds additional command line option (-q) to continue receiving if read command returns -1

./isotpsniffer -s 1D9 -d 481 -q vcan0
read socket s: Invalid or incomplete multibyte or wide character
read socket t: Invalid or incomplete multibyte or wide character
 vcan0  481  [6]  00 04 12 02 10 06  - '......'
 vcan0  1D9  [6]  00 04 22 02 12 00  - '.."...'
 vcan0  481  [16]  00 0E 12 0C 0A 0A 08 08 12 06 0A 04 52 BB 74 EF  - '............R.t.'
 vcan0  1D9  [14]  00 0C 22 0A 1A 08 12 06 0A 04 52 BB 74 EF  - '..".......R.t.'
^C
                                                                                                                    
┌──(kali㉿kali)-[~/can-utils]

As side effect, when socket is closed while isotpsniffer is working, isotpsniffer will NOT exit - that's why this is added as option, not default behaviour:

$ ./isotpsniffer -s 1D9 -d 481 -q vcan0
read socket s: Invalid or incomplete multibyte or wide character
read socket t: Invalid or incomplete multibyte or wide character
 vcan0  481  [6]  00 04 12 02 10 06  - '......'
 vcan0  1D9  [6]  00 04 22 02 12 00  - '.."...'
read socket t: Network is down
[continues to work. do not quit]

I see possibility to make decision about exit basing on errno value. What do you think about that? Any comments appreciated.

hartkopp · 2024-07-21T19:40:55Z

isotpsniffer.c

@@ -79,6 +79,7 @@ void print_usage(char *prg)
 	fprintf(stderr, "         -f <format>  (1 = HEX, 2 = ASCII, 3 = HEX & ASCII - default: %d)\n", FORMAT_DEFAULT);
 	fprintf(stderr, "         -L           (set link layer options for CAN FD)\n");
 	fprintf(stderr, "         -h <len>     (head: print only first <len> bytes)\n");
+	fprintf(stderr, "         -q           (don't quit on read error, allows to receive malformed frames)\n");


I would suggest to use '-i' analog to cangen:

-i (ignore syscall errors to receive malformed PDUs)

hartkopp · 2024-07-21T19:42:12Z

isotpsniffer.c

@@ -189,6 +190,7 @@ int main(int argc, char **argv)
 	int head = 0;
 	int timestamp = 0;
 	int format = FORMAT_DEFAULT;
+	int noquit = 0;


To follow up with the '-i' option

ignore_errors = 0

hartkopp · 2024-07-21T19:43:44Z

isotpsniffer.c

 			}
 			if (nbytes > 4095) {
 				r = 1;
+				perror("read socket s too much data");


PDU length %d longer than PDU buffer", nbytes)

hartkopp · 2024-07-21T19:44:08Z

isotpsniffer.c

 			}
 			if (nbytes > 4095) {
 				r = 1;
+				perror("read socket t too much data");


PDU length %d longer than PDU buffer", nbytes)

hartkopp · 2024-07-21T19:48:54Z

isotpsniffer.c

@@ -197,7 +199,7 @@ int main(int argc, char **argv)
 	unsigned char buffer[4096];


Can you add some
#define PDU_BUF_SIZE 8300
here which is then used for buffer[PDU_BUF_SIZE] and in the checks with '4095' below.

hartkopp · 2024-07-21T19:49:40Z

isotpsniffer.c

@@ -371,29 +377,35 @@ int main(int argc, char **argv)
 			if (nbytes < 0) {
 				perror("read socket s");
 				r = 1;
-				goto out;
+				if(!noquit)
+					goto out;
 			}
 			if (nbytes > 4095) {


if (nbytes > PDU_BUF_SIZE - 1)

hartkopp · 2024-07-21T19:49:59Z

isotpsniffer.c

 		}

 		if (FD_ISSET(t, &rdfs)) {
 			nbytes = read(t, buffer, 4096);
 			if (nbytes < 0) {
 				perror("read socket t");
 				r = 1;
-				goto out;
+				if(!noquit)
+					goto out;
 			}
 			if (nbytes > 4095) {


if (nbytes > PDU_BUF_SIZE - 1)

hartkopp · 2024-07-21T19:51:45Z

isotpsniffer.c

@@ -249,6 +251,10 @@ int main(int argc, char **argv)
 			}
 			break;

+		case 'q':


hartkopp · 2024-07-21T19:57:21Z

Sorry for the mess in this review. It was the first time where I not only added single comments but used the "start a review" button ...

Thanks for the contribution!

maziu · 2024-09-26T12:33:01Z

Provided changes you requested - sorry that it took so long

maziu · 2024-09-26T13:04:32Z

Sorry for the force push. It was non-intentional change, I've modified github actions script on my fork and forgot that it will go also to this PR (facepalm). Force push reverted it to previous state

hartkopp · 2024-09-26T18:07:16Z

isotpsniffer.c

 				r = 1;
-				perror("read socket s too much data");
+				fprintf(stderr, "PDU length %d longer than PDU buffer: %s\n", nbytes, strerror(errno));


fprintf(stderr, "PDU length %d longer than PDU buffer size %d\n", nbytes, PDU_BUF_SIZE - 1);

(same applies for line 406)

Additionally please set PDU_BUF_SIZE to 8300 as suggested in my previous review.

Can you finally please put all these things into one single patch for the PR.
If this turns out to be too complicated, feel free to create a new PR.

Many thanks!

marckleinebudde

....and as Oliver suggested, please merge both patches.

marckleinebudde · 2024-09-27T07:28:50Z

isotpsniffer.c

@@ -59,13 +59,16 @@
 #include <linux/can.h>
 #include <linux/can/isotp.h>
 #include <linux/sockios.h>
+#include <errno.h>


please sort alphabetically

marckleinebudde · 2024-09-27T07:29:16Z

isotpsniffer.c

-	int noquit = 0;
+	int ignore_errors = 0;


marckleinebudde · 2024-09-27T07:29:28Z

isotpsniffer.c

-		case 'q':
-			noquit = 1;
+		case 'i':
+			ignore_errors = 1;


isotpsniffer: option for no quitting on invalid message received

1e2c8fe

olerem requested a review from hartkopp July 21, 2024 18:36

hartkopp reviewed Jul 21, 2024

View reviewed changes

isotpsniffer: change -q to -i parameter, const buffer size

0cfff2e

maziu force-pushed the master branch from 738b2b7 to 0cfff2e Compare September 26, 2024 13:03

hartkopp reviewed Sep 26, 2024

View reviewed changes

marckleinebudde requested changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

isotpsniffer: option for no quitting on invalid message received #550

isotpsniffer: option for no quitting on invalid message received #550

maziu commented Jul 19, 2024 •

edited

Loading

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp Jul 21, 2024

hartkopp commented Jul 21, 2024

maziu commented Sep 26, 2024

maziu commented Sep 26, 2024 •

edited

Loading

hartkopp Sep 26, 2024

marckleinebudde left a comment

marckleinebudde Sep 27, 2024

marckleinebudde Sep 27, 2024

marckleinebudde Sep 27, 2024

		@@ -197,7 +199,7 @@ int main(int argc, char **argv)
		unsigned char buffer[4096];

isotpsniffer: option for no quitting on invalid message received #550

Are you sure you want to change the base?

isotpsniffer: option for no quitting on invalid message received #550

Conversation

maziu commented Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartkopp commented Jul 21, 2024

maziu commented Sep 26, 2024

maziu commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

marckleinebudde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maziu commented Jul 19, 2024 •

edited

Loading

maziu commented Sep 26, 2024 •

edited

Loading