MISRA Compliance Update (#132)

* Changes to MISRA.md to reflect new format. Added two variables to caseInsensitiveStringCmp function for MISRA compliance. Changed in line comments to reflect new MISRA.md format. Modified misra.config reasons. * 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. * Using DNDEBUG when building coverity_analysis target for removal of assert. Co-authored-by: Soren Ptak <[email protected]>
FreeRTOS · Aug 17, 2022 · a75d06a · a75d06a
1 parent 6ab1ff1
commit a75d06a
Show file tree

Hide file tree

Showing 7 changed files with 210 additions and 78 deletions.
diff --git 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
@@ -9,7 +9,7 @@
     </tr>
     <tr>
         <td>core_http_client.c</td>
-        <td><center>3.2K</center></td>
+        <td><center>3.1K</center></td>
         <td><center>2.5K</center></td>
     </tr>
     <tr>
@@ -29,7 +29,7 @@
     </tr>
     <tr>
         <td><b>Total estimates</b></td>
-        <td><b><center>24.0K</center></b></td>
+        <td><b><center>23.9K</center></b></td>
         <td><b><center>20.7K</center></b></td>
     </tr>
 </table>
diff --git 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
@@ -24,7 +24,6 @@
  * @file core_http_client.c
  * @brief Implements the user-facing functions in core_http_client.h.
  */
-
 #include <assert.h>
 #include <string.h>
 #include <ctype.h>
@@ -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;