slcanpty: use the SLCAN message length when parsing commands

pty2can() detected complete SLCAN messages by looking for the '\r'
termination but did not use this message length for parsing. The
content was read at fixed positions and the start of the following
message was derived from the parsed content.

Too short messages (e.g. 'T1\r') read the DLC and data from stale
buffer content and sent a phantom CAN frame. Following commands got
lost after short messages, unknown commands or frames with timestamp.

Terminate the current message at its '\r' position, check that the
CAN ID fits into the message and always continue with the next message
behind the '\r'.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

asc2log.c

@@ -42,6 +42,7 @@
  *
  */
 
+#include <ctype.h>
 #include <libgen.h>
 #include <locale.h>
 #include <stdio.h>
@@ -114,6 +115,42 @@ static void get_can_id(canid_t *can_id, char *idstring, int base)
 	*can_id |= strtoul(idstring, NULL, base);
 }
 
+/*
+ * Read 'len' ASCII hex bytes separated by single whitespace characters.
+ * Returns a pointer to the content behind the data or NULL on error.
+ */
+static char *get_hex_data(char *ptr, unsigned char *data, int len)
+{
+	unsigned char ctmp;
+	int i;
+
+	for (i = 0; i < len; i++) {
+		ctmp = asc2nibble(ptr[0]);
+		if (ctmp > 0x0F)
+			return NULL;
+
+		data[i] = (ctmp << 4);
+
+		ctmp = asc2nibble(ptr[1]);
+		if (ctmp > 0x0F)
+			return NULL;
+
+		data[i] |= ctmp;
+
+		/*
+		 * The hex byte has to be followed by a separator (space/tab)
+		 * or an EOL char ('\n', '\r'). This also ensures that we do
+		 * not skip the string termination with ptr += 3 below.
+		 */
+		if (!isspace((unsigned char)ptr[2]))
+			return NULL;
+
+		ptr += 3; /* start of next ASCII hex byte */
+	}
+
+	return ptr;
+}
+
 static void calc_tv(struct timeval *tv, struct timeval *read_tv,
 		    struct timeval *date_tv, char timestamps, int dplace)
 {
@@ -155,7 +192,7 @@ static void eval_can(char* buf, struct timeval *date_tvp, char timestamps,
 	struct canfd_frame cf = { 0 };
 	struct can_frame *ccf = (struct can_frame *)&cf; /* for len8_dlc */
 	char rtr;
-	int dlc = 0;
+	unsigned int dlc = 0;
 	int len = 0;
 	int data[8];
 	char idstr[21];
@@ -261,13 +298,12 @@ static void eval_canfd(char* buf, struct timeval *date_tvp, char timestamps,
 	static struct timeval tv; /* current frame timestamp */
 	static struct timeval read_tv; /* frame timestamp from ASC file */
 	struct canfd_frame cf = { 0 };
-	unsigned char brs, esi, ctmp;
+	unsigned char brs, esi;
 	unsigned int flags;
-	int dlc, dlen = 0;
+	unsigned int dlc, dlen = 0;
 	char idstr[21];
 	char dir[5]; /* 'Rx'/'Tx'/'TxRq' plus terminating zero */
 	char *ptr;
-	int i;
 	int n = 0; /* sscanf consumed characters */
 	unsigned long long sec, usec;
 
@@ -283,12 +319,12 @@ static void eval_canfd(char* buf, struct timeval *date_tvp, char timestamps,
 	 */
 
 	/* check for valid line without symbolic name */
-	if (sscanf(buf, "%llu.%llu %*s %d %4s %20s %hhx %hhx %x %d %n",
+	if (sscanf(buf, "%llu.%llu %*s %d %4s %20s %hhx %hhx %x %u %n",
 		   &sec, &usec, &interface,
 		   dir, idstr, &brs, &esi, &dlc, &dlen, &n) != 9) {
 
 		/* check for valid line with a symbolic name */
-		if (sscanf(buf, "%llu.%llu %*s %d %4s %20s %*s %hhx %hhx %x %d %n",
+		if (sscanf(buf, "%llu.%llu %*s %d %4s %20s %*s %hhx %hhx %x %u %n",
 			   &sec, &usec, &interface,
 			   dir, idstr, &brs, &esi, &dlc, &dlen, &n) != 9) {
 
@@ -328,22 +364,10 @@ static void eval_canfd(char* buf, struct timeval *date_tvp, char timestamps,
 
 	cf.len = dlen;
 
-	for (i = 0; i < dlen; i++) {
-		ctmp = asc2nibble(ptr[0]);
-		if (ctmp > 0x0F)
+	ptr = get_hex_data(ptr, cf.data, dlen);
+	if (!ptr)
 		return;
 
-		cf.data[i] = (ctmp << 4);
-
-		ctmp = asc2nibble(ptr[1]);
-		if (ctmp > 0x0F)
-			return;
-
-		cf.data[i] |= ctmp;
-
-		ptr += 3; /* start of next ASCII hex byte */
-	}
-
 	/* skip MessageDuration and MessageLength to get Flags value */
 	if (sscanf(ptr, "   %*x %*x %x ", &flags) != 1)
 		return;
@@ -387,13 +411,11 @@ static void eval_canxl_cc(char* buf, struct timeval *date_tvp, char timestamps,
 	static struct timeval tv; /* current frame timestamp */
 	static struct timeval read_tv; /* frame timestamp from ASC file */
 	struct can_frame cf = { 0 };
-	unsigned char ctmp;
 	unsigned int flags;
-	int dlc, dlen = 0;
+	unsigned int dlc, dlen = 0;
 	char idstr[21];
 	char dir[5]; /* 'Rx'/'Tx'/'TxRq' plus terminating zero */
 	char *ptr;
-	int i;
 	int n = 0; /* sscanf consumed characters */
 	unsigned long long sec, usec;
 
@@ -408,7 +430,7 @@ static void eval_canxl_cc(char* buf, struct timeval *date_tvp, char timestamps,
 	if (sscanf(buf,
 		   "%llu.%llu %*s %d %4s " /* time, CANXL, channel, direction */
 		   "%*s %*s %*s %20s " /* frame format, msg dur, bit count, ID */
-		   "%x %d %n", /* DLC, Datalen */
+		   "%x %u %n", /* DLC, Datalen */
 		   &sec, &usec, &interface, dir,
 		   idstr,
 		   &dlc, &dlen, &n) != 7) {
@@ -416,7 +438,7 @@ static void eval_canxl_cc(char* buf, struct timeval *date_tvp, char timestamps,
 		if (sscanf(buf,
 			   "%llu.%llu %*s %d %4s " /* time, CANXL, channel, direction */
 			   "%*s %*s %*s %20s " /* frame format, msg dur, bit count, ID */
-			   "%*s %x %d %n", /* sym name, DLC, Datalen */
+			   "%*s %x %u %n", /* sym name, DLC, Datalen */
 			   &sec, &usec, &interface, dir,
 			   idstr,
 			   &dlc, &dlen, &n) != 7) {
@@ -452,22 +474,10 @@ static void eval_canxl_cc(char* buf, struct timeval *date_tvp, char timestamps,
 
 	cf.len = dlen;
 
-	for (i = 0; i < dlen; i++) {
-		ctmp = asc2nibble(ptr[0]);
-		if (ctmp > 0x0F)
+	ptr = get_hex_data(ptr, cf.data, dlen);
+	if (!ptr)
 		return;
 
-		cf.data[i] = (ctmp << 4);
-
-		ctmp = asc2nibble(ptr[1]);
-		if (ctmp > 0x0F)
-			return;
-
-		cf.data[i] |= ctmp;
-
-		ptr += 3; /* start of next ASCII hex byte */
-	}
-
 	/* skip FCRC to get Flags value */
 	if (sscanf(ptr, "%*x %x ", &flags) != 1)
 		return;
@@ -498,13 +508,11 @@ static void eval_canxl_fd(char* buf, struct timeval *date_tvp, char timestamps,
 	static struct timeval tv; /* current frame timestamp */
 	static struct timeval read_tv; /* frame timestamp from ASC file */
 	struct canfd_frame cf = { 0 };
-	unsigned char ctmp;
 	unsigned int flags;
-	int dlc, dlen = 0;
+	unsigned int dlc, dlen = 0;
 	char idstr[21];
 	char dir[5]; /* 'Rx'/'Tx'/'TxRq' plus terminating zero */
 	char *ptr;
-	int i;
 	int n = 0; /* sscanf consumed characters */
 	unsigned long long sec, usec;
 
@@ -519,7 +527,7 @@ static void eval_canxl_fd(char* buf, struct timeval *date_tvp, char timestamps,
 	if (sscanf(buf,
 		   "%llu.%llu %*s %d %4s " /* time, CANXL, channel, direction */
 		   "%*s %*s %*s %20s " /* frame format, msg dur, bit count, ID */
-		   "%x %d %n", /* DLC, Datalen */
+		   "%x %u %n", /* DLC, Datalen */
 		   &sec, &usec, &interface, dir,
 		   idstr,
 		   &dlc, &dlen, &n) != 7) {
@@ -527,7 +535,7 @@ static void eval_canxl_fd(char* buf, struct timeval *date_tvp, char timestamps,
 		if (sscanf(buf,
 			   "%llu.%llu %*s %d %4s " /* time, CANXL, channel, direction */
 			   "%*s %*s %*s %20s " /* frame format, msg dur, bit count, ID */
-			   "%*s %x %d %n", /* sym name, DLC, Datalen */
+			   "%*s %x %u %n", /* sym name, DLC, Datalen */
 			   &sec, &usec, &interface, dir,
 			   idstr,
 			   &dlc, &dlen, &n) != 7) {
@@ -563,22 +571,10 @@ static void eval_canxl_fd(char* buf, struct timeval *date_tvp, char timestamps,
 
 	cf.len = dlen;
 
-	for (i = 0; i < dlen; i++) {
-		ctmp = asc2nibble(ptr[0]);
-		if (ctmp > 0x0F)
+	ptr = get_hex_data(ptr, cf.data, dlen);
+	if (!ptr)
 		return;
 
-		cf.data[i] = (ctmp << 4);
-
-		ctmp = asc2nibble(ptr[1]);
-		if (ctmp > 0x0F)
-			return;
-
-		cf.data[i] |= ctmp;
-
-		ptr += 3; /* start of next ASCII hex byte */
-	}
-
 	/* skip stuff field and FCRC to get Flags value */
 	if (sscanf(ptr, "%*s %*s %x ", &flags) != 1)
 		return;
@@ -605,13 +601,12 @@ static void eval_canxl_xl(char* buf, struct timeval *date_tvp, char timestamps,
 	static struct timeval tv; /* current frame timestamp */
 	static struct timeval read_tv; /* frame timestamp from ASC file */
 	struct canxl_frame cf = { 0 };
-	unsigned char sdt, vcid, secbit, ctmp;
+	unsigned char sdt, vcid, secbit;
 	unsigned int af, flags;
-	int dlc, dlen = 0;
+	unsigned int dlc, dlen = 0;
 	char idstr[21];
 	char dir[5]; /* 'Rx'/'Tx'/'TxRq' plus terminating zero */
 	char *ptr;
-	int i;
 	int n = 0; /* sscanf consumed characters */
 	unsigned long long sec, usec;
 
@@ -627,7 +622,7 @@ static void eval_canxl_xl(char* buf, struct timeval *date_tvp, char timestamps,
 	if (sscanf(buf,
 		   "%llu.%llu %*s %d %4s " /* time, CANXL, channel, direction */
 		   "%*s %*s %*s %20s " /* frame format, msg dur, bit count, ID */
-		   "%hhx %hhx %x %d " /* SDT, SEC, DLC, Datalen */
+		   "%hhx %hhx %x %u " /* SDT, SEC, DLC, Datalen */
 		   "%*s %*s %hhx %x %n", /* stuff bit count, crc, VCID, AF */
 		   &sec, &usec, &interface, dir,
 		   idstr,
@@ -637,7 +632,7 @@ static void eval_canxl_xl(char* buf, struct timeval *date_tvp, char timestamps,
 		if (sscanf(buf,
 			   "%llu.%llu %*s %d %4s " /* time, CANXL, channel, direction */
 			   "%*s %*s %*s %20s " /* frame format, msg dur, bit count, ID */
-			   "%*s %hhx %hhx %x %d " /* sym name, SDT, SEC, DLC, Datalen */
+			   "%*s %hhx %hhx %x %u " /* sym name, SDT, SEC, DLC, Datalen */
 			   "%*s %*s %hhx %x %n", /* stuff bit count, crc, VCID, AF */
 			   &sec, &usec, &interface, dir,
 			   idstr,
@@ -689,22 +684,10 @@ static void eval_canxl_xl(char* buf, struct timeval *date_tvp, char timestamps,
 
 	cf.len = dlen;
 
-	for (i = 0; i < dlen; i++) {
-		ctmp = asc2nibble(ptr[0]);
-		if (ctmp > 0x0F)
+	ptr = get_hex_data(ptr, cf.data, dlen);
+	if (!ptr)
 		return;
 
-		cf.data[i] = (ctmp << 4);
-
-		ctmp = asc2nibble(ptr[1]);
-		if (ctmp > 0x0F)
-			return;
-
-		cf.data[i] |= ctmp;
-
-		ptr += 3; /* start of next ASCII hex byte */
-	}
-
 	/* skip FCRC to get Flags value */
 	if (sscanf(ptr, "%*x %x ", &flags) != 1)
 		return;

cangw.c

@@ -338,7 +338,7 @@ static int parse_mod(char *optarg, struct modattr *modmsg)
 	ptr = optarg;
 	nptr = strchr(ptr, ':');
 
-	if ((nptr - ptr > 3) || (nptr - ptr == 0))
+	if (!nptr || (nptr - ptr > 3) || (nptr - ptr == 0))
 		return 1;
 
 	if (!strncmp(ptr, "AND", 3))
@@ -355,7 +355,7 @@ static int parse_mod(char *optarg, struct modattr *modmsg)
 	ptr = nptr+1;
 	nptr = strchr(ptr, ':');
 
-	if ((nptr - ptr > 3) || (nptr - ptr == 0))
+	if (!nptr || (nptr - ptr > 3) || (nptr - ptr == 0))
 		return 3;
 
 	modmsg->modtype = 0;
@@ -406,7 +406,7 @@ static int parse_fdmod(char *optarg, struct fdmodattr *modmsg)
 	ptr = optarg;
 	nptr = strchr(ptr, ':');
 
-	if ((nptr - ptr > 3) || (nptr - ptr == 0))
+	if (!nptr || (nptr - ptr > 3) || (nptr - ptr == 0))
 		return 1;
 
 	if (!strncmp(ptr, "AND", 3))
@@ -423,7 +423,7 @@ static int parse_fdmod(char *optarg, struct fdmodattr *modmsg)
 	ptr = nptr+1;
 	nptr = strchr(ptr, ':');
 
-	if ((nptr - ptr > 4) || (nptr - ptr == 0))
+	if (!nptr || (nptr - ptr > 4) || (nptr - ptr == 0))
 		return 3;
 
 	modmsg->modtype = 0;

lib.c

@@ -664,6 +664,7 @@ int snprintf_long_canframe(char *buf, size_t size, union cfu *cu, int view)
 	return offset;
 }
 
+/* check CANERR_BUFFSZ in lib.h when changing the error frame strings */
 static const char *error_classes[] = {
 	"tx-timeout",
 	"lost-arbitration",
@@ -844,6 +845,13 @@ int snprintf_can_error_frame(char *buf, size_t len, const struct canfd_frame *cf
 	if (!sep)
 		sep = defsep;
 
+	/* the size of the provided buffer has to fit CANERR_BUFFSZ (see lib.h) */
+	if (len < CANERR_BUFFSZ) {
+		if (len)
+			buf[0] = 0; /* empty terminated string */
+		return 0;
+	}
+
 	for (i = 0; i < (int)ARRAY_SIZE(error_classes); i++) {
 		mask = 1 << i;
 		if (class & mask) {

lib.h

@@ -238,11 +238,30 @@ int snprintf_long_canframe(char *buf, size_t size, union cfu *cu, int view);
  * data bytes is limited to 64 to fit terminal output use-cases.
  */
 
-int snprintf_can_error_frame(char *buf, size_t len, const struct canfd_frame *cf,
-			     const char *sep);
 /*
  * Creates a CAN error frame output in user readable format.
+ *
+ * buf: buffer for the output string
+ * len: size of the provided buffer buf in bytes (incl. string termination)
+ * sep: separator string between the error classes (NULL => default ",")
+ *
+ * The buffer size len needs to be at least CANERR_BUFFSZ bytes - otherwise
+ * an empty string is returned.
+ *
+ * Calculation of the required buffer size:
+ * The longest error frame content (all error classes and details set) has
+ * 465 characters plus the string termination = 466 bytes. Additionally up
+ * to 9 separators are inserted which results in 466 + 9 * strlen(sep) bytes,
+ * e.g. 466 + 9 * 2 = 484 bytes for "\n\t" (as used in candump).
+ * With CANERR_BUFFSZ = 800 the separator must not exceed 37 characters.
+ *
+ * When the error frame content (the strings in lib.c) is changed, the current
+ * values have to be re-calculated and CANERR_BUFFSZ has to be adapted if needed.
  */
+#define CANERR_BUFFSZ 800
+
+int snprintf_can_error_frame(char *buf, size_t len, const struct canfd_frame *cf,
+			     const char *sep);
 
 /**
  * timespec_diff_ms - calculate timespec difference in milliseconds

slcanpty.c

@@ -58,6 +58,7 @@ static int pty2can(int pty, int socket, struct can_filter *fi,
 	static char buf[200];
 	char replybuf[10]; /* for answers to received commands */
 	unsigned int ptr;
+	unsigned int msglen; /* length of the current SLCAN message w/o '\r' */
 	struct can_frame frame;
 	int ret, i;
 	static unsigned int rxoffset = 0; /* points to the end of an received incomplete SLCAN message */
@@ -102,6 +103,7 @@ rx_restart:
 		return 0;
 	}
 
+	msglen = tmp;
 	cmd = buf[0];
 	buf[nbytes] = 0;
 
@@ -112,10 +114,11 @@ rx_restart:
 			pr_debug("%c", buf[tmp]);
 	pr_debug("\n");
 
+	/* terminate the current SLCAN message to not read the following content */
+	buf[msglen] = 0;
+
 	/* check for filter configuration commands */
 	if (cmd == 'm' || cmd == 'M') {
-		buf[9] = 0; /* terminate filter string */
-		ptr = 9;
 #if 0
 		/* the filter is no SocketCAN filter :-( */
 
@@ -140,14 +143,12 @@ rx_restart:
 	/* check for timestamp on/off command */
 	if (cmd == 'Z') {
 		*tstamp = buf[1] & 0x01;
-		ptr = 2;
 		goto rx_out_ack;
 	}
 
 	/* check for 'O'pen command */
 	if (cmd == 'O') {
 		setsockopt(socket, SOL_CAN_RAW, CAN_RAW_FILTER, fi, sizeof(struct can_filter));
-		ptr = 1;
 		*is_open = 1;
 		goto rx_out_ack;
 	}
@@ -155,7 +156,6 @@ rx_restart:
 	/* check for 'C'lose command */
 	if (cmd == 'C') {
 		setsockopt(socket, SOL_CAN_RAW, CAN_RAW_FILTER, NULL, 0);
-		ptr = 1;
 		*is_open = 0;
 		goto rx_out_ack;
 	}
@@ -164,14 +164,12 @@ rx_restart:
 	if (cmd == 'V') {
 		sprintf(replybuf, "V1013\r");
 		tmp = strlen(replybuf);
-		ptr = 1;
 		goto rx_out;
 	}
 	/* check for 'v'ersion command */
 	if (cmd == 'v') {
 		sprintf(replybuf, "v1014\r");
 		tmp = strlen(replybuf);
-		ptr = 1;
 		goto rx_out;
 	}
 
@@ -179,7 +177,6 @@ rx_restart:
 	if (cmd == 'N') {
 		sprintf(replybuf, "N4242\r");
 		tmp = strlen(replybuf);
-		ptr = 1;
 		goto rx_out;
 	}
 
@@ -187,29 +184,23 @@ rx_restart:
 	if (cmd == 'F') {
 		sprintf(replybuf, "F00\r");
 		tmp = strlen(replybuf);
-		ptr = 1;
 		goto rx_out;
 	}
 
 	/* correctly answer unsupported commands */
 	if (cmd == 'U') {
-		ptr = 2;
 		goto rx_out_ack;
 	}
 	if (cmd == 'S') {
-		ptr = 2;
 		goto rx_out_ack;
 	}
 	if (cmd == 's') {
-		ptr = 5;
 		goto rx_out_ack;
 	}
 	if (cmd == 'P' || cmd == 'A') {
-		ptr = 1;
 		goto rx_out_nack;
 	}
 	if (cmd == 'X') {
-		ptr = 2;
 		if (buf[1] & 0x01)
 			goto rx_out_ack;
 		else
@@ -217,16 +208,18 @@ rx_restart:
 	}
 
 	/* catch unknown commands */
-	if ((cmd != 't') && (cmd != 'T') && (cmd != 'r') && (cmd != 'R')) {
-		ptr = nbytes - 1;
+	if ((cmd != 't') && (cmd != 'T') && (cmd != 'r') && (cmd != 'R'))
 		goto rx_out_nack;
-	}
 
 	if (cmd & 0x20) /* tiny chars 'r' 't' => SFF */
 		ptr = 4; /* dlc position tiiid */
 	else
 		ptr = 9; /* dlc position Tiiiiiiiid */
 
+	/* the CAN ID has to fit into the current SLCAN message */
+	if (ptr > msglen)
+		goto rx_out_nack;
+
 	memset(&frame.data, 0, 8); /* clear data[] */
 
 	if ((cmd | 0x20) == 'r' && buf[ptr] != '0') {
@@ -249,7 +242,6 @@ rx_restart:
 
 		buf[ptr] = frame.can_dlc; /* restore following byte */
 		frame.can_dlc = 0;
-		ptr--; /* we have no dlc component in the violation case */
 
 	} else {
 		if (!(buf[ptr] >= '0' && buf[ptr] < '9'))
@@ -277,9 +269,6 @@ rx_restart:
 				goto rx_out_nack;
 			frame.data[i] |= tmp;
 		}
-		/* point to last real data */
-		if (frame.can_dlc)
-			ptr--;
 	}
 
 	ret = write(socket, &frame, sizeof(frame));
@@ -303,8 +292,8 @@ rx_out:
 	}
 
 	/* check if there is another command in this buffer */
-	if (nbytes > ptr + 1) {
-		for (tmp = 0, ptr++; ptr + tmp < nbytes; tmp++)
+	if (nbytes > msglen + 1) {
+		for (tmp = 0, ptr = msglen + 1; ptr + tmp < nbytes; tmp++)
 			buf[tmp] = buf[ptr + tmp];
 		nbytes = tmp;
 		goto rx_restart;