diff --git a/MISRA.md b/MISRA.md index 60b36021..49fd1208 100644 --- a/MISRA.md +++ b/MISRA.md @@ -2,33 +2,94 @@ 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 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. | - -### 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. | +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.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 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. | +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. + +#### 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/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
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 17a23df0..459f3789 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -24,7 +24,6 @@ * @file core_http_client.c * @brief Implements the user-facing functions in core_http_client.h. */ - #include #include #include @@ -556,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 ); /*-----------------------------------------------------------*/ @@ -569,15 +568,21 @@ 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++ ) { - if( toupper( str1[ i ] ) != toupper( str2[ i ] ) ) + firstChar = toupper( ( int32_t ) ( str1[ i ] ) ); + secondChar = toupper( ( ( int32_t ) str2[ i ] ) ); + + if( ( firstChar ) != ( secondChar ) ) { break; } @@ -586,6 +591,7 @@ static int8_t caseInsensitiveStringCmp( const char * str1, return ( i == n ) ? 0 : 1; } + /*-----------------------------------------------------------*/ static void processCompleteHeader( HTTPParsingContext_t * pParsingContext ) @@ -829,10 +835,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 ) ); } @@ -841,8 +845,9 @@ 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 ) { pResponse->contentLength = ( size_t ) ( pHttpParser->content_length ); @@ -924,12 +929,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 [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 ); @@ -938,9 +939,8 @@ 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. */ + /* 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] */ if( pLoc > pNextWriteLoc ) @@ -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 ); @@ -1358,7 +1362,7 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, /* Copy the field separator, ": ", into the buffer. */ ( void ) memcpy( pBufferCur, - HTTP_HEADER_FIELD_SEPARATOR, + httpFieldSeparator, HTTP_HEADER_FIELD_SEPARATOR_LEN ); pBufferCur += HTTP_HEADER_FIELD_SEPARATOR_LEN; @@ -1376,7 +1380,7 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, /* Copy the header end indicator, "\r\n\r\n" into the buffer. */ ( void ) memcpy( pBufferCur, - HTTP_HEADER_END_INDICATOR, + pHeaderEndIndicator, HTTP_HEADER_END_INDICATOR_LEN ); /* Update the headers length value only when everything is successful. */ @@ -1474,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 ); @@ -1523,9 +1530,8 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, HTTP_PROTOCOL_VERSION, HTTP_PROTOCOL_VERSION_LEN ); pBufferCur += HTTP_PROTOCOL_VERSION_LEN; - ( void ) memcpy( pBufferCur, - HTTP_HEADER_LINE_SEPARATOR, + pHeaderLineSeparator, HTTP_HEADER_LINE_SEPARATOR_LEN ); pRequestHeaders->headersLen = toAddLen; } @@ -2034,7 +2040,10 @@ 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; + + /* 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; } else { @@ -2058,10 +2067,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 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, - currentReceived ); + ( uint64_t ) currentReceived ); } /* Reading should continue if there are no errors in the transport receive @@ -2324,7 +2335,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 +2349,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/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 d92a942e..6d27d46f 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -45,6 +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} ) +# Build HTTP library target without logging +target_compile_options(coverity_analysis PUBLIC -DNDEBUG ) # ===================== 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..0c7ed84c --- /dev/null +++ b/tools/coverity/misra.config @@ -0,0 +1,62 @@ +// 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. 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.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. 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.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 21.1", + reason: "Allow use of all names." + }, + { + deviation: "Rule 21.2", + reason: "Allow use of all names." + } + ] +}