Remove restriction on LWT payload being zero (#221)

AniruddhaKanhere · web-flow · commit 25b496854be0 · 2022-09-22T10:34:17.000-07:00
* Update the changelog * Remove payload non-zero restriction Ref: http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718031 * Update changelog * Fix 10.4 violation * FRemove unused files * Update memory table * Add assert to check invalid conditions * Update the assertion to be correct * Fix last CBMC proof by making sure all pointers are allocated properly
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,9 @@
 # Changelog for coreMQTT Client Library
 
 ## v2.0.0 (September 2022)
+
+### Changes
+- [#221](https://github.com/FreeRTOS/coreMQTT/pull/221) Remove LWT payload non-zero restriction.
 - [#219](https://github.com/FreeRTOS/coreMQTT/pull/219) Fix MISRA deviations in the source.
 - [#218](https://github.com/FreeRTOS/coreMQTT/pull/218) Fix bugs in receiveSingleIteration and optimize sendMessageVector.
 - [#213](https://github.com/FreeRTOS/coreMQTT/pull/213) Fix MISRA deviations in the source.
diff --git a/docs/doxygen/include/size_table.md b/docs/doxygen/include/size_table.md
@@ -9,7 +9,7 @@
     </tr>
     <tr>
         <td>core_mqtt.c</td>
-        <td><center>3.9K</center></td>
+        <td><center>4.0K</center></td>
         <td><center>3.4K</center></td>
     </tr>
     <tr>
@@ -24,7 +24,7 @@
     </tr>
     <tr>
         <td><b>Total estimates</b></td>
-        <td><b><center>8.4K</center></b></td>
+        <td><b><center>8.5K</center></b></td>
         <td><b><center>6.9K</center></b></td>
     </tr>
 </table>
diff --git a/source/core_mqtt.c b/source/core_mqtt.c
@@ -1814,20 +1814,30 @@ static size_t addEncodedStringToVector( uint8_t serailizedLength[ 2 ],
     const size_t seralizedLengthFieldSize = 2U;
     TransportOutVector_t * pLocalIterator = iterator;
     /* This function always adds 2 vectors. */
-    const size_t vectorsAdded = 2U;
+    size_t vectorsAdded = 0U;
+
+    /* When length is non-zero, the string must be non-NULL. */
+    assert( ( length != 0U ) == ( string != NULL ) );
 
     serailizedLength[ 0 ] = ( ( uint8_t ) ( ( length ) >> 8 ) );
     serailizedLength[ 1 ] = ( ( uint8_t ) ( ( length ) & 0x00ffU ) );
 
     /* Add the serialized length of the string first. */
     pLocalIterator[ 0 ].iov_base = serailizedLength;
     pLocalIterator[ 0 ].iov_len = seralizedLengthFieldSize;
+    vectorsAdded++;
+    packetLength = seralizedLengthFieldSize;
 
-    /* Then add the pointer to the string itself. */
-    pLocalIterator[ 1 ].iov_base = string;
-    pLocalIterator[ 1 ].iov_len = length;
-
-    packetLength = length + seralizedLengthFieldSize;
+    /* Sometimes the string can be NULL that is, of 0 length. In that case,
+     * only the length field should be encoded in the vector. */
+    if( ( string != NULL ) && ( length != 0U ) )
+    {
+        /* Then add the pointer to the string itself. */
+        pLocalIterator[ 1 ].iov_base = string;
+        pLocalIterator[ 1 ].iov_len = length;
+        vectorsAdded++;
+        packetLength += length;
+    }
 
     ( *updatedLength ) = ( *updatedLength ) + packetLength;
 
@@ -2099,11 +2109,6 @@ static MQTTStatus_t sendConnectWithoutCopy( MQTTContext_t * pContext,
         LogError( ( "pWillInfo->pTopicName cannot be NULL if Will is present." ) );
         status = MQTTBadParameter;
     }
-    else if( ( pWillInfo != NULL ) && ( pWillInfo->pPayload == NULL ) )
-    {
-        LogError( ( "pWillInfo->pPayload cannot be NULL if Will is present." ) );
-        status = MQTTBadParameter;
-    }
     else
     {
         pIndex = MQTT_SerializeConnectFixedHeader( pIndex,
@@ -2149,7 +2154,7 @@ static MQTTStatus_t sendConnectWithoutCopy( MQTTContext_t * pContext,
             ioVectorLength += vectorsAdded;
 
 
-            /* Serialize the payload. */
+            /* Serialize the payload. Payload of last will and testament can be NULL. */
             vectorsAdded = addEncodedStringToVector( serializedPayloadLength,
                                                      pWillInfo->pPayload,
                                                      ( uint16_t ) pWillInfo->payloadLength,
diff --git a/test/cbmc/proofs/MQTT_Connect/MQTT_Connect_harness.c b/test/cbmc/proofs/MQTT_Connect/MQTT_Connect_harness.c
@@ -54,6 +54,33 @@ void harness()
         totalMessageLength += pConnectInfo->passwordLength;
         totalMessageLength += pConnectInfo->userNameLength;
         totalMessageLength += pConnectInfo->clientIdentifierLength;
+
+        if( pConnectInfo->passwordLength == 0 )
+        {
+            pConnectInfo->pPassword = NULL;
+        }
+        else
+        {
+            __CPROVER_assume( pConnectInfo->pPassword != NULL );
+        }
+
+        if( pConnectInfo->userNameLength == 0 )
+        {
+            pConnectInfo->pUserName = NULL;
+        }
+        else
+        {
+            __CPROVER_assume( pConnectInfo->pUserName != NULL );
+        }
+
+        if( pConnectInfo->clientIdentifierLength == 0 )
+        {
+            pConnectInfo->pClientIdentifier = NULL;
+        }
+        else
+        {
+            __CPROVER_assume( pConnectInfo->pClientIdentifier != NULL );
+        }
     }
 
     pWillInfo = allocateMqttPublishInfo( NULL );
@@ -65,6 +92,24 @@ void harness()
         __CPROVER_assume( pWillInfo->topicNameLength < 268435456 );
         __CPROVER_assume( pWillInfo->payloadLength < 268435456 );
 
+        if( pWillInfo->topicNameLength == 0 )
+        {
+            pWillInfo->pTopicName = NULL;
+        }
+        else
+        {
+            __CPROVER_assume( pWillInfo->pTopicName != NULL );
+        }
+
+        if( pWillInfo->payloadLength == 0 )
+        {
+            pWillInfo->pPayload = NULL;
+        }
+        else
+        {
+            __CPROVER_assume( pWillInfo->pPayload != NULL );
+        }
+
         totalMessageLength += pWillInfo->topicNameLength;
         totalMessageLength += pWillInfo->payloadLength;
     }
diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
@@ -1059,7 +1059,7 @@ void test_MQTT_Connect_ProperWIllInfo( void )
 /**
  * @brief Test MQTT_Connect, with no payload in will message.
  */
-void test_MQTT_Connect_ImproperWIllInfo( void )
+void test_MQTT_Connect_ProperWIllInfoWithNoPayload( void )
 {
     MQTTContext_t mqttContext = { 0 };
     MQTTConnectInfo_t connectInfo = { 0 };
@@ -1107,7 +1107,64 @@ void test_MQTT_Connect_ImproperWIllInfo( void )
 
     status = MQTT_Connect( &mqttContext, &connectInfo, &willInfo, timeout, &sessionPresent );
 
-    TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status );
+    TEST_ASSERT_EQUAL_INT( MQTTSendFailed, status );
+}
+
+/**
+ * @brief Test MQTT_Connect, with payload in will message but the length is
+ * zero.
+ */
+void test_MQTT_Connect_ProperWIllInfoWithPayloadButZeroLength( void )
+{
+    MQTTContext_t mqttContext = { 0 };
+    MQTTConnectInfo_t connectInfo = { 0 };
+    uint32_t timeout = 2;
+    bool sessionPresent;
+    MQTTStatus_t status;
+    TransportInterface_t transport = { 0 };
+    MQTTFixedBuffer_t networkBuffer = { 0 };
+    size_t remainingLength;
+    size_t packetSize;
+    MQTTPublishInfo_t willInfo;
+
+    setupTransportInterface( &transport );
+    transport.writev = transportWritevFail;
+    setupNetworkBuffer( &networkBuffer );
+
+    memset( &mqttContext, 0x0, sizeof( mqttContext ) );
+    memset( &willInfo, 0, sizeof( MQTTPublishInfo_t ) );
+
+    willInfo.pTopicName = "MQTTTopic";
+    willInfo.topicNameLength = strlen( willInfo.pTopicName );
+    willInfo.pPayload = "Payload";
+    willInfo.payloadLength = 0;
+
+    connectInfo.pUserName = "MQTTUser";
+    connectInfo.userNameLength = strlen( connectInfo.pUserName );
+    connectInfo.pPassword = "NotSafePassword";
+    connectInfo.passwordLength = strlen( connectInfo.pPassword );
+
+    MQTT_Init( &mqttContext, &transport, getTime, eventCallback, &networkBuffer );
+
+    /* Transport send failed when sending CONNECT. */
+
+    /* Choose 10 bytes variable header + 1 byte payload for the remaining
+     * length of the CONNECT. The packet size needs to be nonzero for this test
+     * as that is the amount of bytes used in the call to send the packet. */
+    packetSize = 13;
+    remainingLength = 11;
+    mqttContext.transportInterface.send = transportSendFailure;
+    MQTT_SerializeConnectFixedHeader_Stub( MQTT_SerializeConnectFixedHeader_cb );
+    MQTT_GetConnectPacketSize_ExpectAnyArgsAndReturn( MQTTSuccess );
+    MQTT_GetConnectPacketSize_IgnoreArg_pPacketSize();
+    MQTT_GetConnectPacketSize_IgnoreArg_pRemainingLength();
+    MQTT_GetConnectPacketSize_ReturnThruPtr_pPacketSize( &packetSize );
+    MQTT_GetConnectPacketSize_ReturnThruPtr_pRemainingLength( &remainingLength );
+    MQTT_SerializeConnect_IgnoreAndReturn( MQTTSuccess );
+
+    status = MQTT_Connect( &mqttContext, &connectInfo, &willInfo, timeout, &sessionPresent );
+
+    TEST_ASSERT_EQUAL_INT( MQTTSendFailed, status );
 }
 
 /**
