From 9bc41be69178ed8df1b20ba8adf1a0c1713ccf90 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 15 Jul 2022 23:29:09 +0000 Subject: [PATCH 01/18] Moved some rules around in the MISRA.md file, added a misra.config file to the repo, slight changes to the client for MISRA/Coverity compliance. --- MISRA.md | 10 +++--- source/core_http_client.c | 28 +++++++++++----- test/CMakeLists.txt | 1 + tools/coverity/misra.config | 67 +++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 13 deletions(-) create mode 100644 tools/coverity/misra.config diff --git a/MISRA.md b/MISRA.md index 60b36021..69af5592 100644 --- a/MISRA.md +++ b/MISRA.md @@ -14,21 +14,23 @@ include the third-party [http-parser source code](https://github.com/nodejs/http | Rule 2.4 | Advisory | Allow unused tags. Some compilers warn if types are not tagged. | | Rule 2.5 | Advisory | Allow unused macros. Library headers may define macros intended for the application's use, but are not used by a specific file. | | Rule 3.1 | Required | Allow nested comments. C++ style `//` comments are used in example code within Doxygen documentation blocks. | +| Rule 8.7 | Advisory | 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. | +| Rule 8.13 | Advisory | 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. | | Rule 11.5 | Advisory | 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. | +| Rule 18.3 | Required | It is expected that the current location http-parser passes into the body parsing callback points to the same user response buffer; if it does not, an assertion is triggered. | ### Flagged by Coverity | Deviation | Category | Justification | | :-: | :-: | :-- | | Directive 4.6 | Advisory | 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. -| Rule 8.7 | Advisory | 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. | -| Rule 8.13 | Advisory | 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. | | Rule 10.5 | Advisory | The third-party http-parser library has a structure with a field of type unsigned int whose values are intended to be mapped to an enum. This field contains error codes used by the HTTP client library. | -| Rule 14.3 | Required | 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. | ### Suppressed with Coverity Comments | Deviation | Category | Justification | | :-: | :-: | :-- | | Rule 5.4 | Required | The length of string literal macro identifiers are labeled with the identifier of the string literal itself postfixed with "_LEN" for clarity. This is consistent throughout the library. | +| Rule 10.3 | Required | The character comparison involves casting the characters to ints, which sets of coverity. | Rule 10.8 | Required | The size of the headers is found by taking the current location being parsed and subtracting it from the start of the headers. The start of the headers is set on the first header field found from http-parser. This always comes before finding the header length; if it does not, an assertion is triggered. | +| Rule 13.2 | Required | Coverity scans says that the order that the toupper() calls is run can impact the if statement, which is not true. | Rule 11.8 | Required | If the response body uses chunked transfer encoding, then it is necessary to copy over the chunk headers with the data to which the current parsing location points. The current parsing location is returned to the callback implementation as a `const char *`. | -| Rule 18.3 | Required | It is expected that the current location http-parser passes into the body parsing callback points to the same user response buffer; if it does not, an assertion is triggered. | +| Rule 14.3 | Required | 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. | diff --git a/source/core_http_client.c b/source/core_http_client.c index ba27499e..4eff601a 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -24,8 +24,11 @@ * @file core_http_client.c * @brief Implements the user-facing functions in core_http_client.h. */ - -#include +#ifdef DISABLE_LOGGING + #define assert( x ) +#else /* !DISABLE_LOGGING */ + #include +#endif #include #include @@ -577,7 +580,9 @@ static int8_t caseInsensitiveStringCmp( const char * str1, for( i = 0U; i < n; i++ ) { - if( toupper( str1[ i ] ) != toupper( str2[ i ] ) ) + /* coverity[misra_c_2012_rule_10_3_violation] */ + /* coverity[misra_c_2012_rule_13_2_violation] */ + if( ( toupper( ( (unsigned char) str1[ i ] ) ) ) != ( toupper( ( (unsigned char) str2[ i ] ) ) ) ) { break; } @@ -843,6 +848,7 @@ static int httpParserOnHeadersCompleteCallback( llhttp_t * pHttpParser ) /* If the Content-Length header was found, then pHttpParser->content_length * will not be equal to the maximum 64 bit integer. */ + /* coverity[misra_c_2012_rule_14_3_violation] */ if( pHttpParser->content_length != UINT64_MAX ) { pResponse->contentLength = ( size_t ) ( pHttpParser->content_length ); @@ -1980,7 +1986,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT const HTTPRequestHeaders_t * pRequestHeaders ) { HTTPStatus_t returnStatus = HTTPSuccess; - size_t totalReceived = 0U; + uint64_t totalReceived = 0U; int32_t currentReceived = 0; HTTPParsingContext_t parsingContext = { 0 }; uint8_t shouldRecv = 1U, shouldParse = 1U, timeoutReached = 0U; @@ -2034,7 +2040,9 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT * Because we cannot know how large the HTTP response will be in * total, parsing will tell us if the end of the message is reached.*/ shouldParse = 1U; - totalReceived += currentReceived; + /* Coverity compliance requires the cast to an unsigned type, since we've checked that + * the value of currentReceived is greater than 0 we don't need to worry about int overflow. */ + totalReceived += (size_t) currentReceived; } else { @@ -2058,10 +2066,12 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT { /* Data is received into the buffer is immediately parsed. Parsing * is invoked even with a length of zero. A length of zero indicates - * to the parser that there is no more data from the server (EOF). */ + * to the parser that there is no more data from the server (EOF). + * Additionally coverity compliance requires the cast to a larger type, but since we + * know that the value is greater than 0 we don't need to worry about int overflow. */ returnStatus = parseHttpResponse( &parsingContext, pResponse, - currentReceived ); + (uint64_t) currentReceived ); } /* Reading should continue if there are no errors in the transport receive @@ -2324,7 +2334,7 @@ static int findHeaderValueParserCallback( llhttp_t * pHttpParser, /* As we have found the value associated with the header, we don't need * to parse the response any further. */ - retCode = LLHTTP_STOP_PARSING; + retCode = (int) LLHTTP_STOP_PARSING; } else { @@ -2338,7 +2348,7 @@ static int findHeaderValueParserCallback( llhttp_t * pHttpParser, static int findHeaderOnHeaderCompleteCallback( llhttp_t * pHttpParser ) { - findHeaderContext_t * pContext = NULL; + const findHeaderContext_t * pContext = NULL; /* Disable unused parameter warning. */ ( void ) pHttpParser; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index d92a942e..87bae881 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -45,6 +45,7 @@ target_compile_definitions( coverity_analysis PUBLIC HTTP_DO_NOT_USE_CUSTOM_CONF # HTTP public include path. target_include_directories( coverity_analysis PUBLIC ${HTTP_INCLUDE_PUBLIC_DIRS} ) + target_compile_options(coverity_analysis PUBLIC -DDISABLE_LOGGING ) # ===================== Clone needed third-party libraries ====================== # Define an llhttp paser resource path. diff --git a/tools/coverity/misra.config b/tools/coverity/misra.config new file mode 100644 index 00000000..a734de3c --- /dev/null +++ b/tools/coverity/misra.config @@ -0,0 +1,67 @@ +// MISRA C-2012 Rules + +{ + version : "2.0", + standard : "c2012", + title: "Coverity MISRA Configuration", + deviations : [ + // Disable the following rules. + { + 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.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. 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.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 3.1", + reason: "Allow nested comments. Documentation blocks contain comments for example code." + }, + { + deviation: "Rule 8.7", + reason: "API functions are not used by library. They must be externally visible in order to be used by the 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 11.5", + reason: "Allow casts from void *. Contexts are passed as void * and must be cast to the correct data type before use." + }, + { + deviation: "Rule 18.3", + reason: "It is expected that the current location http-parser passes into the body parsing callback points to the same user response buffer; if it does not, an assertion is triggered." + }, + { + deviation: "Rule 21.1", + reason: "Allow use of all names." + }, + { + deviation: "Rule 21.2", + reason: "Allow use of all names." + } + ] +} From eaf27f1f3825c0d606e20b8c88342374012a908e Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Mon, 18 Jul 2022 16:59:53 +0000 Subject: [PATCH 02/18] Fixing a "typo" that caused the formatting pull request check to fail --- source/core_http_client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index 4eff601a..c04c6c08 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -2040,8 +2040,8 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT * Because we cannot know how large the HTTP response will be in * total, parsing will tell us if the end of the message is reached.*/ shouldParse = 1U; - /* Coverity compliance requires the cast to an unsigned type, since we've checked that - * the value of currentReceived is greater than 0 we don't need to worry about int overflow. */ + /* Coverity compliance requires the cast to an unsigned type, since we have checked that + * the value of current received is greater than 0 we don't need to worry about int overflow. */ totalReceived += (size_t) currentReceived; } else From 1372fbc8f8331c0692117bf1736eff5791e02c79 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 29 Jul 2022 00:17:59 +0000 Subject: [PATCH 03/18] Changes to MISRA.md to reflect new format. Removed MISRA warnings in caseInsensitiveStringCmp function. Changed in line comments to reflect new MISRA.md format. Modified misra.config reasons. --- MISRA.md | 111 ++++++++++++++++------ source/core_http_client.c | 32 +++---- source/include/core_http_client_private.h | 30 +++--- test/CMakeLists.txt | 3 +- tools/coverity/misra.config | 9 +- 5 files changed, 112 insertions(+), 73 deletions(-) diff --git a/MISRA.md b/MISRA.md index 69af5592..f10f850c 100644 --- a/MISRA.md +++ b/MISRA.md @@ -2,35 +2,86 @@ The HTTP Client library files conform to the [MISRA C:2012](https://www.misra.org.uk) guidelines, with some noted exceptions. Compliance is checked with Coverity static analysis. -Deviations from the MISRA standard are listed below. The deviations below do not -include the third-party [http-parser source code](https://github.com/nodejs/http-parser/tree/v2.9.3): - -### Ignored by [Coverity Configuration](https://github.com/aws/aws-iot-device-sdk-embedded-C/blob/main/tools/coverity/misra.config) -| Deviation | Category | Justification | -| :-: | :-: | :-- | -| Directive 4.5 | Advisory | Allow names that MISRA considers ambiguous (such as LogInfo and LogError). | -| Directive 4.8 | Advisory | 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. | -| Directive 4.9 | Advisory | 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. | -| Rule 2.4 | Advisory | Allow unused tags. Some compilers warn if types are not tagged. | -| Rule 2.5 | Advisory | Allow unused macros. Library headers may define macros intended for the application's use, but are not used by a specific file. | -| Rule 3.1 | Required | Allow nested comments. C++ style `//` comments are used in example code within Doxygen documentation blocks. | -| Rule 8.7 | Advisory | 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. | -| Rule 8.13 | Advisory | 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. | -| Rule 11.5 | Advisory | 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. | -| Rule 18.3 | Required | It is expected that the current location http-parser passes into the body parsing callback points to the same user response buffer; if it does not, an assertion is triggered. | - -### Flagged by Coverity -| Deviation | Category | Justification | -| :-: | :-: | :-- | -| Directive 4.6 | Advisory | 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. -| Rule 10.5 | Advisory | The third-party http-parser library has a structure with a field of type unsigned int whose values are intended to be mapped to an enum. This field contains error codes used by the HTTP client library. | +The specific deviations, suppressed inline, are listed below. + +Additionally, [MISRA configuration file](https://github.com/FreeRTOS/coreHTTP/blob/main/tools/coverity/misra.config) contains the project wide deviations. ### Suppressed with Coverity Comments -| Deviation | Category | Justification | -| :-: | :-: | :-- | -| Rule 5.4 | Required | The length of string literal macro identifiers are labeled with the identifier of the string literal itself postfixed with "_LEN" for clarity. This is consistent throughout the library. | -| Rule 10.3 | Required | The character comparison involves casting the characters to ints, which sets of coverity. -| Rule 10.8 | Required | The size of the headers is found by taking the current location being parsed and subtracting it from the start of the headers. The start of the headers is set on the first header field found from http-parser. This always comes before finding the header length; if it does not, an assertion is triggered. | -| Rule 13.2 | Required | Coverity scans says that the order that the toupper() calls is run can impact the if statement, which is not true. -| Rule 11.8 | Required | If the response body uses chunked transfer encoding, then it is necessary to copy over the chunk headers with the data to which the current parsing location points. The current parsing location is returned to the callback implementation as a `const char *`. | -| Rule 14.3 | Required | 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. | +To find the violation references in the source files run grep on the source code +with ( Assuming rule 5.4 violation; with justification in point 2 ): +``` +grep 'MISRA Ref 5.4.2' . -rI +``` + +#### Rule 5.4 +_Ref 5.4-1_ + +- MISRA Rule 5.4 flags the following macro's name as ambiguous from the + one postfixed with _LEN. This rule is suppressed for naming consistency with + other HTTP header field and value string and length macros in this file. + +_Ref 5.4-2_ + +- MISRA Rule 5.4 flags the following macro's name as ambiguous from the one + above it. This rule is suppressed for naming consistency with other HTTP + header field and value string and length macros in this file. + +_Ref 5.4-3_ + +- MISRA Rule 5.4 flags the following macro's name as ambiguous from the one + postfixed with _LEN. This rule is suppressed for naming consistency with + other HTTP header field and value string and length macros in this file. + +_Ref 5.4-4_ + +- MISRA Rule 5.4 flags the following macro's name as ambiguous from the one + above it. This rule is suppressed for naming consistency with other HTTP + header field and value string and length macros in this file. + +_Ref 5.4-5_ + +- MISRA Rule 5.4 flags the following macro's name as ambiguous from the one + postfixed with _LEN. This rule is suppressed for naming consistency with + other HTTP header field and value string and length macros in this file. + +_Ref 5.4-6_ + +- MISRA Rule 5.4 flags the following macro's name as ambiguous from the one + above it. This rule is suppressed for naming consistency with other HTTP + header field and value string and length macros in this file. + +#### Rule 10.8 +_Ref 10.8.1_ + +- MISRA Rule 10.8 The size of the headers is found by taking the current location + being parsed and subtracting it from the start of the headers. The start of + the headers is set on the first header field found from http-parser. This always + comes before finding the header length; if it does not, an assertion is triggered. + This rule is suppressed because in the previous statement it is + asserted that the pointer difference will never be negative. + +#### Rule 14.3 +_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 + will not be equal to the maximum 64 bit integer. + +#### Rule 11.8 +_Ref 11.8.1_ + +- MISRA Rule 11.8 flags casting away the const qualifier in the pointer + type. This rule is suppressed because when the body is of transfer + encoding chunked, the body must be copied over the chunk headers that + precede it. This is done to have a contiguous response body. This does + affect future parsing as the changed segment will always be before the + next place to parse. + +#### Rule 18.3 +_Ref 18.3.1_ + +- MISRA Rule 18.3 flags pLoc and pNextWriteLoc as pointing to two different + objects. This rule is suppressed because both pNextWriteLoc and pLoc + point to a location in the response buffer. */ diff --git a/source/core_http_client.c b/source/core_http_client.c index c04c6c08..988dc7c5 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -577,12 +577,14 @@ static int8_t caseInsensitiveStringCmp( const char * str1, size_t n ) { size_t i = 0U; - + /* Inclusion of inbetween variables for coverity rule 13.2 compliance */ + uint32_t firstChar; + uint32_t secondChar; for( i = 0U; i < n; i++ ) { - /* coverity[misra_c_2012_rule_10_3_violation] */ - /* coverity[misra_c_2012_rule_13_2_violation] */ - if( ( toupper( ( (unsigned char) str1[ i ] ) ) ) != ( toupper( ( (unsigned char) str2[ i ] ) ) ) ) + firstChar = ( uint32_t ) toupper( ( ( int32_t ) ( unsigned char ) str1[ i ] ) ); + secondChar = ( uint32_t ) toupper( ( ( int32_t ) ( unsigned char ) str2[ i ] ) ); + if( firstChar != secondChar) { break; } @@ -834,10 +836,8 @@ static int httpParserOnHeadersCompleteCallback( llhttp_t * pHttpParser ) /* The start of the headers ALWAYS come before the the end of the headers. */ assert( ( const char * ) ( pResponse->pHeaders ) < pParsingContext->pBufferCur ); - /* MISRA Rule 10.8 flags the following line for casting from a signed - * pointer difference to a size_t. This rule is suppressed because in - * in the previous statement it is asserted that the pointer difference - * will never be negative. */ + /* MISRA Ref 10.8.1 [Essential type casting] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-108 */ /* coverity[misra_c_2012_rule_10_8_violation] */ pResponse->headersLen = ( size_t ) ( pParsingContext->pBufferCur - ( const char * ) ( pResponse->pHeaders ) ); } @@ -846,8 +846,8 @@ static int httpParserOnHeadersCompleteCallback( llhttp_t * pHttpParser ) pResponse->headersLen = 0U; } - /* If the Content-Length header was found, then pHttpParser->content_length - * will not be equal to the maximum 64 bit integer. */ + /* MISRA Ref 14.3.1 [Configuration dependent invariant] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-143 */ /* coverity[misra_c_2012_rule_14_3_violation] */ if( pHttpParser->content_length != UINT64_MAX ) { @@ -930,12 +930,8 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, /* The next location to write. */ - /* MISRA Rule 11.8 flags casting away the const qualifier in the pointer - * type. This rule is suppressed because when the body is of transfer - * encoding chunked, the body must be copied over the chunk headers that - * precede it. This is done to have a contiguous response body. This does - * affect future parsing as the changed segment will always be before the - * next place to parse. */ + /* MISRA Ref 11.8.1 [Function pointer and use of const 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 ); @@ -944,9 +940,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, * and must be moved up in the buffer. When pLoc is greater than the current * end of the body, that signals the parser found a chunk header. */ - /* MISRA Rule 18.3 flags pLoc and pNextWriteLoc as pointing to two different - * objects. This rule is suppressed because both pNextWriteLoc and pLoc - * point to a location in the response buffer. */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-183 */ /* coverity[pointer_parameter] */ /* coverity[misra_c_2012_rule_18_3_violation] */ if( pLoc > pNextWriteLoc ) diff --git a/source/include/core_http_client_private.h b/source/include/core_http_client_private.h index fc2ac28b..b2b146da 100644 --- a/source/include/core_http_client_private.h +++ b/source/include/core_http_client_private.h @@ -103,41 +103,35 @@ /* Constants for header values added based on flags. */ -/* MISRA Rule 5.4 flags the following macro's name as ambiguous from the - * one postfixed with _LEN. This rule is suppressed for naming consistency with - * other HTTP header field and value string and length macros in this file.*/ +/* MISRA Ref 5.4.1 [Macro identifiers] */ +/* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-54 */ /* coverity[other_declaration] */ #define HTTP_CONNECTION_KEEP_ALIVE_VALUE "keep-alive" /**< HTTP header value "keep-alive" for the "Connection" header field. */ -/* MISRA Rule 5.4 flags the following macro's name as ambiguous from the one - * above it. This rule is suppressed for naming consistency with other HTTP - * header field and value string and length macros in this file.*/ +/* MISRA Ref 5.4.2 [Macro identifiers] */ +/* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-54 */ /* coverity[misra_c_2012_rule_5_4_violation] */ #define HTTP_CONNECTION_KEEP_ALIVE_VALUE_LEN ( sizeof( HTTP_CONNECTION_KEEP_ALIVE_VALUE ) - 1U ) /**< The length of #HTTP_CONNECTION_KEEP_ALIVE_VALUE. */ /* Constants relating to Range Requests. */ -/* MISRA Rule 5.4 flags the following macro's name as ambiguous from the - * one postfixed with _LEN. This rule is suppressed for naming consistency with - * other HTTP header field and value string and length macros in this file.*/ +/* MISRA Ref 5.4.3 [Macro identifiers] */ +/* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-54 */ /* coverity[other_declaration] */ #define HTTP_RANGE_REQUEST_HEADER_FIELD "Range" /**< HTTP header field "Range". */ -/* MISRA Rule 5.4 flags the following macro's name as ambiguous from the one - * above it. This rule is suppressed for naming consistency with other HTTP - * header field and value string and length macros in this file.*/ +/* MISRA Ref 5.4.4 [Macro identifiers] */ +/* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-54 */ /* coverity[misra_c_2012_rule_5_4_violation] */ #define HTTP_RANGE_REQUEST_HEADER_FIELD_LEN ( sizeof( HTTP_RANGE_REQUEST_HEADER_FIELD ) - 1U ) /**< The length of #HTTP_RANGE_REQUEST_HEADER_FIELD. */ -/* MISRA Rule 5.4 flags the following macro's name as ambiguous from the - * one postfixed with _LEN. This rule is suppressed for naming consistency with - * other HTTP header field and value string and length macros in this file.*/ +/* MISRA Ref 5.4.5 [Macro identifiers] */ +/* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-54 */ /* coverity[other_declaration] */ #define HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX "bytes=" /**< HTTP required header value prefix when specifying a byte range for partial content. */ -/* MISRA Rule 5.4 flags the following macro's name as ambiguous from the one - * above it. This rule is suppressed for naming consistency with other HTTP - * header field and value string and length macros in this file.*/ +/* MISRA Ref 5.4.6 [Macro identifiers] */ +/* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-54 */ /* coverity[misra_c_2012_rule_5_4_violation] */ #define HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX_LEN ( sizeof( HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX ) - 1U ) /**< The length of #HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX. */ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 87bae881..9075a972 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -45,7 +45,8 @@ target_compile_definitions( coverity_analysis PUBLIC HTTP_DO_NOT_USE_CUSTOM_CONF # HTTP public include path. target_include_directories( coverity_analysis PUBLIC ${HTTP_INCLUDE_PUBLIC_DIRS} ) - target_compile_options(coverity_analysis PUBLIC -DDISABLE_LOGGING ) +# Build HTTP library target without logging +target_compile_options(coverity_analysis PUBLIC -DDISABLE_LOGGING ) # ===================== Clone needed third-party libraries ====================== # Define an llhttp paser resource path. diff --git a/tools/coverity/misra.config b/tools/coverity/misra.config index a734de3c..46593218 100644 --- a/tools/coverity/misra.config +++ b/tools/coverity/misra.config @@ -20,7 +20,7 @@ }, { deviation: "Directive 4.9", - reason: "Allow inclusion of function like macros. Logging is done using function like macros." + 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", @@ -34,14 +34,13 @@ 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. Documentation blocks contain comments for example code." + 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 library. They must be externally visible in order to be used by the application." + 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", @@ -49,7 +48,7 @@ }, { deviation: "Rule 11.5", - reason: "Allow casts from void *. Contexts are passed as void * and must be cast to the correct data type before use." + 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 18.3", From a1f69e25c282ad9993b99369258f1b7207cefb9d Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 29 Jul 2022 00:59:46 +0000 Subject: [PATCH 04/18] Trying to fix formatting issues --- source/core_http_client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index affdec19..b578d951 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -2036,7 +2036,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT shouldParse = 1U; /* Coverity compliance requires the cast to an unsigned type, since we have checked that * the value of current received is greater than 0 we don't need to worry about int overflow. */ - totalReceived += (size_t) currentReceived; + totalReceived += ( size_t ) currentReceived; } else { @@ -2065,7 +2065,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT * know that the value is greater than 0 we don't need to worry about int overflow. */ returnStatus = parseHttpResponse( &parsingContext, pResponse, - (uint64_t) currentReceived ); + ( uint64_t ) currentReceived ); } /* Reading should continue if there are no errors in the transport receive @@ -2328,7 +2328,7 @@ static int findHeaderValueParserCallback( llhttp_t * pHttpParser, /* As we have found the value associated with the header, we don't need * to parse the response any further. */ - retCode = (int) LLHTTP_STOP_PARSING; + retCode = ( int ) LLHTTP_STOP_PARSING; } else { From f0eb2289c18336400699811d2398fe53dbe82e3f Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 29 Jul 2022 01:09:51 +0000 Subject: [PATCH 05/18] Added two words to lexicon, another attempt at formatting fix --- lexicon.txt | 2 ++ source/core_http_client.c | 1 + 2 files changed, 3 insertions(+) diff --git a/lexicon.txt b/lexicon.txt index 3a308b07..3dd9066f 100644 --- a/lexicon.txt +++ b/lexicon.txt @@ -59,6 +59,7 @@ findheaderinresponse findheaderonheadercompletecallback findheadervalueparsercallback firstpartbytes +freertos gcc getfinalresponsestatus gettime @@ -108,6 +109,7 @@ httpstatus httpsuccess ietf ifndef +inbetween inc ingroup init diff --git a/source/core_http_client.c b/source/core_http_client.c index b578d951..75f35127 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -2034,6 +2034,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT * Because we cannot know how large the HTTP response will be in * total, parsing will tell us if the end of the message is reached.*/ shouldParse = 1U; + /* Coverity compliance requires the cast to an unsigned type, since we have checked that * the value of current received is greater than 0 we don't need to worry about int overflow. */ totalReceived += ( size_t ) currentReceived; From 0c1fc94757263e20dd756292caca53cca4df99e7 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 29 Jul 2022 09:35:09 -0700 Subject: [PATCH 06/18] Crustify fix --- source/core_http_client.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index 75f35127..0eb5e2c3 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -580,11 +580,13 @@ static int8_t caseInsensitiveStringCmp( const char * str1, /* Inclusion of inbetween variables for coverity rule 13.2 compliance */ uint32_t firstChar; uint32_t secondChar; + for( i = 0U; i < n; i++ ) { firstChar = ( uint32_t ) toupper( ( ( int32_t ) ( unsigned char ) str1[ i ] ) ); secondChar = ( uint32_t ) toupper( ( ( int32_t ) ( unsigned char ) str2[ i ] ) ); - if( firstChar != secondChar) + + if( firstChar != secondChar ) { break; } From e9a4291605648fa3409cf2df6c543401f9c8133d Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 29 Jul 2022 20:18:40 +0000 Subject: [PATCH 07/18] The cast to (unsigned char) before passing the char[index] value into toupper() was causing the CBMC test to fail. Through much trial and error it appears there isn't a way around it. So adding in a MISRA exception for the rule related to that cast. --- MISRA.md | 10 +++++++++- source/core_http_client.c | 18 +++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/MISRA.md b/MISRA.md index f10f850c..49fd1208 100644 --- a/MISRA.md +++ b/MISRA.md @@ -84,4 +84,12 @@ _Ref 18.3.1_ - MISRA Rule 18.3 flags pLoc and pNextWriteLoc as pointing to two different objects. This rule is suppressed because both pNextWriteLoc and pLoc - point to a location in the response buffer. */ + point to a location in the response buffer. + +#### Rule 21.13 +_Ref 21.13.1_ + +- MISRA Rule 21.13 flags any value passed into a ctype.h function that isn't cast + as an unsigned char. Thorough testing by use of our CBMC proofs shows that adding + the cast to ( unsigned char ) inside of the toupper() call has potential to lead + to errors. Due to this we supress this warning for our use case. diff --git a/source/core_http_client.c b/source/core_http_client.c index 0eb5e2c3..9a3b67d9 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -578,15 +578,22 @@ static int8_t caseInsensitiveStringCmp( const char * str1, { size_t i = 0U; /* Inclusion of inbetween variables for coverity rule 13.2 compliance */ - uint32_t firstChar; - uint32_t secondChar; + int32_t firstChar; + int32_t secondChar; for( i = 0U; i < n; i++ ) { - firstChar = ( uint32_t ) toupper( ( ( int32_t ) ( unsigned char ) str1[ i ] ) ); - secondChar = ( uint32_t ) toupper( ( ( int32_t ) ( unsigned char ) str2[ i ] ) ); + /* MISRA Ref 21.13.1 [Essential type casting] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ + /* coverity[misra_c_2012_rule_21_13_violation] */ + firstChar = ( int32_t ) toupper( ( ( int32_t ) str1[ i ] ) ); - if( firstChar != secondChar ) + /* MISRA Ref 21.13.1 [Essential Type Casting] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ + /* coverity[misra_c_2012_rule_21_13_violation] */ + secondChar = ( int32_t ) toupper( ( ( int32_t ) str2[ i ] ) ); + + if( firstChar != secondChar ) { break; } @@ -595,6 +602,7 @@ static int8_t caseInsensitiveStringCmp( const char * str1, return ( i == n ) ? 0 : 1; } + /*-----------------------------------------------------------*/ static void processCompleteHeader( HTTPParsingContext_t * pParsingContext ) From 5ad224e301e8e34576036962c20cd4ef46cacacf Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 29 Jul 2022 13:38:59 -0700 Subject: [PATCH 08/18] Formatting fix --- source/core_http_client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index 9a3b67d9..fdc1d6db 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -586,14 +586,14 @@ static int8_t caseInsensitiveStringCmp( const char * str1, /* MISRA Ref 21.13.1 [Essential type casting] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ /* coverity[misra_c_2012_rule_21_13_violation] */ - firstChar = ( int32_t ) toupper( ( ( int32_t ) str1[ i ] ) ); + firstChar = ( int32_t ) toupper( ( ( int32_t ) str1[ i ] ) ); /* MISRA Ref 21.13.1 [Essential Type Casting] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ /* coverity[misra_c_2012_rule_21_13_violation] */ secondChar = ( int32_t ) toupper( ( ( int32_t ) str2[ i ] ) ); - if( firstChar != secondChar ) + if( firstChar != secondChar ) { break; } From e9123cc962f5a3a12bf433eb92fea49e46c75ac8 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Mon, 1 Aug 2022 18:40:07 +0000 Subject: [PATCH 09/18] Changing DISABLE_LOGGING to DISABLE_ASSERT to be accurate --- source/core_http_client.c | 4 ++-- test/CMakeLists.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index fdc1d6db..e6d0bab9 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -24,9 +24,9 @@ * @file core_http_client.c * @brief Implements the user-facing functions in core_http_client.h. */ -#ifdef DISABLE_LOGGING +#ifdef DISABLE_ASSERT #define assert( x ) -#else /* !DISABLE_LOGGING */ +#else /* !DISABLE_ASSERT */ #include #endif #include diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 9075a972..ac1c67fa 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -46,7 +46,7 @@ target_compile_definitions( coverity_analysis PUBLIC HTTP_DO_NOT_USE_CUSTOM_CONF target_include_directories( coverity_analysis PUBLIC ${HTTP_INCLUDE_PUBLIC_DIRS} ) # Build HTTP library target without logging -target_compile_options(coverity_analysis PUBLIC -DDISABLE_LOGGING ) +target_compile_options(coverity_analysis PUBLIC -DDISABLE_ASSERT ) # ===================== Clone needed third-party libraries ====================== # Define an llhttp paser resource path. From 55c9d2178e853cac6bcff886d0531dafae6a5fb6 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 2 Aug 2022 16:23:42 +0000 Subject: [PATCH 10/18] Removing an extra cast that wasn't needed --- source/core_http_client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index e6d0bab9..e18df4b0 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -586,12 +586,12 @@ static int8_t caseInsensitiveStringCmp( const char * str1, /* MISRA Ref 21.13.1 [Essential type casting] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ /* coverity[misra_c_2012_rule_21_13_violation] */ - firstChar = ( int32_t ) toupper( ( ( int32_t ) str1[ i ] ) ); + firstChar = toupper( ( ( int32_t ) str1[ i ] ) ); /* MISRA Ref 21.13.1 [Essential Type Casting] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ /* coverity[misra_c_2012_rule_21_13_violation] */ - secondChar = ( int32_t ) toupper( ( ( int32_t ) str2[ i ] ) ); + secondChar = toupper( ( ( int32_t ) str2[ i ] ) ); if( firstChar != secondChar ) { From db5eef38382ab09401558344069a1b2322096986 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 2 Aug 2022 22:39:44 +0000 Subject: [PATCH 11/18] Moving to DNDEBUG for removal of assert. Change to str compare function to see if it passes github checks --- source/core_http_client.c | 28 ++++++++-------------------- test/CMakeLists.txt | 2 +- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index e18df4b0..620122b1 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -24,11 +24,7 @@ * @file core_http_client.c * @brief Implements the user-facing functions in core_http_client.h. */ -#ifdef DISABLE_ASSERT - #define assert( x ) -#else /* !DISABLE_ASSERT */ - #include -#endif +#include #include #include @@ -559,8 +555,8 @@ static HTTPStatus_t processLlhttpError( const llhttp_t * pHttpParser ); * 0 if str1 is equal to str2 * 1 if str1 is not equal to str2. */ -static int8_t caseInsensitiveStringCmp( const char * str1, - const char * str2, +static int8_t caseInsensitiveStringCmp( const unsigned char * str1, + const unsigned char * str2, size_t n ); /*-----------------------------------------------------------*/ @@ -572,28 +568,20 @@ static uint32_t getZeroTimestampMs( void ) /*-----------------------------------------------------------*/ -static int8_t caseInsensitiveStringCmp( const char * str1, - const char * str2, +static int8_t caseInsensitiveStringCmp( const unsigned char * str1, + const unsigned char * str2, size_t n ) { size_t i = 0U; /* Inclusion of inbetween variables for coverity rule 13.2 compliance */ int32_t firstChar; int32_t secondChar; - for( i = 0U; i < n; i++ ) { - /* MISRA Ref 21.13.1 [Essential type casting] */ - /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ - /* coverity[misra_c_2012_rule_21_13_violation] */ - firstChar = toupper( ( ( int32_t ) str1[ i ] ) ); - - /* MISRA Ref 21.13.1 [Essential Type Casting] */ - /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-2113 */ - /* coverity[misra_c_2012_rule_21_13_violation] */ + firstChar = toupper( ( int32_t ) ( str1[ i ] ) ); secondChar = toupper( ( ( int32_t ) str2[ i ] ) ); - if( firstChar != secondChar ) + if( ( firstChar ) != ( secondChar ) ) { break; } @@ -1990,7 +1978,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT const HTTPRequestHeaders_t * pRequestHeaders ) { HTTPStatus_t returnStatus = HTTPSuccess; - uint64_t totalReceived = 0U; + size_t totalReceived = 0U; int32_t currentReceived = 0; HTTPParsingContext_t parsingContext = { 0 }; uint8_t shouldRecv = 1U, shouldParse = 1U, timeoutReached = 0U; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index ac1c67fa..6d27d46f 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -46,7 +46,7 @@ target_compile_definitions( coverity_analysis PUBLIC HTTP_DO_NOT_USE_CUSTOM_CONF target_include_directories( coverity_analysis PUBLIC ${HTTP_INCLUDE_PUBLIC_DIRS} ) # Build HTTP library target without logging -target_compile_options(coverity_analysis PUBLIC -DDISABLE_ASSERT ) +target_compile_options(coverity_analysis PUBLIC -DNDEBUG ) # ===================== Clone needed third-party libraries ====================== # Define an llhttp paser resource path. From 04592bd4f81b7612c377e071acd60beb01b597af Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 2 Aug 2022 15:45:07 -0700 Subject: [PATCH 12/18] Formatting --- source/core_http_client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source/core_http_client.c b/source/core_http_client.c index 620122b1..e5f4f49f 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -576,6 +576,7 @@ static int8_t caseInsensitiveStringCmp( const unsigned char * str1, /* Inclusion of inbetween variables for coverity rule 13.2 compliance */ int32_t firstChar; int32_t secondChar; + for( i = 0U; i < n; i++ ) { firstChar = toupper( ( int32_t ) ( str1[ i ] ) ); From aef59b3c85860c35899cb8eb1dc917067d4c4bfe Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Tue, 2 Aug 2022 15:51:07 -0700 Subject: [PATCH 13/18] File size changed --- docs/doxygen/include/size_table.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/doxygen/include/size_table.md b/docs/doxygen/include/size_table.md index fed45453..64dccdd3 100644 --- a/docs/doxygen/include/size_table.md +++ b/docs/doxygen/include/size_table.md @@ -9,7 +9,7 @@ core_http_client.c -
3.2K
+
3.1K
2.5K
@@ -29,7 +29,7 @@ Total estimates -
24.0K
+
23.9K
20.7K
From 5232eb202869360542b657b00f1ae297d1f6705e Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 12 Aug 2022 19:50:03 +0000 Subject: [PATCH 14/18] Adding Rule 7.4 suppression due to swapping to memcpy() instead of strncpy(), removed a rule that wasn't needed from the config file, cleaning up comments. --- MISRA.md | 9 +++++++++ source/core_http_client.c | 14 ++++++++++++-- tools/coverity/misra.config | 4 ---- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/MISRA.md b/MISRA.md index 49fd1208..7ab9c3c8 100644 --- a/MISRA.md +++ b/MISRA.md @@ -50,6 +50,15 @@ _Ref 5.4-6_ above it. This rule is suppressed for naming consistency with other HTTP header field and value string and length macros in this file. +#### Rule 7.4 +_Ref 7.4.1_ +- MISRA Rule 7.4 flags the strings being passed into memcpy() as not being of type + 'void const * restrict' the strings are defined as literals using macros + and as such will not be able to change. Attempting to cast the strings as defined + to a different type leads to different violations. For further information + on why memcpy() is being used here instead of strncpy() refer to + [this](https://github.com/FreeRTOS/coreHTTP/issues/133) compiler warning. + #### Rule 10.8 _Ref 10.8.1_ diff --git a/source/core_http_client.c b/source/core_http_client.c index e5f4f49f..6bee4124 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -939,6 +939,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, * and must be moved up in the buffer. When pLoc is greater than the current * end of the body, that signals the parser found a chunk header. */ + /* MISRA Ref 18.3.1 [Pointer comparison] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-183 */ /* coverity[pointer_parameter] */ /* coverity[misra_c_2012_rule_18_3_violation] */ @@ -1356,6 +1357,9 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, pBufferCur += fieldLen; /* Copy the field separator, ": ", into the buffer. */ + /* MISRA Ref 7.4.1 [String literals] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-74 */ + /* coverity[misra_c_2012_rule_7_4_violation] */ ( void ) memcpy( pBufferCur, HTTP_HEADER_FIELD_SEPARATOR, HTTP_HEADER_FIELD_SEPARATOR_LEN ); @@ -1374,6 +1378,9 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, pBufferCur += valueLen; /* Copy the header end indicator, "\r\n\r\n" into the buffer. */ + /* MISRA Ref 7.4.1 [String literals] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-74 */ + /* coverity[misra_c_2012_rule_7_4_violation] */ ( void ) memcpy( pBufferCur, HTTP_HEADER_END_INDICATOR, HTTP_HEADER_END_INDICATOR_LEN ); @@ -1523,6 +1530,9 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, HTTP_PROTOCOL_VERSION_LEN ); pBufferCur += HTTP_PROTOCOL_VERSION_LEN; + /* MISRA Ref 7.4.1 [String literals] */ + /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-74 */ + /* coverity[misra_c_2012_rule_7_4_violation] */ ( void ) memcpy( pBufferCur, HTTP_HEADER_LINE_SEPARATOR, HTTP_HEADER_LINE_SEPARATOR_LEN ); @@ -2034,7 +2044,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT * total, parsing will tell us if the end of the message is reached.*/ shouldParse = 1U; - /* Coverity compliance requires the cast to an unsigned type, since we have checked that + /* MISRA compliance requires the cast to an unsigned type, since we have checked that * the value of current received is greater than 0 we don't need to worry about int overflow. */ totalReceived += ( size_t ) currentReceived; } @@ -2061,7 +2071,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT /* Data is received into the buffer is immediately parsed. Parsing * is invoked even with a length of zero. A length of zero indicates * to the parser that there is no more data from the server (EOF). - * Additionally coverity compliance requires the cast to a larger type, but since we + * Additionally MISRA compliance requires the cast to a larger type, but since we * know that the value is greater than 0 we don't need to worry about int overflow. */ returnStatus = parseHttpResponse( &parsingContext, pResponse, diff --git a/tools/coverity/misra.config b/tools/coverity/misra.config index 46593218..0c7ed84c 100644 --- a/tools/coverity/misra.config +++ b/tools/coverity/misra.config @@ -50,10 +50,6 @@ 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 18.3", - reason: "It is expected that the current location http-parser passes into the body parsing callback points to the same user response buffer; if it does not, an assertion is triggered." - }, { deviation: "Rule 21.1", reason: "Allow use of all names." From 017b93b6926b9a06592ca9fd90fa3976dbf7c176 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 12 Aug 2022 12:53:07 -0700 Subject: [PATCH 15/18] Crustify fix --- source/core_http_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index 6bee4124..9620cbff 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -939,7 +939,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, * and must be moved up in the buffer. When pLoc is greater than the current * end of the body, that signals the parser found a chunk header. */ - /* MISRA Ref 18.3.1 [Pointer comparison] */ + /* MISRA Ref 18.3.1 [Pointer comparison] */ /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-183 */ /* coverity[pointer_parameter] */ /* coverity[misra_c_2012_rule_18_3_violation] */ From 6c147d9fa409c8ca73b575c72cc117c3affc3281 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 12 Aug 2022 20:08:29 +0000 Subject: [PATCH 16/18] Changing comment --- source/core_http_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index 9620cbff..aff907bc 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -929,7 +929,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser, /* The next location to write. */ - /* MISRA Ref 11.8.1 [Function pointer and use of const pointer] */ + /* 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 ); From 1f3fbbbaaa58167965b741e325dd158fd9021c00 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 12 Aug 2022 23:32:43 +0000 Subject: [PATCH 17/18] Using inbetween variables to remove MISRA violations --- MISRA.md | 9 --------- source/core_http_client.c | 23 ++++++++++------------- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/MISRA.md b/MISRA.md index 7ab9c3c8..49fd1208 100644 --- a/MISRA.md +++ b/MISRA.md @@ -50,15 +50,6 @@ _Ref 5.4-6_ above it. This rule is suppressed for naming consistency with other HTTP header field and value string and length macros in this file. -#### Rule 7.4 -_Ref 7.4.1_ -- MISRA Rule 7.4 flags the strings being passed into memcpy() as not being of type - 'void const * restrict' the strings are defined as literals using macros - and as such will not be able to change. Attempting to cast the strings as defined - to a different type leads to different violations. For further information - on why memcpy() is being used here instead of strncpy() refer to - [this](https://github.com/FreeRTOS/coreHTTP/issues/133) compiler warning. - #### Rule 10.8 _Ref 10.8.1_ diff --git a/source/core_http_client.c b/source/core_http_client.c index aff907bc..04ebcf0c 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -1314,6 +1314,10 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, size_t toAddLen = 0U; size_t backtrackHeaderLen = 0U; + /* These variables are here to pass into memcpy for MISRA compliance */ + const char * pHeaderEndIndicator = HTTP_HEADER_END_INDICATOR; + const char * httpFieldSeparator = HTTP_HEADER_FIELD_SEPARATOR; + assert( pRequestHeaders != NULL ); assert( pRequestHeaders->pBuffer != NULL ); assert( pField != NULL ); @@ -1357,11 +1361,8 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, pBufferCur += fieldLen; /* Copy the field separator, ": ", into the buffer. */ - /* MISRA Ref 7.4.1 [String literals] */ - /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-74 */ - /* coverity[misra_c_2012_rule_7_4_violation] */ ( void ) memcpy( pBufferCur, - HTTP_HEADER_FIELD_SEPARATOR, + httpFieldSeparator, HTTP_HEADER_FIELD_SEPARATOR_LEN ); pBufferCur += HTTP_HEADER_FIELD_SEPARATOR_LEN; @@ -1378,11 +1379,8 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, pBufferCur += valueLen; /* Copy the header end indicator, "\r\n\r\n" into the buffer. */ - /* MISRA Ref 7.4.1 [String literals] */ - /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-74 */ - /* coverity[misra_c_2012_rule_7_4_violation] */ ( void ) memcpy( pBufferCur, - HTTP_HEADER_END_INDICATOR, + pHeaderEndIndicator, HTTP_HEADER_END_INDICATOR_LEN ); /* Update the headers length value only when everything is successful. */ @@ -1480,6 +1478,9 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, char * pBufferCur = NULL; size_t toAddLen = 0U; + /* This variable is here to pass into memcpy for MISRA compliance */ + const char * pHeaderLineSeparator = HTTP_HEADER_LINE_SEPARATOR; + assert( pRequestHeaders != NULL ); assert( pRequestHeaders->pBuffer != NULL ); assert( pMethod != NULL ); @@ -1529,12 +1530,8 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, HTTP_PROTOCOL_VERSION, HTTP_PROTOCOL_VERSION_LEN ); pBufferCur += HTTP_PROTOCOL_VERSION_LEN; - - /* MISRA Ref 7.4.1 [String literals] */ - /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-74 */ - /* coverity[misra_c_2012_rule_7_4_violation] */ ( void ) memcpy( pBufferCur, - HTTP_HEADER_LINE_SEPARATOR, + pHeaderLineSeparator, HTTP_HEADER_LINE_SEPARATOR_LEN ); pRequestHeaders->headersLen = toAddLen; } From 5a352f992b7ac34d024efdb7b866dbb517fce1c3 Mon Sep 17 00:00:00 2001 From: Soren Ptak Date: Fri, 12 Aug 2022 17:11:31 -0700 Subject: [PATCH 18/18] Formatting fix --- source/core_http_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/core_http_client.c b/source/core_http_client.c index 04ebcf0c..3317454f 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -1315,7 +1315,7 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, size_t backtrackHeaderLen = 0U; /* These variables are here to pass into memcpy for MISRA compliance */ - const char * pHeaderEndIndicator = HTTP_HEADER_END_INDICATOR; + const char * pHeaderEndIndicator = HTTP_HEADER_END_INDICATOR; const char * httpFieldSeparator = HTTP_HEADER_FIELD_SEPARATOR; assert( pRequestHeaders != NULL );