CR comments and fixes

1 files changed, 47 insertions, 32 deletions

diff --git a/src/lib/protocols/SOMEIP.c b/src/lib/protocols/SOMEIP.c
index 9c12343e8..a0d347417 100644
--- a/src/lib/protocols/SOMEIP.c
+++ b/src/lib/protocols/SOMEIP.c
@@ -24,11 +24,7 @@
 #include "ndpi_protocols.h"
 #ifdef NDPI_PROTOCOL_SOMEIP
 
-// CR: these MQTT references are no longer relevant, rigth?
-/**
- * The type of control messages in mqtt version 3.1.1
- * see http://docs.oasis-open.org/mqtt/mqtt/v3.1.1
- */
+// CR: these MQTT references are no longer relevant, rigth? ANS: true.
 enum SOMEIP_MESSAGE_TYPES {
 	REQUEST = 0x00,
 	REQUEST_NO_RETURN = 0x01,
@@ -53,7 +49,8 @@ enum SOMEIP_RETURN_CODES {
 	E_WRONG_PROTOCOL_VERSION = 0x07,
 	E_WRONG_INTERFACE_VERSION = 0x08,
 	E_MALFORMED_MESSAGE = 0x09,
-	E_WRONG_MESSAGE_TYPE = 0x0a
+	E_WRONG_MESSAGE_TYPE = 0x0a,
+	E_RETURN_CODE_LEGAL_THRESHOLD = 0x40  //return codes from 0x40 (inclusive) and upwards are illegal.
 };
 
 enum SPECIAL_MESSAGE_IDS {
@@ -62,6 +59,21 @@ enum SPECIAL_MESSAGE_IDS {
 	MSG_SD = 0xffff8100
 };
 
+enum PROTOCOL_VERSION{
+	LEGAL_PROTOCOL_VERSION = 0x01
+};
+
+enum MAGIC_COOKIE_CONSTANTS{
+	MC_REQUEST_ID = 0xDEADBEEF,
+	MC_LENGTH = 0x08,
+	MC_INTERFACE_VERSION = 0x01
+};
+
+enum DEFAULT_PROTOCOL_PORTS{
+	PORT_DEFAULT_CLIENT = 30491,
+	PORT_DEFAULT_SERVER = 30501,
+	PORT_DEFAULT_SD = 30490
+};
 
 /**
  * Entry point when protocol is identified.
@@ -83,12 +95,12 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
 	//####Maybe check carrier protocols?####
 
 	NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "SOME/IP search called...\n");
-	// CR: can packet be const?
-	struct ndpi_packet_struct *packet = &flow->packet;
+	// CR: can packet be const? ANS: Probably yeah, needs testing but I changed it.
+	struct const ndpi_packet_struct *packet = &flow->packet;
 	if (packet->detected_protocol_stack[0] != NDPI_PROTOCOL_UNKNOWN) {
 		return;
 	}
-	// CR: let's reach a decision in this issue.
+	// CR: let's reach a decision in this issue. ANS: I think it's unnecessary and would get dropped on length checks or whatever, so we can remove this.
 
 	/*NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "SOME/IP detection...\n");
 	if (flow->packet_counter > 10) {
@@ -100,8 +112,8 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
 	*/
 
 	//we extract the Message ID and Request ID and check for special cases later
-	u_int32_t message_id = (u_int32_t) ((packet->payload[0]<<24)+(packet->payload[1]<<16)+(packet->payload[2]<<8)+packet->payload[3]);
-	u_int32_t request_id = (u_int32_t) ((packet->payload[8]<<24)+(packet->payload[9]<<16)+(packet->payload[10]<<8)+packet->payload[11]);
+	u_int32_t message_id = ntohl(*((u_int32_t *)packet->payload[0]));
+	u_int32_t request_id = ntohl(*((u_int32_t *)packet->payload[8]));
 
 	NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "====>>>> SOME/IP Message ID: %08x [len: %u]\n",
 			message_id, packet->payload_packet_len);
@@ -119,8 +131,8 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
 	*/
 
 	// we extract the remaining length
-	// CR: cast the payload to unsigned int, then use ntohl
-	u_int32_t someip_len = (u_int32_t) ((packet->payload[4]<<24) + (packet->payload[5]<<16) + (packet->payload[6]<<8) +packet->payload[7]);
+	// CR: cast the payload to unsigned int, then use ntohl ANS: done
+	u_int32_t someip_len = ntohl(*((u_int32_t *)packet->payload[4]));
 	if (packet->payload_packet_len != (someip_len + 8)) {
 		NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "Excluding SOME/IP .. Length field invalid!\n");
 		NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_SOMEIP);
@@ -129,8 +141,8 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
 
 	u_int8_t protocol_version = (u_int8_t) (packet->payload[12]);
 	NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG,"====>>>> SOME/IP protocol version: [%d]\n",protocol_version);
-	// CR: don't use magic numbers, convert this to a constant instead
-	if (protocol_version != 0x01){
+	// CR: don't use magic numbers, convert this to a constant instead ANS: done
+	if (protocol_version != LEGAL_PROTOCOL_VERSION){
 		NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "Excluding SOME/IP .. invalid protocol version!\n");
 		NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_SOMEIP);
 		return;
@@ -141,9 +153,10 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
 	u_int8_t message_type = (u_int8_t) (packet->payload[14]);
 	NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG,"====>>>> SOME/IP message type: [%d]\n",message_type);
 
