Check for empty topic filters in subscribes (#139)

* Validate each topic filter in a subscribe or unsubscribe packet is of nonzero length

source/core_mqtt_serializer.c

@@ -201,7 +201,7 @@ static bool calculatePublishPacketSize( const MQTTPublishInfo_t * pPublishInfo,
  * @param[in] subscriptionType #MQTT_SUBSCRIBE or #MQTT_UNSUBSCRIBE.
  *
  * #MQTTBadParameter if the packet would exceed the size allowed by the
- * MQTT spec; #MQTTSuccess otherwise.
+ * MQTT spec or a subscription is empty; #MQTTSuccess otherwise.
  */
 static MQTTStatus_t calculateSubscriptionPacketSize( const MQTTSubscribeInfo_t * pSubscriptionList,
                                                      size_t subscriptionCount,
@@ -1032,6 +1032,17 @@ static MQTTStatus_t calculateSubscriptionPacketSize( const MQTTSubscribeInfo_t *
         {
             packetSize += 1U;
         }
+
+        /* Validate each topic filter. */
+        if( ( pSubscriptionList[ i ].topicFilterLength == 0U ) ||
+            ( pSubscriptionList[ i ].pTopicFilter == NULL ) )
+        {
+            status = MQTTBadParameter;
+            LogError( ( "Subscription #%lu in %sSUBSCRIBE packet cannot be empty.",
+                        ( unsigned long ) i,
+                        ( subscriptionType == MQTT_SUBSCRIBE ) ? "" : "UN" ) );
+            /* It is not necessary to break as an error status has already been set. */
+        }
     }
 
     /* At this point, the "Remaining length" has been calculated. Return error
@@ -1045,7 +1056,8 @@ static MQTTStatus_t calculateSubscriptionPacketSize( const MQTTSubscribeInfo_t *
                     MQTT_MAX_REMAINING_LENGTH ) );
         status = MQTTBadParameter;
     }
-    else
+
+    if( status == MQTTSuccess )
     {
         *pRemainingLength = packetSize;
 
@@ -1669,19 +1681,12 @@ MQTTStatus_t MQTT_GetSubscribePacketSize( const MQTTSubscribeInfo_t * pSubscript
     }
     else
     {
-        /* Calculate the MQTT UNSUBSCRIBE packet size. */
+        /* Calculate the MQTT SUBSCRIBE packet size. */
         status = calculateSubscriptionPacketSize( pSubscriptionList,
                                                   subscriptionCount,
                                                   pRemainingLength,
                                                   pPacketSize,
                                                   MQTT_SUBSCRIBE );
-
-        if( status == MQTTBadParameter )
-        {
-            LogError( ( "SUBSCRIBE packet remaining length exceeds %lu, which is the "
-                        "maximum size allowed by MQTT 3.1.1.",
-                        MQTT_MAX_REMAINING_LENGTH ) );
-        }
     }
 
     return status;
@@ -1768,19 +1773,12 @@ MQTTStatus_t MQTT_GetUnsubscribePacketSize( const MQTTSubscribeInfo_t * pSubscri
     }
     else
     {
-        /* Calculate the MQTT SUBSCRIBE packet size. */
+        /* Calculate the MQTT UNSUBSCRIBE packet size. */
         status = calculateSubscriptionPacketSize( pSubscriptionList,
                                                   subscriptionCount,
                                                   pRemainingLength,
                                                   pPacketSize,
                                                   MQTT_UNSUBSCRIBE );
-
-        if( status == MQTTBadParameter )
-        {
-            LogError( ( "UNSUBSCRIBE packet remaining length exceeds %lu, which is the "
-                        "maximum size allowed by MQTT 3.1.1.",
-                        MQTT_MAX_REMAINING_LENGTH ) );
-        }
     }
 
     return status;

test/unit-test/core_mqtt_serializer_utest.c

@@ -656,7 +656,7 @@ void test_MQTT_SerializeConnect( void )
 void test_MQTT_GetSubscribePacketSize( void )
 {
     MQTTSubscribeInfo_t subscriptionList;
-    size_t subscriptionCount = 0;
+    size_t subscriptionCount = 1;
     size_t remainingLength = 0;
     size_t packetSize = 0;
     MQTTStatus_t status = MQTTSuccess;
@@ -693,6 +693,22 @@ void test_MQTT_GetSubscribePacketSize( void )
                                           &packetSize );
     TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
 
+    /* Zero length topic filter. */
+    subscriptionCount = 1;
+    status = MQTT_GetSubscribePacketSize( &subscriptionList,
+                                          subscriptionCount,
+                                          &remainingLength,
+                                          &packetSize );
+    TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
+
+    /* NULL topic filter, nonzero length. */
+    subscriptionList.topicFilterLength = 1;
+    status = MQTT_GetSubscribePacketSize( &subscriptionList,
+                                          subscriptionCount,
+                                          &remainingLength,
+                                          &packetSize );
+    TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
+
     /* Verify packet size cannot exceed limit. Note the max remaining length of
      * an MQTT packet is 2^28-1 = 268435455, or 256MiB. Since the only way to increase
      * the subscribe packet size is with the topic filters of the subscriptions
@@ -701,6 +717,10 @@ void test_MQTT_GetSubscribePacketSize( void )
     for( i = 0; i < 4096; i++ )
     {
         fourThousandSubscriptions[ i ].topicFilterLength = UINT16_MAX;
+
+        /* We need to set this to avoid an early bad parameter, however we do
+         * not need a 65535 byte buffer as the packet will not be serialized. */
+        fourThousandSubscriptions[ i ].pTopicFilter = "";
     }
 
     subscriptionCount = sizeof( fourThousandSubscriptions ) / sizeof( fourThousandSubscriptions[ 0 ] );
@@ -730,7 +750,7 @@ void test_MQTT_GetSubscribePacketSize( void )
 void test_MQTT_GetUnsubscribePacketSize( void )
 {
     MQTTSubscribeInfo_t subscriptionList;
-    size_t subscriptionCount = 0;
+    size_t subscriptionCount = 1;
     size_t remainingLength = 0;
     size_t packetSize = 0;
     MQTTStatus_t status = MQTTSuccess;
@@ -767,6 +787,22 @@ void test_MQTT_GetUnsubscribePacketSize( void )
                                             &packetSize );
     TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
 
+    /* Zero length topic filter. */
+    subscriptionCount = 1;
+    status = MQTT_GetUnsubscribePacketSize( &subscriptionList,
+                                            subscriptionCount,
+                                            &remainingLength,
+                                            &packetSize );
+    TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
+
+    /* NULL topic filter, nonzero length. */
+    subscriptionList.topicFilterLength = 1;
+    status = MQTT_GetUnsubscribePacketSize( &subscriptionList,
+                                            subscriptionCount,
+                                            &remainingLength,
+                                            &packetSize );
+    TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
+
     /* Verify packet size cannot exceed limit. Note the max remaining length of
      * an MQTT packet is 2^28-1 = 268435455, or 256MiB. Since the only way to increase
      * the subscribe packet size is with the topic filters of the subscriptions
@@ -775,6 +811,10 @@ void test_MQTT_GetUnsubscribePacketSize( void )
     for( i = 0; i < 4096; i++ )
     {
         fourThousandSubscriptions[ i ].topicFilterLength = UINT16_MAX;
+
+        /* We need to set this to avoid an early bad parameter, however we do
+         * not need a 65535 byte buffer as the packet will not be serialized. */
+        fourThousandSubscriptions[ i ].pTopicFilter = "";
     }
 
     subscriptionCount = sizeof( fourThousandSubscriptions ) / sizeof( fourThousandSubscriptions[ 0 ] );