MQTT: Fixed MQTT header constructing and parsing

lkaino · hedeshianaws · commit 0829b191972b · 2017-09-27T12:42:03.000-07:00
Bit fields must not be used in communication protocol frame parsing. See http://stackoverflow.com/questions/1490092/c-c-force-bit-field-order-and-alignment. The bit-fields used in AWS C library caused issues in PowerPC target I'm using. This might not be the most beautiful way to fix it, but it works. Tested on x86 and PowerPC architectures. fixes #59
diff --git a/include/aws_iot_mqtt_client_common_internal.h b/include/aws_iot_mqtt_client_common_internal.h
@@ -69,26 +69,17 @@ typedef enum msgTypes {
 	DISCONNECT = 14
 } MessageTypes;
 
+/* Macros for parsing header fields from incoming MQTT frame. */
+#define MQTT_HEADER_FIELD_TYPE(_byte)	((_byte >> 4) & 0x0F)
+#define MQTT_HEADER_FIELD_DUP(_byte)	((_byte & (1 << 3)) >> 3)
+#define MQTT_HEADER_FIELD_QOS(_byte)	((_byte & (3 << 1)) >> 1)
+#define MQTT_HEADER_FIELD_RETAIN(_byte)	((_byte & (1 << 0)) >> 0)
+
 /**
  * Bitfields for the MQTT header byte.
  */
 typedef union {
 	unsigned char byte;				/**< the whole byte */
-#if defined(REVERSED)
-	struct {
-		unsigned int type : 4;		/**< message type nibble */
-		unsigned int dup : 1;		/**< DUP flag bit */
-		unsigned int qos : 2;		/**< QoS value, 0, 1 or 2 */
-		unsigned int retain : 1;	/**< retained flag bit */
-	} bits;
-#else
-	struct {
-		unsigned int retain : 1;	/**< retained flag bit */
-		unsigned int qos : 2;		/**< QoS value, 0, 1 or 2 */
-		unsigned int dup : 1;		/**< DUP flag bit */
-		unsigned int type : 4;		/**< message type nibble */
-	} bits;
-#endif
 } MQTTHeader;
 
 IoT_Error_t aws_iot_mqtt_internal_init_header(MQTTHeader *pHeader, MessageTypes message_type,
diff --git a/src/aws_iot_mqtt_client_common_internal.c b/src/aws_iot_mqtt_client_common_internal.c
@@ -194,72 +194,73 @@ IoT_Error_t aws_iot_mqtt_internal_init_header(MQTTHeader *pHeader, MessageTypes
 
 	/* Set all bits to zero */
 	pHeader->byte = 0;
+	uint8_t type = 0;
 	switch(message_type) {
 		case UNKNOWN:
 			/* Should never happen */
 			return FAILURE;
 		case CONNECT:
-			pHeader->bits.type = 0x01;
+			type = 0x01;
 			break;
 		case CONNACK:
-			pHeader->bits.type = 0x02;
+			type = 0x02;
 			break;
 		case PUBLISH:
-			pHeader->bits.type = 0x03;
+			type = 0x03;
 			break;
 		case PUBACK:
-			pHeader->bits.type = 0x04;
+			type = 0x04;
 			break;
 		case PUBREC:
-			pHeader->bits.type = 0x05;
+			type = 0x05;
 			break;
 		case PUBREL:
-			pHeader->bits.type = 0x06;
+			type = 0x06;
 			break;
 		case PUBCOMP:
-			pHeader->bits.type = 0x07;
+			type = 0x07;
 			break;
 		case SUBSCRIBE:
-			pHeader->bits.type = 0x08;
+			type = 0x08;
 			break;
 		case SUBACK:
-			pHeader->bits.type = 0x09;
+			type = 0x09;
 			break;
 		case UNSUBSCRIBE:
-			pHeader->bits.type = 0x0A;
+			type = 0x0A;
 			break;
 		case UNSUBACK:
-			pHeader->bits.type = 0x0B;
+			type = 0x0B;
 			break;
 		case PINGREQ:
-			pHeader->bits.type = 0x0C;
+			type = 0x0C;
 			break;
 		case PINGRESP:
-			pHeader->bits.type = 0x0D;
+			type = 0x0D;
 			break;
 		case DISCONNECT:
-			pHeader->bits.type = 0x0E;
+			type = 0x0E;
 			break;
 		default:
 			/* Should never happen */
 		FUNC_EXIT_RC(FAILURE);
 	}
 
-	pHeader->bits.dup = (1 == dup) ? 0x01 : 0x00;
+	pHeader->byte = type << 4;
+	pHeader->byte |= dup << 3;
+
 	switch(qos) {
 		case QOS0:
-			pHeader->bits.qos = 0x00;
 			break;
 		case QOS1:
-			pHeader->bits.qos = 0x01;
+			pHeader->byte |= 1 << 1;
 			break;
 		default:
 			/* Using QOS0 as default */
-			pHeader->bits.qos = 0x00;
 			break;
 	}
 
-	pHeader->bits.retain = (1 == retained) ? 0x01 : 0x00;
+	pHeader->byte |= (1 == retained) ? 0x01 : 0x00;
 
 	FUNC_EXIT_RC(SUCCESS);
 }
@@ -414,7 +415,7 @@ static IoT_Error_t _aws_iot_mqtt_internal_read_packet(AWS_IoT_Client *pClient, T
 	}
 
 	header.byte = pClient->clientData.readBuf[0];
-	*pPacketType = header.bits.type;
+	*pPacketType = MQTT_HEADER_FIELD_TYPE(header.byte);
 
 	FUNC_EXIT_RC(rc);
 }