-	// CR: don't use magic numbers, convert these to constants instead
-	if ((message_type != 0x00) && (message_type != 0x01) && (message_type != 0x02) && (message_type != 0x40) && (message_type != 0x41) && 
-					(message_type != 0x42) && (message_type != 0x80) && (message_type != 0x81) && (message_type != 0xc0) && (message_type != 0xc1)) {
+	// CR: don't use magic numbers, convert these to constants instead ANS: done
+	if ((message_type != REQUEST) && (message_type != REQUEST_NO_RETURN) && (message_type != NOTIFICATION) && (message_type != REQUEST_ACK) && 
+					(message_type != REQUEST_NO_RETURN_ACK) && (message_type != NOTIFICATION_ACK) && (message_type != RESPONSE) && 
+					(message_type != ERROR) && (message_type != RESPONSE_ACK) && (message_type != ERROR_ACK)) {
 		NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "Excluding SOME/IP .. invalid message type!\n");
 		NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_SOMEIP);
 		return;
@@ -151,17 +164,17 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
 
 	u_int8_t return_code = (u_int8_t) (packet->payload[15]);
 	NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG,"====>>>> SOME/IP return code: [%d]\n", return_code);
-	// CR: don't use magic numbers, convert this to a constant instead
-	if ((return_code > 0x3f)) {
+	// CR: don't use magic numbers, convert this to a constant instead ANS: done
+	if ((return_code >= E_RETURN_CODE_LEGAL_THRESHOLD)) {
 		NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "Excluding SOME/IP .. invalid return code!\n");
 		NDPI_ADD_PROTOCOL_TO_BITMASK(flow->excluded_protocol_bitmask, NDPI_PROTOCOL_SOMEIP);
 		return;
 	}
 	
  	if (message_id == MSG_MAGIC_COOKIE){
-		// CR: don't use magic numbers, convert these to constants instead
-		if ((someip_len == 0x08) && (request_id == 0xDEADBEEF) && (interface_version == 0x01) &&
-					(message_type == 0x01) && (return_code == 0x00)){
+		// CR: don't use magic numbers, convert these to constants instead ANS:done
+		if ((someip_len == MC_LENGTH) && (request_id == MC_REQUEST_ID) && (interface_version == MC_INTERFACE_VERSION) &&
+					(message_type == REQUEST_NO_RETURN\) && (return_code == E_OK)){
 			NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "SOME/IP found Magic Cookie\n",message_type);
 			ndpi_int_someip_add_connection(ndpi_struct, flow);
 			return;
@@ -174,9 +187,9 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
  	}
 	
 	if (message_id == MSG_MAGIC_COOKIE_ACK){
-		// CR: don't use magic numbers, convert these to constants instead
-		if ((someip_len == 0x08) && (request_id == 0xDEADBEEF) && (interface_version == 0x01) &&
-					(message_type == 0x02) && (return_code == 0x00)){
+		// CR: don't use magic numbers, convert these to constants instead ANS: done
+		if ((someip_len == MC_LENGTH) && (request_id == MC_REQUEST_ID) && (interface_version == MC_INTERFACE_VERSION\) &&
+					(message_type == REQUEST_NO_RETURN) && (return_code == E_OK)){
 			NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "SOME/IP found Magic Cookie ACK\n",message_type);
 			ndpi_int_someip_add_connection(ndpi_struct, flow);
 			return;
@@ -189,24 +202,26 @@ void ndpi_search_someip (struct ndpi_detection_module_struct *ndpi_struct,
  	}
 
 	if (message_id == MSG_SD){
-		// CR: let's talk about this (i.e. what should be here right now? what documentation should we leave behind?)
+		// CR: let's talk about this (i.e. what should be here right now? what documentation should we leave behind?) ANS: a TON of stuff. SD is basically another protocol built ontop SOMEIP. at the very least I expect it to be as long as everything else we've done already.
 		//####Service Discovery message. Fill in later!####
 	}
 
 	// CR: while this is for demo purposes, the port numbers are as specified in the SOME/IP document, so we should change the
-	// comment to reflect this.
-	// Also, don't use magic numbers, use constants.
+	// comment to reflect this. ANS: done
+	// Also, don't use magic numbers, use constants. ANS: done
 
-	//Filtering by port as per request. This is PURELY for demo purposes and the rest of the check must be filled in later on!
+	//Filtering by port. 
+	//This check is NOT a 100% thing - these ports are mentioned in the documentation but the documentation also states they haven't been approved by IANA yet, and that the user is free to use different ports.
+	//This is is PURELY for demo purposes and the rest of the check must be filled in later on!
 	if (packet->l4_protocol == IPPROTO_UDP){
-		if ((packet->udp->dest == ntohs(30491)) || (packet->udp->dest == ntohs(30501)) || (packet->udp->dest == ntohs(30490))) {
+		if ((packet->udp->dest == ntohs(PORT_DEFAULT_CLIENT)) || (packet->udp->dest == ntohs(PORT_DEFAULT_SERVER)) || (packet->udp->dest == ntohs(PORT_DEFAULT_SD))) {
 			NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "SOME/IP found\n",message_type);
 			ndpi_int_someip_add_connection(ndpi_struct, flow);
 			return;
 		}
 	}
 	if (packet->l4_protocol == IPPROTO_TCP){
-		if ((packet->tcp->dest == ntohs(30491)) || (packet->tcp->dest == ntohs(30501))) {
+		if ((packet->tcp->dest == ntohs(PORT_DEFAULT_CLIENT)) || (packet->tcp->dest == ntohs(PORT_DEFAULT_SERVER))) {
 			NDPI_LOG(NDPI_PROTOCOL_SOMEIP, ndpi_struct, NDPI_LOG_DEBUG, "SOME/IP found\n",message_type);
 			ndpi_int_someip_add_connection(ndpi_struct, flow);
 			return;