add checks for pOtaBuffer to be non_null #402

Bujain · 2021-12-09T23:16:20Z

OTA_Init is a public function and a user can pass NULL value to pOtaBuffer which can result in NULL pointer de-referencing in initializeAppBuffers function.

Description

This PR adds checks such that if NULL value is passed to pOtaBuffer then the otaAgent would not run and display error in the logs indicating the NULL pointer in pOtaBuffer.

Checklist:

I have tested my changes. No regression in existing tests.
My code is formatted using Uncrustify.
I have read and applied the rules stated in CONTRIBUTING.md.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

paulbartell · 2021-12-10T19:56:31Z

source/ota.c

            }
            else
            {
-                LogError( ( "Error: Thing name is too long.\r\n" ) );
+                uint32_t strLength = ( uint32_t ) ( strlen( ( const char * ) pThingName ) );


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.

paulbartell · 2021-12-10T20:05:55Z

source/ota.c

-                LogError( ( "Error: Thing name is too long.\r\n" ) );
+                uint32_t strLength = ( uint32_t ) ( strlen( ( const char * ) pThingName ) );
+
+                if( strLength <= otaconfigMAX_THINGNAME_LEN )


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; }

paulbartell · 2021-12-10T20:06:23Z

source/ota.c

+                }
+                else
+                {
+                    LogError( ( "Error: Thing name is too long.\r\n" ) );


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

Suggested change

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

LogError( ( "Error: Thing name is too long." ) );

paulbartell · 2021-12-10T20:08:04Z

source/ota.c

-                 */
-                ( void ) memcpy( otaAgent.pThingName, pThingName, strLength + 1UL );
-                returnStatus = OtaErrNone;
+                LogError( ( "Error: Thing name is NULL.\r\n" ) );


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.

paulbartell · 2021-12-10T20:08:15Z

source/ota.c

        {
-            LogError( ( "Error: Thing name is NULL.\r\n" ) );
+            LogError( ( "Error: pOtaBuffer is NULL.\r\n" ) );


Suggested change

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

LogError( ( "Error: pOtaBuffer is NULL." ) );

paulbartell · 2021-12-10T20:11:05Z

source/ota.c

        {
-            LogError( ( "Error: Thing name is NULL.\r\n" ) );
+            LogError( ( "Error: pOtaBuffer is NULL.\r\n" ) );
        }


Suggested change

}

returnStatus = OtaErrInvalidArg;

}

paulbartell · 2021-12-10T20:13:11Z

source/ota.c

        {
-            LogError( ( "Error: Thing name is NULL.\r\n" ) );
+            LogError( ( "Error: pOtaBuffer is NULL.\r\n" ) );
        }
        else


Suggested change

else

if( returnStatus == OtaErrNone )

paulbartell · 2021-12-10T20:14:39Z

test/unit-test/ota_utest.c

+{
+    OtaAppBuffer_t * otaAppBuffer = NULL;
+
+    OTA_Init( otaAppBuffer,


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

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.

paulbartell · 2021-12-10T20:20:12Z

source/ota.c

-                 */
-                ( void ) memcpy( otaAgent.pThingName, pThingName, strLength + 1UL );
-                returnStatus = OtaErrNone;
+                LogError( ( "Error: Thing name is NULL.\r\n" ) );
            }
            else


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

paulbartell · 2021-12-10T20:21:40Z

source/ota.c

@@ -3094,9 +3094,6 @@ OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
         */
        otaAgent.pOtaInterface = pOtaInterfaces;


Is pOtaInterfaces allowed to be null?

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.

Why not return an error if the parameter is invalid?

paulbartell · 2021-12-10T20:22:01Z

source/ota.c

@@ -3094,9 +3094,6 @@ OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
         */
        otaAgent.pOtaInterface = pOtaInterfaces;

-        /* Initialize application buffers. */
-        initializeAppBuffers( pOtaBuffer );
-
        /* Initialize local buffers. */
        initializeLocalBuffers();



is OtaAppCallback allowed to be NULL?

Bujain added 2 commits December 9, 2021 23:13

add checks for pOtaBuffer to be non_null

c6ac8ff

add unit test

d5d686b

divekarshubham approved these changes Dec 10, 2021

View reviewed changes

paulbartell reviewed Dec 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add checks for pOtaBuffer to be non_null #402

add checks for pOtaBuffer to be non_null #402

Bujain commented Dec 9, 2021

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021 •

edited

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021 •

edited

paulbartell Dec 10, 2021

Bujain Dec 10, 2021

paulbartell Dec 10, 2021

paulbartell Dec 10, 2021

Bujain Dec 10, 2021

paulbartell Dec 11, 2021

paulbartell Dec 10, 2021

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

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

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

		@@ -3094,9 +3094,6 @@ OtaErr_t OTA_Init( OtaAppBuffer_t * pOtaBuffer,
		*/
		otaAgent.pOtaInterface = pOtaInterfaces;

add checks for pOtaBuffer to be non_null #402

Are you sure you want to change the base?

add checks for pOtaBuffer to be non_null #402

Conversation

Bujain commented Dec 9, 2021

Description

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbartell Dec 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbartell Dec 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbartell Dec 10, 2021 •

edited

paulbartell Dec 10, 2021 •

edited