diff --git a/src/aws_iot_mqtt_client_connect.c b/src/aws_iot_mqtt_client_connect.c
@@ -194,19 +194,24 @@ static IoT_Error_t _aws_iot_mqtt_serialize_connect(unsigned char *pTxBuf, size_t
 	//}
 
 	flags.all = 0;
-	flags.bits.cleansession = (pConnectParams->isCleanSession) ? 1 : 0;
-	flags.bits.will = (pConnectParams->isWillMsgPresent) ? 1 : 0;
-	if(flags.bits.will) {
-		flags.bits.willQoS = pConnectParams->will.qos;
-		flags.bits.willRetain = (pConnectParams->will.isRetained) ? 1 : 0;
+	if (pConnectParams->isCleanSession)
+	{
+		flags.all |= 1 << 1;
 	}
 
-	if(pConnectParams->pUsername) {
-		flags.bits.username = 1;
+	if (pConnectParams->isWillMsgPresent)
+	{
+		flags.all |= 1 << 2;
+		flags.all |= pConnectParams->will.qos << 3;
+		flags.all |= pConnectParams->will.isRetained << 5;
 	}
 
 	if(pConnectParams->pPassword) {
-		flags.bits.password = 1;
+		flags.all |= 1 << 6;
+	}
+
+	if(pConnectParams->pUsername) {
+		flags.all |= 1 << 7;
 	}
 
 	aws_iot_mqtt_internal_write_char(&ptr, flags.all);
@@ -225,11 +230,11 @@ static IoT_Error_t _aws_iot_mqtt_serialize_connect(unsigned char *pTxBuf, size_t
 		aws_iot_mqtt_internal_write_utf8_string(&ptr, pConnectParams->will.pMessage, pConnectParams->will.msgLen);
 	}
 
-	if(flags.bits.username) {
+	if(pConnectParams->pUsername) {
 		aws_iot_mqtt_internal_write_utf8_string(&ptr, pConnectParams->pUsername, pConnectParams->usernameLen);
 	}
 
-	if(flags.bits.password) {
+	if(pConnectParams->pPassword) {
 		aws_iot_mqtt_internal_write_utf8_string(&ptr, pConnectParams->pPassword, pConnectParams->passwordLen);
 	}
 
@@ -274,7 +279,7 @@ static IoT_Error_t _aws_iot_mqtt_deserialize_connack(unsigned char *pSessionPres
 	readBytesLen = 0;
 
 	header.byte = aws_iot_mqtt_internal_read_char(&curdata);
-	if(CONNACK != header.bits.type) {
+	if(CONNACK != MQTT_HEADER_FIELD_TYPE(header.byte)) {
 		FUNC_EXIT_RC(FAILURE);
 	}
 
diff --git a/src/aws_iot_mqtt_client_publish.c b/src/aws_iot_mqtt_client_publish.c
@@ -338,13 +338,13 @@ IoT_Error_t aws_iot_mqtt_internal_deserialize_publish(uint8_t *dup, QoS *qos,
 	}
 
 	header.byte = aws_iot_mqtt_internal_read_char(&curData);
-	if(PUBLISH != header.bits.type) {
+	if(PUBLISH != MQTT_HEADER_FIELD_TYPE(header.byte)) {
 		FUNC_EXIT_RC(FAILURE);
 	}
 
-	*dup = header.bits.dup;
-	*qos = (QoS) header.bits.qos;
-	*retained = header.bits.retain;
+	*dup = MQTT_HEADER_FIELD_DUP(header.byte);
+	*qos = (QoS) MQTT_HEADER_FIELD_QOS(header.byte);
+	*retained = MQTT_HEADER_FIELD_RETAIN(header.byte);
 
 	/* read remaining length */
 	rc = aws_iot_mqtt_internal_decode_remaining_length_from_buffer(curData, &decodedLen, &readBytesLen);
@@ -404,8 +404,8 @@ IoT_Error_t aws_iot_mqtt_internal_deserialize_ack(unsigned char *pPacketType, un
 
 
 	header.byte = aws_iot_mqtt_internal_read_char(&curdata);
-	*dup = header.bits.dup;
-	*pPacketType = header.bits.type;
+	*dup = MQTT_HEADER_FIELD_DUP(header.byte);
+	*pPacketType = MQTT_HEADER_FIELD_TYPE(header.byte);
 
 	/* read remaining length */
 	rc = aws_iot_mqtt_internal_decode_remaining_length_from_buffer(curdata, &decodedLen, &readBytesLen);
diff --git a/src/aws_iot_mqtt_client_subscribe.c b/src/aws_iot_mqtt_client_subscribe.c
@@ -140,7 +140,7 @@ static IoT_Error_t _aws_iot_mqtt_deserialize_suback(uint16_t *pPacketId, uint32_
 	}
 
 	header.byte = aws_iot_mqtt_internal_read_char(&curData);
-	if(SUBACK != header.bits.type) {
+	if(SUBACK != MQTT_HEADER_FIELD_TYPE(header.byte)) {
 		FUNC_EXIT_RC(FAILURE);
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ static IoT_Error_t _aws_iot_mqtt_deserialize_suback(uint16_t *pPacketId, uint32_`
`140`	`140`	`}`
`141`	`141`
`142`	`142`	`header.byte = aws_iot_mqtt_internal_read_char(&curData);`
`143`		`- if(SUBACK != header.bits.type) {`
	`143`	`+ if(SUBACK != MQTT_HEADER_FIELD_TYPE(header.byte)) {`
`144`	`144`	`FUNC_EXIT_RC(FAILURE);`
`145`	`145`	`}`
`146`	`146`