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/8] 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/8] 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/8] 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/8] 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/8] 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/8] 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/8] 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 From b71970f9ebceb687a9c1ca32e6fce70c858b2786 Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Mon, 26 Jun 2023 11:44:07 -0700 Subject: [PATCH 8/8] Fix MQTT_Status_strerror to return correct error on NeedMoreBytes error. --- source/core_mqtt.c | 4 ++++ test/unit-test/core_mqtt_utest.c | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/source/core_mqtt.c b/source/core_mqtt.c index b625f08d7..1efb3520a 100644 --- a/source/core_mqtt.c +++ b/source/core_mqtt.c @@ -3373,6 +3373,10 @@ const char * MQTT_Status_strerror( MQTTStatus_t status ) str = "MQTTKeepAliveTimeout"; break; + case MQTTNeedMoreBytes: + str = "MQTTNeedMoreBytes"; + break; + default: str = "Invalid MQTT Status code"; break; diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c index 762feefe7..97f87c080 100644 --- a/test/unit-test/core_mqtt_utest.c +++ b/test/unit-test/core_mqtt_utest.c @@ -5842,7 +5842,11 @@ void test_MQTT_Status_strerror( void ) str = MQTT_Status_strerror( status ); TEST_ASSERT_EQUAL_STRING( "MQTTKeepAliveTimeout", str ); - status = MQTTKeepAliveTimeout + 1; + status = MQTTNeedMoreBytes; + str = MQTT_Status_strerror( status ); + TEST_ASSERT_EQUAL_STRING( "MQTTNeedMoreBytes", str ); + + status = MQTTNeedMoreBytes + 1; str = MQTT_Status_strerror( status ); TEST_ASSERT_EQUAL_STRING( "Invalid MQTT Status code", str ); }