From 52d47ca379f485950bac210b15d87dcd78091103 Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Tue, 20 Feb 2024 21:47:20 +0000 Subject: [PATCH] Fix MISRA violations --- MISRA.md | 13 +++-- source/core_http_client.c | 72 +++++++++++-------------- test/CMakeLists.txt | 101 ++++++++++++++++++++---------------- tools/coverity/misra.config | 63 +++++++++++----------- 4 files changed, 124 insertions(+), 125 deletions(-) diff --git a/MISRA.md b/MISRA.md index f2b350de..d00fdbd5 100644 --- a/MISRA.md +++ b/MISRA.md @@ -64,11 +64,18 @@ _Ref 10.8.1_ _Ref 14.3.1_ - MISRA Rule 14.3 The third-party http-parser library sets a uint64_t type field to - `ULLONG_MAX` or `( ( uint64_t ) -1 )`, during its internal parsing. Coverity MISRA does not detect - that this variable changes. This field is checked by the HTTP Client library. - If the Content-Length header was found, then pHttpParser->content_length + `ULLONG_MAX` or `( ( uint64_t ) -1 )`, during its internal parsing. Coverity MISRA + does not detect that this variable changes. This field is checked by the HTTP + Client library. If the Content-Length header was found, then pHttpParser->content_length will not be equal to the maximum 64 bit integer. +#### Rule 11.4 +_Ref 11.4.1_ + +- MISRA Rule 11.4 does not allow casting pointers to different data types as this may lead + to misaligned pointers. But in this case, the pointers are casted to different data + type to get the length of the header. It is not casted back to a pointer. + #### Rule 11.8 _Ref 11.8.1_ diff --git a/source/core_http_client.c b/source/core_http_client.c index c4a4533a..19c4edf4 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -43,19 +43,6 @@ */ static uint32_t getZeroTimestampMs( void ); - -HTTPStatus_t HTTPClient_SendHttpData( const TransportInterface_t * pTransport, - HTTPClient_GetCurrentTimeFunc_t getTimestampMs, - const uint8_t * pData, - size_t dataLen ); - - -HTTPStatus_t HTTPClient_SendHttpHeaders( const TransportInterface_t * pTransport, - HTTPClient_GetCurrentTimeFunc_t getTimestampMs, - HTTPRequestHeaders_t * pRequestHeaders, - size_t reqBodyLen, - uint32_t sendFlags ); - /** * @brief Adds the Content-Length header field and value to the * @p pRequestHeaders. @@ -162,11 +149,6 @@ static HTTPStatus_t getFinalResponseStatus( HTTPParsingState_t parsingState, size_t totalReceived, size_t responseBufferLen ); - -HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t * pTransport, - HTTPResponse_t * pResponse, - const HTTPRequestHeaders_t * pRequestHeaders ); - /** * @brief Send the HTTP request over the network. * @@ -668,7 +650,7 @@ static int httpParserOnStatusCallback( llhttp_t * pHttpParser, assert( pResponse != NULL ); /* Set the location of what to parse next. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ];; /* Initialize the first header field and value to be passed to the user * callback. */ @@ -748,7 +730,7 @@ static int httpParserOnHeaderFieldCallback( llhttp_t * pHttpParser, } /* Set the location of what to parse next. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ];; /* The httpParserOnHeaderFieldCallback() always follows the * httpParserOnHeaderValueCallback() if there is another header field. When @@ -794,7 +776,7 @@ static int httpParserOnHeaderValueCallback( llhttp_t * pHttpParser, pParsingContext = ( HTTPParsingContext_t * ) ( pHttpParser->data ); /* Set the location of what to parse next. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ];; /* If httpParserOnHeaderValueCallback() is invoked in succession, then the * last time llhttp_execute() was called only part of the header field @@ -916,7 +898,7 @@ static int httpParserOnHeadersCompleteCallback( llhttp_t * pHttpParser ) * parsing here. */ if( ( pResponse->respOptionFlags & HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG ) != 0U ) { - shouldContinueParse = LLHTTP_PAUSE_PARSING; + shouldContinueParse = ( int ) LLHTTP_PAUSE_PARSING; } return shouldContinueParse; @@ -963,7 +945,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, /* MISRA Ref 11.8.1 [Removal of const from pointer] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-118 */ /* coverity[misra_c_2012_rule_11_8_violation] */ - pNextWriteLoc = ( char * ) ( pResponse->pBody + pResponse->bodyLen ); + pNextWriteLoc = ( char * ) ( &pResponse->pBody[ pResponse->bodyLen ] ); /* If the response is of type Transfer-Encoding: chunked, then actual body * will follow the the chunked header. This body data is in a later location @@ -985,7 +967,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, pResponse->bodyLen += length; /* Set the next location of parsing. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ];; LogDebug( ( "Response parsing: Found the response body: " "BodyLength=%lu", @@ -1243,7 +1225,7 @@ static HTTPStatus_t parseHttpResponse( HTTPParsingContext_t * pParsingContext, else { /* The next location to parse is after what has already been parsed. */ - pParsingContext->pBufferCur = parsingStartLoc + parseLen; + pParsingContext->pBufferCur = &parsingStartLoc[ parseLen ]; } returnStatus = processLlhttpError( &( pParsingContext->llhttpParser ) ); @@ -1378,18 +1360,18 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, assert( fieldLen != 0U ); assert( valueLen != 0U ); - pBufferCur = ( char * ) ( pRequestHeaders->pBuffer + pRequestHeaders->headersLen ); + pBufferCur = ( char * ) ( &pRequestHeaders->pBuffer[ pRequestHeaders->headersLen ] ); backtrackHeaderLen = pRequestHeaders->headersLen; /* Backtrack before trailing "\r\n" (HTTP header end) if it's already written. * Note that this method also writes trailing "\r\n" before returning. * The first condition prevents reading before start of the header. */ if( ( HTTP_HEADER_END_INDICATOR_LEN <= pRequestHeaders->headersLen ) && - ( strncmp( ( char * ) pBufferCur - HTTP_HEADER_END_INDICATOR_LEN, + ( strncmp( ( char * ) &pBufferCur[ 0U - HTTP_HEADER_END_INDICATOR_LEN ], HTTP_HEADER_END_INDICATOR, HTTP_HEADER_END_INDICATOR_LEN ) == 0 ) ) { backtrackHeaderLen -= HTTP_HEADER_LINE_SEPARATOR_LEN; - pBufferCur -= HTTP_HEADER_LINE_SEPARATOR_LEN; + pBufferCur = &pBufferCur[ 0U - HTTP_HEADER_LINE_SEPARATOR_LEN ]; } /* Check if there is enough space in buffer for additional header. */ @@ -1411,14 +1393,14 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, if( returnStatus == HTTPSuccess ) { - pBufferCur += fieldLen; + pBufferCur = &pBufferCur[ fieldLen ]; /* Copy the field separator, ": ", into the buffer. */ ( void ) memcpy( pBufferCur, httpFieldSeparator, HTTP_HEADER_FIELD_SEPARATOR_LEN ); - pBufferCur += HTTP_HEADER_FIELD_SEPARATOR_LEN; + pBufferCur = &pBufferCur[ HTTP_HEADER_FIELD_SEPARATOR_LEN ]; /* Copy the header value into the buffer. */ if( httpHeaderStrncpy( pBufferCur, pValue, valueLen, HTTP_HEADER_STRNCPY_IS_VALUE ) == NULL ) @@ -1429,7 +1411,7 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, if( returnStatus == HTTPSuccess ) { - pBufferCur += valueLen; + pBufferCur = &pBufferCur[ valueLen ]; /* Copy the header end indicator, "\r\n\r\n" into the buffer. */ ( void ) memcpy( pBufferCur, @@ -1485,7 +1467,7 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders, /* Write the range start value in the buffer. */ rangeValueLength += convertInt32ToAscii( rangeStartOrlastNbytes, - rangeValueBuffer + rangeValueLength, + &rangeValueBuffer[ rangeValueLength ], sizeof( rangeValueBuffer ) - rangeValueLength ); /* Add remaining value data depending on the range specification type. */ @@ -1494,19 +1476,19 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders, if( rangeEnd != HTTP_RANGE_REQUEST_END_OF_FILE ) { /* Write the "-" character to the buffer.*/ - *( rangeValueBuffer + rangeValueLength ) = DASH_CHARACTER; + rangeValueBuffer[ rangeValueLength ] = DASH_CHARACTER; rangeValueLength += DASH_CHARACTER_LEN; /* Write the rangeEnd value of the request range to the buffer. */ rangeValueLength += convertInt32ToAscii( rangeEnd, - rangeValueBuffer + rangeValueLength, + &rangeValueBuffer[ rangeValueLength ], sizeof( rangeValueBuffer ) - rangeValueLength ); } /* Case when request is for bytes in the range [rangeStart, EoF). */ else if( rangeStartOrlastNbytes >= 0 ) { /* Write the "-" character to the buffer.*/ - *( rangeValueBuffer + rangeValueLength ) = DASH_CHARACTER; + rangeValueBuffer[ rangeValueLength ] = DASH_CHARACTER; rangeValueLength += DASH_CHARACTER_LEN; } else @@ -1565,10 +1547,10 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, { /* Write " HTTP/1.1\r\n" to start the HTTP header. */ ( void ) memcpy( pBufferCur, pMethod, methodLen ); - pBufferCur += methodLen; + pBufferCur = &pBufferCur[ methodLen ]; *pBufferCur = SPACE_CHARACTER; - pBufferCur += SPACE_CHARACTER_LEN; + pBufferCur = &pBufferCur[ SPACE_CHARACTER_LEN ]; /* Use "/" as default value if is NULL. */ if( ( pPath == NULL ) || ( pathLen == 0U ) ) @@ -1576,21 +1558,21 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, ( void ) memcpy( pBufferCur, pHttpEmptyPath, HTTP_EMPTY_PATH_LEN ); - pBufferCur += HTTP_EMPTY_PATH_LEN; + pBufferCur = &pBufferCur[ HTTP_EMPTY_PATH_LEN ]; } else { ( void ) memcpy( pBufferCur, pPath, pathLen ); - pBufferCur += pathLen; + pBufferCur = &pBufferCur[ pathLen ]; } *pBufferCur = SPACE_CHARACTER; - pBufferCur += SPACE_CHARACTER_LEN; + pBufferCur = &pBufferCur[ SPACE_CHARACTER_LEN ]; ( void ) memcpy( pBufferCur, pHttpProtocolVersion, HTTP_PROTOCOL_VERSION_LEN ); - pBufferCur += HTTP_PROTOCOL_VERSION_LEN; + pBufferCur = &pBufferCur[ HTTP_PROTOCOL_VERSION_LEN ]; ( void ) memcpy( pBufferCur, pHeaderLineSeparator, HTTP_HEADER_LINE_SEPARATOR_LEN ); @@ -1880,7 +1862,7 @@ HTTPStatus_t HTTPClient_SendHttpData( const TransportInterface_t * pTransport, lastSendTimeMs = getTimestampMs(); bytesRemaining -= ( size_t ) bytesSent; - pIndex += bytesSent; + pIndex = &pIndex[ bytesSent ]; LogDebug( ( "Sent data over the transport: " "BytesSent=%ld, BytesRemaining=%lu, TotalBytesSent=%lu", ( long int ) bytesSent, @@ -2081,7 +2063,7 @@ HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t { /* Receive the HTTP response data into the pResponse->pBuffer. */ currentReceived = pTransport->recv( pTransport->pNetworkContext, - pResponse->pBuffer + totalReceived, + &pResponse->pBuffer[ totalReceived ], pResponse->bufferLen - totalReceived ); /* Transport receive errors are negative. */ @@ -2157,6 +2139,10 @@ HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t /* There may be dangling data if we parse with do not parse body flag. * We expose this data through body to let the applications access it. */ pResponse->pBody = ( const uint8_t * ) parsingContext.pBufferCur; + + /* MISRA Ref 11.4.1 [Casting pointer to int] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-114 */ + /* coverity[misra_c_2012_rule_11_4_violation] */ pResponse->bodyLen = totalReceived - ( size_t ) ( ( ( uintptr_t ) pResponse->pBody ) - ( ( uintptr_t ) pResponse->pBuffer ) ); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 4710c701..50e72e4a 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,6 +1,6 @@ # Project information. cmake_minimum_required ( VERSION 3.13.0 ) -project ( "coreHTTP unit test" +project ( "coreHTTP tests" VERSION 1.0.0 LANGUAGES C ) @@ -11,6 +11,12 @@ set_property( GLOBAL PROPERTY USE_FOLDERS ON ) set( CMAKE_C_STANDARD 90 ) set( CMAKE_C_STANDARD_REQUIRED ON ) +# If no configuration is defined, turn everything on. +if( NOT DEFINED COV_ANALYSIS AND NOT DEFINED UNITTEST ) + set( COV_ANALYSIS TRUE ) + set( UNITTEST TRUE ) +endif() + # Do not allow in-source build. if( ${PROJECT_SOURCE_DIR} STREQUAL ${PROJECT_BINARY_DIR} ) message( FATAL_ERROR "In-source build is not allowed. Please build in a separate directory, such as ${PROJECT_SOURCE_DIR}/build." ) @@ -32,21 +38,23 @@ set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib ) # ====================== Coverity Analysis Configuration ======================= -# Include filepaths for source and include. -include( ${MODULE_ROOT_DIR}/httpFilePaths.cmake ) +if( COV_ANALYSIS ) + # Include filepaths for source and include. + include( ${MODULE_ROOT_DIR}/httpFilePaths.cmake ) -# Target for Coverity analysis that builds the library. -add_library( coverity_analysis - ${HTTP_SOURCES} ) + # Target for Coverity analysis that builds the library. + add_library( coverity_analysis + ${HTTP_SOURCES} ) -# Build HTTP library target without custom config dependency. -target_compile_definitions( coverity_analysis PUBLIC HTTP_DO_NOT_USE_CUSTOM_CONFIG=1 ) + # Build HTTP library target without custom config dependency. + target_compile_definitions( coverity_analysis PUBLIC HTTP_DO_NOT_USE_CUSTOM_CONFIG=1 ) -# HTTP public include path. -target_include_directories( coverity_analysis PUBLIC ${HTTP_INCLUDE_PUBLIC_DIRS} ) + # HTTP public include path. + target_include_directories( coverity_analysis PUBLIC ${HTTP_INCLUDE_PUBLIC_DIRS} ) -# Build HTTP library target without logging -target_compile_options(coverity_analysis PUBLIC -DNDEBUG ) + # Build HTTP library target without logging + target_compile_options(coverity_analysis PUBLIC -DNDEBUG ) +endif() # ===================== Clone needed third-party libraries ====================== # Define an llhttp parser resource path. @@ -65,45 +73,46 @@ if( NOT EXISTS ${LLHTTP_DIR}/src/llhttp.c ) endif() # ============================ Test configuration ============================== - -# Define a CMock resource path. -set( CMOCK_DIR ${MODULE_ROOT_DIR}/test/unit-test/CMock CACHE INTERNAL "CMock library source directory." ) - -# Include CMock build configuration. -include( unit-test/cmock_build.cmake ) - -# Check if the CMock source directory exists, and if not present, clone the submodule -# if BUILD_CLONE_SUBMODULES configuration is enabled. -if( NOT EXISTS ${CMOCK_DIR}/src ) - # Attempt to clone CMock. - if( ${BUILD_CLONE_SUBMODULES} ) - clone_cmock() - else() - message( FATAL_ERROR "The required submodule CMock does not exist. Either clone it manually, or set BUILD_CLONE_SUBMODULES to 1 to automatically clone it during build." ) +if( UNITTEST ) + # Define a CMock resource path. + set( CMOCK_DIR ${MODULE_ROOT_DIR}/test/unit-test/CMock CACHE INTERNAL "CMock library source directory." ) + + # Include CMock build configuration. + include( unit-test/cmock_build.cmake ) + + # Check if the CMock source directory exists, and if not present, clone the submodule + # if BUILD_CLONE_SUBMODULES configuration is enabled. + if( NOT EXISTS ${CMOCK_DIR}/src ) + # Attempt to clone CMock. + if( ${BUILD_CLONE_SUBMODULES} ) + clone_cmock() + else() + message( FATAL_ERROR "The required submodule CMock does not exist. Either clone it manually, or set BUILD_CLONE_SUBMODULES to 1 to automatically clone it during build." ) + endif() endif() -endif() -# Add unit test and coverage configuration. + # Add unit test and coverage configuration. -# Use CTest utility for managing test runs. This has to be added BEFORE -# defining test targets with add_test() -enable_testing() + # Use CTest utility for managing test runs. This has to be added BEFORE + # defining test targets with add_test() + enable_testing() -# Add build targets for CMock and Unit, required for unit testing. -add_cmock_targets() + # Add build targets for CMock and Unit, required for unit testing. + add_cmock_targets() -# Add function to enable CMock based tests and coverage. -include( ${MODULE_ROOT_DIR}/tools/cmock/create_test.cmake ) + # Add function to enable CMock based tests and coverage. + include( ${MODULE_ROOT_DIR}/tools/cmock/create_test.cmake ) -# Include build configuration for unit tests. -add_subdirectory( unit-test ) + # Include build configuration for unit tests. + add_subdirectory( unit-test ) -# ==================== Coverage Analysis configuration ======================== + # ==================== Coverage Analysis configuration ======================== -# Add a target for running coverage on tests. -add_custom_target( coverage - COMMAND ${CMAKE_COMMAND} -DCMOCK_DIR=${CMOCK_DIR} - -P ${MODULE_ROOT_DIR}/tools/cmock/coverage.cmake - DEPENDS cmock unity core_http_utest core_http_send_utest - WORKING_DIRECTORY ${CMAKE_BINARY_DIR} -) + # Add a target for running coverage on tests. + add_custom_target( coverage + COMMAND ${CMAKE_COMMAND} -DCMOCK_DIR=${CMOCK_DIR} + -P ${MODULE_ROOT_DIR}/tools/cmock/coverage.cmake + DEPENDS cmock unity core_http_utest core_http_send_utest + WORKING_DIRECTORY ${CMAKE_BINARY_DIR} + ) +endif() diff --git a/tools/coverity/misra.config b/tools/coverity/misra.config index 0c7ed84c..66b0bb8e 100644 --- a/tools/coverity/misra.config +++ b/tools/coverity/misra.config @@ -1,62 +1,59 @@ -// MISRA C-2012 Rules - { - version : "2.0", - standard : "c2012", - title: "Coverity MISRA Configuration", - deviations : [ - // Disable the following rules. + "version" : "2.0", + "standard" : "c2012", + "title": "Coverity MISRA Configuration", + "deviations" : [ { - deviation: "Directive 4.5", - reason: "Allow names that MISRA considers ambiguous (such as enum IOT_MQTT_CONNECT and function IotMqtt_Connect)." + "deviation": "Directive 4.5", + "reason": "Allow names that MISRA considers ambiguous (such as enum IOT_MQTT_CONNECT and function IotMqtt_Connect)." }, { - deviation: "Directive 4.6", - reason: "The third-party http-parser library does not use specific-length typedefs for their callback function signatures and public structure fields. http-parser callbacks are implemented in the HTTP Client source and also flags of basic numerical types are checked from the http-parser structure." + "deviation": "Directive 4.6", + "reason": "The third-party http-parser library does not use specific-length typedefs for their callback function signatures and public structure fields. http-parser callbacks are implemented in the HTTP Client source and also flags of basic numerical types are checked from the http-parser structure." }, { - deviation: "Directive 4.8", - reason: "Allow inclusion of unused types. Header files for a specific port, which are needed by all files, may define types that are not used by a specific file." + "deviation": "Directive 4.8", + "reason": "Allow inclusion of unused types. Header files for a specific port, which are needed by all files, may define types that are not used by a specific file." }, { - deviation: "Directive 4.9", - reason: "Allow inclusion of function like macros. The `assert` macro is used throughout the library for parameter validation, and logging is done using function like macros." + "deviation": "Directive 4.9", + "reason": "Allow inclusion of function like macros. The `assert` macro is used throughout the library for parameter validation, and logging is done using function like macros." }, { - deviation: "Rule 2.3", - reason: "Allow unused types. Library headers may define types intended for the application's use, but not used within the library files." + "deviation": "Rule 2.3", + "reason": "Allow unused types. Library headers may define types intended for the application's use, but not used within the library files." }, { - deviation: "Rule 2.4", - reason: "Allow unused tags. Some compilers warn if types are not tagged." + "deviation": "Rule 2.4", + "reason": "Allow unused tags. Some compilers warn if types are not tagged." }, { - deviation: "Rule 2.5", - reason: "Allow unused macros. Library headers may define macros intended for the application's use, but not used by a specific file." + "deviation": "Rule 2.5", + "reason": "Allow unused macros. Library headers may define macros intended for the application's use, but not used by a specific file." }, { - deviation: "Rule 3.1", - reason: "Allow nested comments. C++ style `//` comments are used in example code within Doxygen documentation blocks." + "deviation": "Rule 3.1", + "reason": "Allow nested comments. C++ style `//` comments are used in example code within Doxygen documentation blocks." }, { - deviation: "Rule 8.7", - reason: "API functions are not used by the library outside of the files they are defined; however, they must be externally visible in order to be used by an application." + "deviation": "Rule 8.7", + "reason": "API functions are not used by the library outside of the files they are defined; however, they must be externally visible in order to be used by an application." }, { - deviation: "Rule 8.13", - reason: "The third-party http-parser library callback definitions have a non-const parameter which holds the state of the parsing. This parameter is never updated in the callback implementations, but have fields that may be read." + "deviation": "Rule 8.13", + "reason": "The third-party http-parser library callback definitions have a non-const parameter which holds the state of the parsing. This parameter is never updated in the callback implementations, but have fields that may be read." }, { - deviation: "Rule 11.5", - reason: "Allow casts from `void *`. The third-party http-parser library callback contexts are saved as `void *` and must be cast to the correct data type before use. |" + "deviation": "Rule 11.5", + "reason": "Allow casts from `void *`. The third-party http-parser library callback contexts are saved as `void *` and must be cast to the correct data type before use. |" }, { - deviation: "Rule 21.1", - reason: "Allow use of all names." + "deviation": "Rule 21.1", + "reason": "Allow use of all names." }, { - deviation: "Rule 21.2", - reason: "Allow use of all names." + "deviation": "Rule 21.2", + "reason": "Allow use of all names." } ] }