From 55f0220d7cbd6e3ee6caab5be52022ccd1b9ca98 Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Wed, 3 May 2023 13:45:06 -0700
Subject: [PATCH 1/7] Fix timeout calculation to account for overflow

---
 source/core_mqtt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/source/core_mqtt.c b/source/core_mqtt.c
index 5d0ca7f1d..5fd5f1ff1 100644
--- a/source/core_mqtt.c
+++ b/source/core_mqtt.c
@@ -767,7 +767,7 @@ static int32_t sendMessageVector( MQTTContext_t * pContext,
                                   size_t ioVecCount )
 {
     int32_t sendResult;
-    uint32_t timeoutMs;
+    uint32_t startTime;
     TransportOutVector_t * pIoVectIterator;
     size_t vectorsToBeSent = ioVecCount;
     size_t bytesToSend = 0U;
@@ -788,8 +788,8 @@ static int32_t sendMessageVector( MQTTContext_t * pContext,
     /* Reset the iterator to point to the first entry in the array. */
     pIoVectIterator = pIoVec;
 
-    /* Set the timeout. */
-    timeoutMs = pContext->getTime() + MQTT_SEND_TIMEOUT_MS;
+    /* Note the start time. */
+    startTime = pContext->getTime();
 
     while( ( bytesSentOrError < ( int32_t ) bytesToSend ) && ( bytesSentOrError >= 0 ) )
     {
@@ -832,7 +832,7 @@ static int32_t sendMessageVector( MQTTContext_t * pContext,
         }
 
         /* Check for timeout. */
-        if( pContext->getTime() >= timeoutMs )
+        if( calculateElapsedTime( pContext->getTime(), startTime ) > MQTT_SEND_TIMEOUT_MS )
         {
             LogError( ( "sendMessageVector: Unable to send packet: Timed out." ) );
             break;
@@ -1701,7 +1701,7 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext,
         pContext->index += ( size_t ) recvBytes;
 
         status = MQTT_ProcessIncomingPacketTypeAndLength( pContext->networkBuffer.pBuffer,
-                                                          &pContext->index,
+                                                          &( pContext->index ),
                                                           &incomingPacket );
 
         totalMQTTPacketLength = incomingPacket.remainingLength + incomingPacket.headerLength;

From 365fdfc8f9396b2b41cfccccf5093656f989d2b0 Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Wed, 3 May 2023 23:50:19 +0000
Subject: [PATCH 2/7] Add unit tests to check for overflow

---
 test/unit-test/core_mqtt_utest.c | 102 +++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
index dc8b0441b..e03c7f693 100644
--- a/test/unit-test/core_mqtt_utest.c
+++ b/test/unit-test/core_mqtt_utest.c
@@ -320,6 +320,20 @@ static uint32_t getTime( void )
     return globalEntryTime++;
 }
 
+static int32_t getTimeMockCallLimit = -1;
+
+/**
+ * @brief A mocked timer query function that increments on every call. This
+ * guarantees that only a single iteration runs in the ProcessLoop for ease
+ * of testing. Additionally, this test whether the number of calls to this
+ * function have exceeded than the set limit and asserts.
+ */
+static uint32_t getTimeMock( void )
+{
+    TEST_ASSERT_GREATER_THAN_INT32( 0, getTimeMockCallLimit-- );
+    return globalEntryTime++;
+}
+
 /**
  * @brief A mocked timer function that could be used on a device with no system
  * time.
@@ -4628,6 +4642,53 @@ void test_MQTT_Subscribe_happy_path2( void )
     TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
 }
 
+/**
+ * @brief This test case verifies that MQTT_Subscribe returns successfully
+ * when valid parameters are passed and all bytes are sent when the getTime
+ * function timer overflows.
+ */
+void test_MQTT_Subscribe_happy_path_timerOverflow( void )
+{
+    MQTTStatus_t mqttStatus;
+    MQTTContext_t context = { 0 };
+    TransportInterface_t transport = { 0 };
+    MQTTFixedBuffer_t networkBuffer = { 0 };
+    MQTTSubscribeInfo_t subscribeInfo[ 2 ];
+    size_t remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;
+    size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
+    MQTTPubAckInfo_t incomingRecords = { 0 };
+    MQTTPubAckInfo_t outgoingRecords = { 0 };
+
+    setupTransportInterface( &transport );
+    setupNetworkBuffer( &networkBuffer );
+    transport.send = transportSendSucceedThenFail;
+    transport.writev = NULL;
+    setupSubscriptionInfo( &subscribeInfo[ 0 ] );
+    setupSubscriptionInfo( &subscribeInfo[ 1 ] );
+
+    globalEntryTime = UINT32_MAX;
+
+    /* Initialize context. */
+    mqttStatus = MQTT_Init( &context, &transport, getTime, eventCallback, &networkBuffer );
+    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
+
+    mqttStatus = MQTT_InitStatefulQoS( &context,
+                                       &outgoingRecords, 4,
+                                       &incomingRecords, 4 );
+    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
+
+    /* Verify MQTTSuccess is returned with the following mocks. */
+    MQTT_GetSubscribePacketSize_ExpectAnyArgsAndReturn( MQTTSuccess );
+    MQTT_GetSubscribePacketSize_ReturnThruPtr_pPacketSize( &packetSize );
+    MQTT_GetSubscribePacketSize_ReturnThruPtr_pRemainingLength( &remainingLength );
+    MQTT_SerializeSubscribeHeader_Stub( MQTT_SerializeSubscribedHeader_cb1 );
+
+    /* Expect the above calls when running MQTT_Subscribe. */
+    mqttStatus = MQTT_Subscribe( &context, subscribeInfo, 2, MQTT_FIRST_VALID_PACKET_ID );
+
+    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
+}
+
 void test_MQTT_Subscribe_MultipleSubscriptions( void )
 {
     MQTTStatus_t mqttStatus;
@@ -4755,6 +4816,47 @@ void test_MQTT_Subscribe_error_paths2( void )
     TEST_ASSERT_EQUAL( MQTTSendFailed, mqttStatus );
 }
 
+/**
+ * @brief This test case verifies that MQTT_Subscribe returns MQTTSendFailed
+ * if transport interface send fails and timer overflows.
+ */
+void test_MQTT_Subscribe_error_paths_timerOverflowCheck( void )
+{
+    MQTTStatus_t mqttStatus;
+    MQTTContext_t context = { 0 };
+    TransportInterface_t transport = { 0 };
+    MQTTFixedBuffer_t networkBuffer = { 0 };
+    MQTTSubscribeInfo_t subscribeInfo = { 0 };
+    size_t remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;
+    size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
+
+    globalEntryTime = UINT32_MAX - MQTT_OVERFLOW_OFFSET;
+    /* The timer function can be called a maximum of these many times
+     * (which is way less than UINT32_MAX). This ensures that if overflow
+     * check is not correct, then the timer mock call will fail and assert. */
+    getTimeMockCallLimit = 1;
+
+    /* Verify that an error is propagated when transport interface returns an error. */
+    setupNetworkBuffer( &networkBuffer );
+    setupSubscriptionInfo( &subscribeInfo );
+    subscribeInfo.qos = MQTTQoS0;
+    setupTransportInterface( &transport );
+    transport.writev = NULL;
+    /* Case when there is timeout in sending data through transport send. */
+    transport.send = transportSendNoBytes; /* Use the mock function that returns zero bytes sent. */
+
+    /* Initialize context. */
+    mqttStatus = MQTT_Init( &context, &transport, getTimeMock, eventCallback, &networkBuffer );
+    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
+
+    MQTT_GetSubscribePacketSize_ExpectAnyArgsAndReturn( MQTTSuccess );
+    MQTT_GetSubscribePacketSize_ReturnThruPtr_pPacketSize( &packetSize );
+    MQTT_GetSubscribePacketSize_ReturnThruPtr_pRemainingLength( &remainingLength );
+    MQTT_SerializeSubscribeHeader_Stub( MQTT_SerializeSubscribedHeader_cb );
+    mqttStatus = MQTT_Subscribe( &context, &subscribeInfo, 1, MQTT_FIRST_VALID_PACKET_ID );
+    TEST_ASSERT_EQUAL( MQTTSendFailed, mqttStatus );
+}
+
 /* ========================================================================== */
 
 /**

From b9199e4ab4c4ed6ba0cbd210dd10249b8b74fdbb Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Thu, 4 May 2023 00:36:21 +0000
Subject: [PATCH 3/7] Update timeout value in UT

---
 test/unit-test/core_mqtt_config.h |  2 ++
 test/unit-test/core_mqtt_utest.c  | 51 ++-----------------------------
 2 files changed, 4 insertions(+), 49 deletions(-)

diff --git a/test/unit-test/core_mqtt_config.h b/test/unit-test/core_mqtt_config.h
index 6c659fa64..ba1d9a02f 100644
--- a/test/unit-test/core_mqtt_config.h
+++ b/test/unit-test/core_mqtt_config.h
@@ -77,4 +77,6 @@ struct NetworkContext
 
 #define MQTT_SUB_UNSUB_MAX_VECTORS    ( 6U )
 
+#define MQTT_SEND_TIMEOUT_MS          ( 20U )
+
 #endif /* ifndef CORE_MQTT_CONFIG_H_ */
diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
index e03c7f693..4094937ff 100644
--- a/test/unit-test/core_mqtt_utest.c
+++ b/test/unit-test/core_mqtt_utest.c
@@ -330,7 +330,7 @@ static int32_t getTimeMockCallLimit = -1;
  */
 static uint32_t getTimeMock( void )
 {
-    TEST_ASSERT_GREATER_THAN_INT32( 0, getTimeMockCallLimit-- );
+    TEST_ASSERT_GREATER_THAN_INT32( -1, getTimeMockCallLimit-- );
     return globalEntryTime++;
 }
 
@@ -4642,53 +4642,6 @@ void test_MQTT_Subscribe_happy_path2( void )
     TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
 }
 
-/**
- * @brief This test case verifies that MQTT_Subscribe returns successfully
- * when valid parameters are passed and all bytes are sent when the getTime
- * function timer overflows.
- */
-void test_MQTT_Subscribe_happy_path_timerOverflow( void )
-{
-    MQTTStatus_t mqttStatus;
-    MQTTContext_t context = { 0 };
-    TransportInterface_t transport = { 0 };
-    MQTTFixedBuffer_t networkBuffer = { 0 };
-    MQTTSubscribeInfo_t subscribeInfo[ 2 ];
-    size_t remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;
-    size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
-    MQTTPubAckInfo_t incomingRecords = { 0 };
-    MQTTPubAckInfo_t outgoingRecords = { 0 };
-
-    setupTransportInterface( &transport );
-    setupNetworkBuffer( &networkBuffer );
-    transport.send = transportSendSucceedThenFail;
-    transport.writev = NULL;
-    setupSubscriptionInfo( &subscribeInfo[ 0 ] );
-    setupSubscriptionInfo( &subscribeInfo[ 1 ] );
-
-    globalEntryTime = UINT32_MAX;
-
-    /* Initialize context. */
-    mqttStatus = MQTT_Init( &context, &transport, getTime, eventCallback, &networkBuffer );
-    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
-
-    mqttStatus = MQTT_InitStatefulQoS( &context,
-                                       &outgoingRecords, 4,
-                                       &incomingRecords, 4 );
-    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
-
-    /* Verify MQTTSuccess is returned with the following mocks. */
-    MQTT_GetSubscribePacketSize_ExpectAnyArgsAndReturn( MQTTSuccess );
-    MQTT_GetSubscribePacketSize_ReturnThruPtr_pPacketSize( &packetSize );
-    MQTT_GetSubscribePacketSize_ReturnThruPtr_pRemainingLength( &remainingLength );
-    MQTT_SerializeSubscribeHeader_Stub( MQTT_SerializeSubscribedHeader_cb1 );
-
-    /* Expect the above calls when running MQTT_Subscribe. */
-    mqttStatus = MQTT_Subscribe( &context, subscribeInfo, 2, MQTT_FIRST_VALID_PACKET_ID );
-
-    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
-}
-
 void test_MQTT_Subscribe_MultipleSubscriptions( void )
 {
     MQTTStatus_t mqttStatus;
@@ -4834,7 +4787,7 @@ void test_MQTT_Subscribe_error_paths_timerOverflowCheck( void )
     /* The timer function can be called a maximum of these many times
      * (which is way less than UINT32_MAX). This ensures that if overflow
      * check is not correct, then the timer mock call will fail and assert. */
-    getTimeMockCallLimit = 1;
+    getTimeMockCallLimit = MQTT_SEND_TIMEOUT_MS + 1;
 
     /* Verify that an error is propagated when transport interface returns an error. */
     setupNetworkBuffer( &networkBuffer );

From a7621944ec6176dd4d7b8dcce3af1356d5a6e6da Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Thu, 4 May 2023 00:43:42 +0000
Subject: [PATCH 4/7] Fix formatting

---
 test/unit-test/core_mqtt_utest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
index 4094937ff..34d0affa1 100644
--- a/test/unit-test/core_mqtt_utest.c
+++ b/test/unit-test/core_mqtt_utest.c
@@ -4784,6 +4784,7 @@ void test_MQTT_Subscribe_error_paths_timerOverflowCheck( void )
     size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
 
     globalEntryTime = UINT32_MAX - MQTT_OVERFLOW_OFFSET;
+
     /* The timer function can be called a maximum of these many times
      * (which is way less than UINT32_MAX). This ensures that if overflow
      * check is not correct, then the timer mock call will fail and assert. */

From a03c173120ca99550bd30e3554f4a4047d646eee Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Wed, 3 May 2023 18:11:03 -0700
Subject: [PATCH 5/7] Update core_mqtt_utest.c

---
 test/unit-test/core_mqtt_utest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
index 34d0affa1..472f72a27 100644
--- a/test/unit-test/core_mqtt_utest.c
+++ b/test/unit-test/core_mqtt_utest.c
@@ -4783,7 +4783,7 @@ void test_MQTT_Subscribe_error_paths_timerOverflowCheck( void )
     size_t remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;
     size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
 
-    globalEntryTime = UINT32_MAX - MQTT_OVERFLOW_OFFSET;
+    globalEntryTime = UINT32_MAX;
 
     /* The timer function can be called a maximum of these many times
      * (which is way less than UINT32_MAX). This ensures that if overflow

From 4501b66d980724b562d8c599939e87efeb29dc3d Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Thu, 4 May 2023 05:42:07 +0000
Subject: [PATCH 6/7] Add one more unit test to check for one corner case

---
 test/unit-test/core_mqtt_utest.c | 63 +++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
index 472f72a27..93e240c05 100644
--- a/test/unit-test/core_mqtt_utest.c
+++ b/test/unit-test/core_mqtt_utest.c
@@ -325,7 +325,7 @@ static int32_t getTimeMockCallLimit = -1;
 /**
  * @brief A mocked timer query function that increments on every call. This
  * guarantees that only a single iteration runs in the ProcessLoop for ease
- * of testing. Additionally, this test whether the number of calls to this
+ * of testing. Additionally, this tests whether the number of calls to this
  * function have exceeded than the set limit and asserts.
  */
 static uint32_t getTimeMock( void )
@@ -334,6 +334,23 @@ static uint32_t getTimeMock( void )
     return globalEntryTime++;
 }
 
+static int32_t getTimeMockBigTimeStepCallLimit = -1;
+
+/**
+ * @brief A mocked timer query function that increments by MQTT_SEND_TIMEOUT_MS
+ * to simulate the time consumed by a long running high priority task on every
+ * call. Additionally, this tests whether the number of calls to this function
+ * have exceeded than the set limit and asserts.
+ */
+static uint32_t getTimeMockBigTimeStep( void )
+{
+    TEST_ASSERT_GREATER_THAN_INT32( -1, getTimeMockBigTimeStepCallLimit-- );
+
+    globalEntryTime += MQTT_SEND_TIMEOUT_MS;
+    return globalEntryTime;
+}
+
+
 /**
  * @brief A mocked timer function that could be used on a device with no system
  * time.
@@ -4809,6 +4826,50 @@ void test_MQTT_Subscribe_error_paths_timerOverflowCheck( void )
     MQTT_SerializeSubscribeHeader_Stub( MQTT_SerializeSubscribedHeader_cb );
     mqttStatus = MQTT_Subscribe( &context, &subscribeInfo, 1, MQTT_FIRST_VALID_PACKET_ID );
     TEST_ASSERT_EQUAL( MQTTSendFailed, mqttStatus );
+    TEST_ASSERT_EQUAL( -1, getTimeMockCallLimit );
+}
+
+/**
+ * @brief This test case verifies that MQTT_Subscribe returns MQTTSendFailed
+ * if transport interface send fails and timer overflows.
+ */
+void test_MQTT_Subscribe_error_paths_timerOverflowCheck1( void )
+{
+    MQTTStatus_t mqttStatus;
+    MQTTContext_t context = { 0 };
+    TransportInterface_t transport = { 0 };
+    MQTTFixedBuffer_t networkBuffer = { 0 };
+    MQTTSubscribeInfo_t subscribeInfo = { 0 };
+    size_t remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;
+    size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
+
+    globalEntryTime = UINT32_MAX - MQTT_SEND_TIMEOUT_MS + 1;
+
+    /* The timer function can be called a exactly 2 times. First when setting
+     * the initial time, next time when checking for timeout.
+     */
+    getTimeMockBigTimeStepCallLimit = 2;
+
+    /* Verify that an error is propagated when transport interface returns an error. */
+    setupNetworkBuffer( &networkBuffer );
+    setupSubscriptionInfo( &subscribeInfo );
+    subscribeInfo.qos = MQTTQoS0;
+    setupTransportInterface( &transport );
+    transport.writev = NULL;
+    /* Case when there is timeout in sending data through transport send. */
+    transport.send = transportSendNoBytes; /* Use the mock function that returns zero bytes sent. */
+
+    /* Initialize context. */
+    mqttStatus = MQTT_Init( &context, &transport, getTimeMockBigTimeStep, eventCallback, &networkBuffer );
+    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
+
+    MQTT_GetSubscribePacketSize_ExpectAnyArgsAndReturn( MQTTSuccess );
+    MQTT_GetSubscribePacketSize_ReturnThruPtr_pPacketSize( &packetSize );
+    MQTT_GetSubscribePacketSize_ReturnThruPtr_pRemainingLength( &remainingLength );
+    MQTT_SerializeSubscribeHeader_Stub( MQTT_SerializeSubscribedHeader_cb );
+    mqttStatus = MQTT_Subscribe( &context, &subscribeInfo, 1, MQTT_FIRST_VALID_PACKET_ID );
+    TEST_ASSERT_EQUAL( MQTTSendFailed, mqttStatus );
+    TEST_ASSERT_EQUAL( -1, getTimeMockBigTimeStepCallLimit );
 }
 
 /* ========================================================================== */

From ec5dfc5184b37e37759ad5cbb9b06ee8b2788b16 Mon Sep 17 00:00:00 2001
From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com>
Date: Thu, 4 May 2023 06:12:25 +0000
Subject: [PATCH 7/7] Make unit-test more robust

---
 test/unit-test/core_mqtt_utest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c
index 93e240c05..762feefe7 100644
--- a/test/unit-test/core_mqtt_utest.c
+++ b/test/unit-test/core_mqtt_utest.c
@@ -4800,7 +4800,7 @@ void test_MQTT_Subscribe_error_paths_timerOverflowCheck( void )
     size_t remainingLength = MQTT_SAMPLE_REMAINING_LENGTH;
     size_t packetSize = MQTT_SAMPLE_REMAINING_LENGTH;
 
-    globalEntryTime = UINT32_MAX;
+    globalEntryTime = UINT32_MAX - 2U;
 
     /* The timer function can be called a maximum of these many times
      * (which is way less than UINT32_MAX). This ensures that if overflow