Skip to content

Fix MQTT 5 SUBACK reason codes passed to completion callbacks #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ZoneMR wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix MQTT 5 SUBACK reason codes passed to completion callbacks #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions source/core_mqtt_agent.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,16 +604,40 @@ static void handleAcks( const MQTTAgentContext_t * pAgentContext,
uint8_t packetType )
{
uint8_t * pSubackCodes = NULL;
MQTTStatus_t status = pDeserializedInfo->deserializationResult;

assert( pAckInfo != NULL );
assert( pAckInfo->pOriginalCommand != NULL );

/* A SUBACK's status codes start 2 bytes after the variable header. */
pSubackCodes = ( packetType == MQTT_PACKET_TYPE_SUBACK ) ? ( &pPacketInfo->pRemainingData[ 2U ] ) : NULL;
/* MQTT 5 places a variable-length properties section after the packet ID.
* Use the parser's reason-code view rather than the MQTT 3 fixed offset,
* which would expose the property length as the first grant code. */
if( packetType == MQTT_PACKET_TYPE_SUBACK )
{
const MQTTAgentSubscribeArgs_t * pArgs =
( const MQTTAgentSubscribeArgs_t * ) pAckInfo->pOriginalCommand->pArgs;

if( ( pAckInfo->pOriginalCommand->commandType != SUBSCRIBE ) ||
( pDeserializedInfo->pReasonCode == NULL ) ||
( pDeserializedInfo->pReasonCode->reasonCode == NULL ) ||
( pDeserializedInfo->pReasonCode->reasonCodeLength != pArgs->numSubscriptions ) )
{
/* Callbacks read one code per requested filter. Never hand them a
* short or malformed array, even if the packet itself parsed. */
status = MQTTBadResponse;
}
else
{
/* Preserve the callback's mutable pointer type without casting
* away const from the parser's view of the same packet buffer. */
pSubackCodes = &pPacketInfo->pRemainingData[
pDeserializedInfo->pReasonCode->reasonCode - pPacketInfo->pRemainingData ];
}
}

concludeCommand( pAgentContext,
pAckInfo->pOriginalCommand,
pDeserializedInfo->deserializationResult,
status,
pSubackCodes );

/* Clear the entry from the list. */
Expand Down
137 changes: 137 additions & 0 deletions test/unit-test/mqtt_agent_utest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ struct MQTTAgentMessageContext
struct MQTTAgentCommandContext
{
MQTTStatus_t returnStatus;
uint8_t * pSubackCodes;
};

/**
Expand Down Expand Up @@ -246,6 +247,7 @@ static void stubCompletionCallback( MQTTAgentCommandContext_t * pCommandCompleti
if( pCommandCompletionContext != NULL )
{
pCommandCompletionContext->returnStatus = pReturnInfo->returnCode;
pCommandCompletionContext->pSubackCodes = pReturnInfo->pSubackCodes;
}

commandCompleteCallbackCount++;
Expand Down Expand Up @@ -387,6 +389,141 @@ static void setupAgentContext( MQTTAgentContext_t * pAgentContext )
pAgentContext->mqttContext.nextPacketId = 1U;
}

/**
* @brief Exercise SUBACK completion with a parser-provided reason-code view.
*/
static void checkSubackCompletion( uint8_t * pRemainingData,
MQTTReasonCodeInfo_t * pReasonCodes,
size_t subscriptionCount,
MQTTAgentCommandType_t commandType,
MQTTStatus_t expectedStatus )
{
MQTTAgentContext_t agentContext;
MQTTAgentCommand_t command = { 0 };
MQTTAgentSubscribeArgs_t subscribeArgs = { 0 };
MQTTAgentCommandContext_t completion = { 0 };
MQTTPacketInfo_t packetInfo = { 0 };
MQTTDeserializedInfo_t deserializedInfo = { 0 };

setupAgentContext( &agentContext );
subscribeArgs.numSubscriptions = subscriptionCount;
command.commandType = commandType;
command.pArgs = &subscribeArgs;
command.pCmdContext = &completion;
command.pCommandCompleteCallback = stubCompletionCallback;
agentContext.pPendingAcks[ 0 ].packetId = 1U;
agentContext.pPendingAcks[ 0 ].pOriginalCommand = &command;
packetInfo.type = MQTT_PACKET_TYPE_SUBACK;
packetInfo.pRemainingData = pRemainingData;
deserializedInfo.packetIdentifier = 1U;
deserializedInfo.deserializationResult = MQTTSuccess;
deserializedInfo.pReasonCode = pReasonCodes;

TEST_ASSERT_TRUE( agentContext.mqttContext.appCallback( &agentContext.mqttContext,
&packetInfo,
&deserializedInfo,
NULL, NULL, NULL ) );
TEST_ASSERT_EQUAL( expectedStatus, completion.returnStatus );

if( expectedStatus == MQTTSuccess )
{
TEST_ASSERT_EQUAL_PTR( pReasonCodes->reasonCode, completion.pSubackCodes );
}
else
{
TEST_ASSERT_NULL( completion.pSubackCodes );
}

TEST_ASSERT_EQUAL( 1U, commandCompleteCallbackCount );
TEST_ASSERT_EQUAL( 1U, commandReleaseCallCount );
TEST_ASSERT_EQUAL( 0U, agentContext.pPendingAcks[ 0 ].packetId );
TEST_ASSERT_NULL( agentContext.pPendingAcks[ 0 ].pOriginalCommand );
}

/**
* @brief A single-filter refusal follows the MQTT 5 property-length byte.
*/
void test_MQTTAgent_Suback_single_refusal( void )
{
uint8_t remainingData[] = { 0U, 1U, 0U, 0x87U };
MQTTReasonCodeInfo_t reasonCodes = { &remainingData[ 3 ], 1U };

checkSubackCompletion( remainingData, &reasonCodes, 1U, SUBSCRIBE, MQTTSuccess );
}

/**
* @brief A batch must expose the last filter's refusal as well as the first grant.
*/
void test_MQTTAgent_Suback_mixed_grants( void )
{
uint8_t remainingData[] = { 0U, 1U, 0U, 1U, 0x87U };
MQTTReasonCodeInfo_t reasonCodes = { &remainingData[ 3 ], 2U };

checkSubackCompletion( remainingData, &reasonCodes, 2U, SUBSCRIBE, MQTTSuccess );
}

/**
* @brief Reason String properties must not be exposed as grant codes.
*/
void test_MQTTAgent_Suback_with_properties( void )
{
uint8_t remainingData[] = { 0U, 1U, 5U, 0x1FU, 0U, 2U, 'n', 'o', 0x87U };
MQTTReasonCodeInfo_t reasonCodes = { &remainingData[ 8 ], 1U };

checkSubackCompletion( remainingData, &reasonCodes, 1U, SUBSCRIBE, MQTTSuccess );
}

/**
* @brief Reject a short array before callbacks read one code per filter.
*/
void test_MQTTAgent_Suback_short_reason_array( void )
{
uint8_t remainingData[] = { 0U, 1U, 0U, 1U };
MQTTReasonCodeInfo_t reasonCodes = { &remainingData[ 3 ], 1U };

checkSubackCompletion( remainingData, &reasonCodes, 2U, SUBSCRIBE, MQTTBadResponse );
}

/**
* @brief Reject an array with more codes than requested filters.
*/
void test_MQTTAgent_Suback_long_reason_array( void )
{
uint8_t remainingData[] = { 0U, 1U, 0U, 1U, 0U };
MQTTReasonCodeInfo_t reasonCodes = { &remainingData[ 3 ], 2U };

checkSubackCompletion( remainingData, &reasonCodes, 1U, SUBSCRIBE, MQTTBadResponse );
}

/**
* @brief Missing parser metadata must not be reported as a successful SUBACK.
*/
void test_MQTTAgent_Suback_missing_reason_info( void )
{
checkSubackCompletion( NULL, NULL, 1U, SUBSCRIBE, MQTTBadResponse );
}

/**
* @brief Missing reason-code storage must not be handed to the callback.
*/
void test_MQTTAgent_Suback_null_reason_array( void )
{
MQTTReasonCodeInfo_t reasonCodes = { NULL, 1U };

checkSubackCompletion( NULL, &reasonCodes, 1U, SUBSCRIBE, MQTTBadResponse );
}

/**
* @brief A SUBACK must match a SUBSCRIBE before interpreting its arguments.
*/
void test_MQTTAgent_Suback_wrong_command_type( void )
{
uint8_t remainingData[] = { 0U, 1U, 0U, 1U };
MQTTReasonCodeInfo_t reasonCodes = { &remainingData[ 3 ], 1U };

checkSubackCompletion( remainingData, &reasonCodes, 1U, PUBLISH, MQTTBadResponse );
}

/**
* @brief Helper function to test API functions of the form
* MQTTStatus_t func( MQTTAgentContext_t *, MQTTAgentCommandInfo_t * )
Expand Down