fsm: centralize checks on the frame

We check the frame length for the various message type in a single place, thus removing all checks around (that didn't cover all cases anyways). Also, we check for protocol version 2, something that we never did. Signed-off-by: Alessandro Rubini <rubini@gnudd.com>

fsm: centralize checks on the frame
We check the frame length for the various message type in a single place, thus removing all checks around (that didn't cover all cases anyways). Also, we check for protocol version 2, something that we never did. Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
cc014e83 · Alessandro Rubini · 168655e6 · cc014e83 · cc014e83 · cc014e83
Commit cc014e83 authored Mar 04, 2016 by Alessandro Rubini
6 changed files
--- a/arch-sim/main-loop.c
+++ b/arch-sim/main-loop.c
@@ -84,12 +84,6 @@ void sim_main_loop(struct pp_globals *ppg)
 						PP_MAX_FRAME_LENGTH - 4,
 						&ppi->last_rcv_time);
-			if (i < PP_MINIMUM_LENGTH) {
-				pp_diag(ppi, frames, 1,	"Error or short frame: "
-					"%d < %d\n", i,	PP_MINIMUM_LENGTH);
-				continue;
-			}
 			sim_set_global_DS(ppi);
 			tmp_ns = 1000LL * 1000LL * pp_state_machine(ppi,
 					ppi->rx_ptp, i - ppi->rx_offset);

--- a/arch-unix/main-loop.c
+++ b/arch-unix/main-loop.c
@@ -116,12 +116,6 @@ void unix_main_loop(struct pp_globals *ppg)
 						errno, strerror(errno));
 					continue;
 				}
-				if (i < PP_MINIMUM_LENGTH) {
-					pp_diag(ppi, frames, 1,
-						"Short frame: %d < %d\n", i,
-						PP_MINIMUM_LENGTH);
-					continue;
-				}
 				tmp_d = pp_state_machine(ppi, ppi->rx_ptp,
 					i - ppi->rx_offset);

--- a/arch-wrs/main-loop.c
+++ b/arch-wrs/main-loop.c
@@ -157,12 +157,6 @@ void wrs_main_loop(struct pp_globals *ppg)
 						errno, strerror(errno));
 					continue;
 				}
-				if (i < PP_MINIMUM_LENGTH) {
-					pp_diag(ppi, frames, 1,
-						"Short frame: %d < %d\n", i,
-						PP_MINIMUM_LENGTH);
-					continue;
-				}
 				tmp_d = pp_state_machine(ppi, ppi->rx_ptp,
 					i - ppi->rx_offset);

--- a/fsm.c
+++ b/fsm.c
@@ -157,6 +157,33 @@ static int pp_packet_prefilter(struct pp_instance *ppi)
 	return 0;
 }
+/* used to make basic checks before the individual state is called */
+static int type_length[__PP_NR_MESSAGES_TYPES] = {
+	[PPM_SYNC]		= PP_SYNC_LENGTH,
+	[PPM_DELAY_REQ]		= PP_DELAY_REQ_LENGTH,
+	[PPM_PDELAY_REQ]	= PP_PDELAY_REQ_LENGTH,
+	[PPM_PDELAY_RESP]	= PP_PDELAY_RESP_LENGTH,
+	[PPM_FOLLOW_UP]		= PP_FOLLOW_UP_LENGTH,
+	[PPM_DELAY_RESP]	= PP_DELAY_RESP_LENGTH,
+	[PPM_PDELAY_R_FUP]	= PP_PDELAY_R_FUP_LENGTH,
+	[PPM_ANNOUNCE]		= PP_ANNOUNCE_LENGTH,
+	[PPM_SIGNALING]		=   PP_HEADER_LENGTH,
+	[PPM_MANAGEMENT]	= PP_MANAGEMENT_LENGTH,
+};
+static int fsm_unpack_verify_frame(struct pp_instance *ppi,
+				   uint8_t *packet, int plen)
+{
+	int msgtype;
+	msgtype = packet[0] & 0xf;
+	if (msgtype >= __PP_NR_MESSAGES_TYPES || plen < type_length[msgtype])
+		return 1; /* too short */
+	if ((packet[1] & 0xf) != 2)
+		return 1; /* wrong ptp version */
+	return msg_unpack_header(ppi, packet, plen);
+}
 /*
 * This is the state machine code. i.e. the extension-independent
 * function that runs the machine. Errors are managed and reported
@@ -190,13 +217,15 @@ int pp_state_machine(struct pp_instance *ppi, uint8_t *packet, int plen)
 	}
 	/*
-	 * Since all ptp frames have the same header, parse it now.
+	 * Since all ptp frames have the same header, parse it now,
+	 * and centralize some error check.
 	 * In case of error continue without a frame, so the current
 	 * ptp state can update ppi->next_delay and return a proper value
 	 */
