Skip to content
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

Jump to bottom

add checks for pOtaBuffer to be non_null #402

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add checks for pOtaBuffer to be non_null #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6ac8ff
add checks for pOtaBuffer to be non_null
Bujain Dec 9, 2021
d5d686b
add unit test
Bujain Dec 9, 2021
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
35 changes: 21 additions & 14 deletions source/ota.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3094,9 +3094,6 @@ OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
*/
otaAgent.pOtaInterface = pOtaInterfaces;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pOtaInterfaces allowed to be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, pOtaInterfaces can be NULL. There is a conditional statement in OTA_EventProcessingTask function which checks if the pOtaInterfaces are NULL, if they are then the OTA agent would throws an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return an error if the parameter is invalid?


/* Initialize application buffers. */
initializeAppBuffers( pOtaBuffer );

/* Initialize local buffers. */
initializeLocalBuffers();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is OtaAppCallback allowed to be NULL?

Expand All @@ -3113,26 +3110,36 @@ OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
*/
( void ) otaAgent.pOtaInterface->os.event.init( NULL );

if( pThingName == NULL )
if( pOtaBuffer == NULL )
{
LogError( ( "Error: Thing name is NULL.\r\n" ) );
LogError( ( "Error: pOtaBuffer is NULL.\r\n" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LogError( ( "Error: pOtaBuffer is NULL.\r\n" ) );
LogError( ( "Error: pOtaBuffer is NULL." ) );

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
returnStatus = OtaErrInvalidArg;
}

else
Copy link
Member

@paulbartell paulbartell Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
else
if( returnStatus == OtaErrNone )

{
uint32_t strLength = ( uint32_t ) ( strlen( ( const char * ) pThingName ) );
/* Initialize application buffers. */
initializeAppBuffers( pOtaBuffer );

if( strLength <= otaconfigMAX_THINGNAME_LEN )
if( pThingName == NULL )
{
/*
* Store the Thing name to be used for topics later. Include zero terminator
* when saving the Thing name.
*/
( void ) memcpy( otaAgent.pThingName, pThingName, strLength + 1UL );
returnStatus = OtaErrNone;
LogError( ( "Error: Thing name is NULL.\r\n" ) );
Copy link
Member

@paulbartell paulbartell Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LogError( ( "Error: Thing name is NULL.\r\n" ) );
LogError( ( "Error: Thing name is NULL." ) );
returnStatus = OtaErrInvalidArg;

LogError messages don't include an \r\n in the rest of this file.

}
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be turned into an if / else if / else set of statements to improve readability.

{
LogError( ( "Error: Thing name is too long.\r\n" ) );
uint32_t strLength = ( uint32_t ) ( strlen( ( const char * ) pThingName ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature of this function should be updated from:

OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
                   const OtaInterfaces_t * pOtaInterfaces,
                   const uint8_t * pThingName,
                   OtaAppCallback_t OtaAppCallback )

to

OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
                   const OtaInterfaces_t * pOtaInterfaces,
                   const char * pThingName,
                   OtaAppCallback_t OtaAppCallback )

if pThingName is indeed a null-terminated string.


if( strLength <= otaconfigMAX_THINGNAME_LEN )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to

if( strnlen( pThingName, otaconfigMAX_THINGNAME_LEN + 1 ) <= otaconfigMAX_THINGNAME_LEN )
{
    (void) strncpy(  otaAgent.pThingName, pThingName, otaconfigMAX_THINGNAME_LEN );
}
else
{
    LogError("...");
    returnStatus = xxx;
}

{
/*
* Store the Thing name to be used for topics later. Include zero terminator
* when saving the Thing name.
*/
( void ) memcpy( otaAgent.pThingName, pThingName, strLength + 1UL );
returnStatus = OtaErrNone;
}
else
{
LogError( ( "Error: Thing name is too long.\r\n" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't returnStatus be set to a particular value here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LogError( ( "Error: Thing name is too long.\r\n" ) );
LogError( ( "Error: Thing name is too long." ) );

}
}
}

Expand Down
11 changes: 11 additions & 0 deletions test/unit-test/ota_utest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,17 @@ void test_OTA_InitNullAppBuffers()
TEST_ASSERT_EQUAL( OtaAgentStateInit, OTA_GetState() );
}

void test_OTA_InitNullOtaAppPointer()
{
OtaAppBuffer_t * otaAppBuffer = NULL;

OTA_Init( otaAppBuffer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return value of OTA_Init should be checked ( Should be OtaErrInvalidArg right? ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the current implementation of the function OTA_Init only returns two values. If there are no errors during the initialization of the agent then it returns OtaErrNone and OtaErrUninitialized otherwise.

&otaInterfaces,
( const uint8_t * ) pOtaDefaultClientId,
mockAppCallback );
TEST_ASSERT_EQUAL( OtaAgentStateStopped, OTA_GetState() );
}

void test_OTA_InitZeroAppBufferSizes()
{
/* Test for having valid pointers with zero sizes. */
Expand Down