-	if (plen && msg_unpack_header(ppi, packet, plen)) {
+	err = fsm_unpack_verify_frame(ppi, packet, plen);
-		packet = NULL;
+	if (err) {
 		plen = 0;
+		packet = NULL;
 	}
 	if (!plen)
 		ppi->received_ptp_header.messageType = PPM_NO_MESSAGE;

--- a/proto-standard/common-fun.c
+++ b/proto-standard/common-fun.c
@@ -144,9 +144,6 @@ static void st_com_add_foreign(struct pp_instance *ppi, unsigned char *buf)
 int st_com_slave_handle_announce(struct pp_instance *ppi, unsigned char *buf,
 				 int len)
 {
-	if (len < PP_ANNOUNCE_LENGTH)
-		return -1;
 	/* st_com_add_foreign takes care of announce unpacking */
 	st_com_add_foreign(ppi, buf);
@@ -168,8 +165,6 @@ int st_com_slave_handle_sync(struct pp_instance *ppi, unsigned char *buf,
 	MsgHeader *hdr = &ppi->received_ptp_header;
 	MsgSync sync;
-	if (len < PP_SYNC_LENGTH)
-		return -1;
 	if (!(ppi->flags & PPI_FLAG_FROM_CURRENT_PARENT))
 		return 0;
@@ -200,9 +195,6 @@ int st_com_peer_handle_pres(struct pp_instance *ppi, unsigned char *buf,
 	MsgHeader *hdr = &ppi->received_ptp_header;
 	int e = 0;
-	if (len < PP_PDELAY_RESP_LENGTH)
-		return -1;
 	msg_unpack_pdelay_resp(buf, &resp);
 	if ((memcmp(&DSPOR(ppi)->portIdentity.clockIdentity,
@@ -294,9 +286,6 @@ int st_com_peer_handle_preq(struct pp_instance *ppi, unsigned char *buf,
 {
 	int e = 0;
-	if (len < PP_PDELAY_REQ_LENGTH)
-		return -1;
 	if (pp_hooks.handle_preq)
 		e = pp_hooks.handle_preq(ppi);
 	if (e)
@@ -318,9 +307,6 @@ int st_com_slave_handle_followup(struct pp_instance *ppi, unsigned char *buf,
 	MsgHeader *hdr = &ppi->received_ptp_header;
-	if (len < PP_FOLLOW_UP_LENGTH)
-		return -1;
 	if (!(ppi->flags & PPI_FLAG_FROM_CURRENT_PARENT)) {
 		pp_error("%s: Follow up message is not from current parent\n",
 			__func__);
@@ -367,9 +353,6 @@ int st_com_slave_handle_followup(struct pp_instance *ppi, unsigned char *buf,
 int st_com_master_handle_announce(struct pp_instance *ppi, unsigned char *buf,
 				  int len)
 {
-	if (len < PP_ANNOUNCE_LENGTH)
-		return -1;
 	pp_diag(ppi, bmc, 2, "Announce message from another foreign master\n");
 	st_com_add_foreign(ppi, buf);

--- a/proto-standard/state-slave.c
+++ b/proto-standard/state-slave.c
@@ -35,9 +35,6 @@ static int slave_handle_response(struct pp_instance *ppi, unsigned char *pkt,
 	MsgHeader *hdr = &ppi->received_ptp_header;
 	MsgDelayResp resp;
-	if (plen < PP_DELAY_RESP_LENGTH)
-		return 0;
 	msg_unpack_delay_resp(pkt, &resp);
 	if ((memcmp(&DSPOR(ppi)->portIdentity.clockIdentity,