Fix MISRA violations in the coreHTTP lib (#174)

AniruddhaKanhere · actions-user · web-flow · commit 1749807ef26d · 2024-02-22T21:54:27.000-08:00
* Fix MISRA violations

* Uncrustify: triggered by comment.

* Fix comment to match the one in coreMQTT

* Add clarifying paranthesis

* Revert file changes

* Fix some MISRA errors

* Fix UT

* Fix some MISRA violations

* Fix some more MISRA errors

* Fix few MISRA violations

* Fix 3 more violations

* Fix remaining MISRA violations

* Update c standard in test cmake

* Spelling fix

* Address PR comments

---------

Co-authored-by: GitHub Action &lt;action@github.com&gt;
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -19,6 +19,7 @@ jobs:
           cmake -S test -B build/ \
           -G "Unix Makefiles" \
           -DCMAKE_BUILD_TYPE=Debug \
+          -DUNITTEST=1 \
           -DCMAKE_C_FLAGS='--coverage -Wall -Wextra -DNDEBUG'
           make -C build/ all
 
diff --git a/MISRA.md b/MISRA.md
@@ -64,11 +64,18 @@ _Ref 10.8.1_
 _Ref 14.3.1_
 
 - MISRA Rule 14.3 The third-party http-parser library sets a uint64_t type field to
-       `ULLONG_MAX` or `( ( uint64_t ) -1 )`, during its internal parsing. Coverity MISRA does not detect
-       that this variable changes. This field is checked by the HTTP Client library.
-       If the Content-Length header was found, then pHttpParser->content_length
+       `ULLONG_MAX` or `( ( uint64_t ) -1 )`, during its internal parsing. Coverity MISRA
+       does not detect that this variable changes. This field is checked by the HTTP
+       Client library. If the Content-Length header was found, then pHttpParser->content_length
        will not be equal to the maximum 64 bit integer.
 
+#### Rule 11.4
+_Ref 11.4.1_
+
+- MISRA Rule 11.4 does not allow casting pointers to different data types as this may lead
+       to misaligned pointers. But in this case, the pointers are casted to different data
+       type to get the length of the header. It is not casted back to a pointer.
+
 #### Rule 11.8
 _Ref 11.8.1_
 
diff --git a/source/core_http_client.c b/source/core_http_client.c
@@ -43,19 +43,6 @@
  */
 static uint32_t getZeroTimestampMs( void );
 
-
-HTTPStatus_t HTTPClient_SendHttpData( const TransportInterface_t * pTransport,
-                                      HTTPClient_GetCurrentTimeFunc_t getTimestampMs,
-                                      const uint8_t * pData,
-                                      size_t dataLen );
-
-
-HTTPStatus_t HTTPClient_SendHttpHeaders( const TransportInterface_t * pTransport,
-                                         HTTPClient_GetCurrentTimeFunc_t getTimestampMs,
-                                         HTTPRequestHeaders_t * pRequestHeaders,
-                                         size_t reqBodyLen,
-                                         uint32_t sendFlags );
-
 /**
  * @brief Adds the Content-Length header field and value to the
  * @p pRequestHeaders.
@@ -162,11 +149,6 @@ static HTTPStatus_t getFinalResponseStatus( HTTPParsingState_t parsingState,
                                             size_t totalReceived,
                                             size_t responseBufferLen );
 
-
-HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t * pTransport,
-                                                     HTTPResponse_t * pResponse,
-                                                     const HTTPRequestHeaders_t * pRequestHeaders );
-
 /**
  * @brief Send the HTTP request over the network.
  *
@@ -668,7 +650,7 @@ static int httpParserOnStatusCallback( llhttp_t * pHttpParser,
     assert( pResponse != NULL );
 
     /* Set the location of what to parse next. */
-    pParsingContext->pBufferCur = pLoc + length;
+    pParsingContext->pBufferCur = &pLoc[ length ];
 
     /* Initialize the first header field and value to be passed to the user
      * callback. */
@@ -748,7 +730,7 @@ static int httpParserOnHeaderFieldCallback( llhttp_t * pHttpParser,
     }
 
     /* Set the location of what to parse next. */
-    pParsingContext->pBufferCur = pLoc + length;
+    pParsingContext->pBufferCur = &pLoc[ length ];
 
     /* The httpParserOnHeaderFieldCallback() always follows the
      * httpParserOnHeaderValueCallback() if there is another header field. When
@@ -794,7 +776,7 @@ static int httpParserOnHeaderValueCallback( llhttp_t * pHttpParser,
     pParsingContext = ( HTTPParsingContext_t * ) ( pHttpParser->data );
 
     /* Set the location of what to parse next. */
-    pParsingContext->pBufferCur = pLoc + length;
+    pParsingContext->pBufferCur = &pLoc[ length ];
 
     /* If httpParserOnHeaderValueCallback() is invoked in succession, then the
      * last time llhttp_execute() was called only part of the header field
@@ -916,7 +898,7 @@ static int httpParserOnHeadersCompleteCallback( llhttp_t * pHttpParser )
      * parsing here. */
     if( ( pResponse->respOptionFlags & HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG ) != 0U )
     {
-        shouldContinueParse = LLHTTP_PAUSE_PARSING;
+        shouldContinueParse = ( int ) LLHTTP_PAUSE_PARSING;
     }
 
     return shouldContinueParse;
@@ -963,7 +945,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser,
     /* MISRA Ref 11.8.1 [Removal of const from pointer] */
     /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-118 */
     /* coverity[misra_c_2012_rule_11_8_violation] */
-    pNextWriteLoc = ( char * ) ( pResponse->pBody + pResponse->bodyLen );
+    pNextWriteLoc = ( char * ) ( &( pResponse->pBody[ pResponse->bodyLen ] ) );
 
     /* If the response is of type Transfer-Encoding: chunked, then actual body
      * will follow the the chunked header. This body data is in a later location
@@ -985,7 +967,7 @@ static int httpParserOnBodyCallback( llhttp_t * pHttpParser,
     pResponse->bodyLen += length;
 
     /* Set the next location of parsing. */
-    pParsingContext->pBufferCur = pLoc + length;
+    pParsingContext->pBufferCur = &pLoc[ length ];
 
     LogDebug( ( "Response parsing: Found the response body: "
                 "BodyLength=%lu",
@@ -1243,7 +1225,7 @@ static HTTPStatus_t parseHttpResponse( HTTPParsingContext_t * pParsingContext,
     else
     {
         /* The next location to parse is after what has already been parsed. */
-        pParsingContext->pBufferCur = parsingStartLoc + parseLen;
+        pParsingContext->pBufferCur = &parsingStartLoc[ parseLen ];
     }
 
     returnStatus = processLlhttpError( &( pParsingContext->llhttpParser ) );
@@ -1378,18 +1360,18 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders,
     assert( fieldLen != 0U );
     assert( valueLen != 0U );
 
-    pBufferCur = ( char * ) ( pRequestHeaders->pBuffer + pRequestHeaders->headersLen );
+    pBufferCur = ( char * ) &( pRequestHeaders->pBuffer[ pRequestHeaders->headersLen ] );
     backtrackHeaderLen = pRequestHeaders->headersLen;
 
     /* Backtrack before trailing "\r\n" (HTTP header end) if it's already written.
      * Note that this method also writes trailing "\r\n" before returning.
      * The first condition prevents reading before start of the header. */
     if( ( HTTP_HEADER_END_INDICATOR_LEN <= pRequestHeaders->headersLen ) &&
-        ( strncmp( ( char * ) pBufferCur - HTTP_HEADER_END_INDICATOR_LEN,
+        ( strncmp( ( char * ) &pBufferCur[ 0 - ( ( int ) HTTP_HEADER_END_INDICATOR_LEN ) ],
                    HTTP_HEADER_END_INDICATOR, HTTP_HEADER_END_INDICATOR_LEN ) == 0 ) )
     {
         backtrackHeaderLen -= HTTP_HEADER_LINE_SEPARATOR_LEN;
-        pBufferCur -= HTTP_HEADER_LINE_SEPARATOR_LEN;
+        pBufferCur = &pBufferCur[ 0 - ( ( int ) HTTP_HEADER_LINE_SEPARATOR_LEN ) ];
     }
 
     /* Check if there is enough space in buffer for additional header. */
@@ -1411,14 +1393,14 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders,
 
         if( returnStatus == HTTPSuccess )
         {
-            pBufferCur += fieldLen;
+            pBufferCur = &pBufferCur[ fieldLen ];
 
             /* Copy the field separator, ": ", into the buffer. */
             ( void ) memcpy( pBufferCur,
                              httpFieldSeparator,
                              HTTP_HEADER_FIELD_SEPARATOR_LEN );
 
-            pBufferCur += HTTP_HEADER_FIELD_SEPARATOR_LEN;
+            pBufferCur = &pBufferCur[ HTTP_HEADER_FIELD_SEPARATOR_LEN ];
 
             /* Copy the header value into the buffer. */
             if( httpHeaderStrncpy( pBufferCur, pValue, valueLen, HTTP_HEADER_STRNCPY_IS_VALUE ) == NULL )
@@ -1429,7 +1411,7 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders,
 
         if( returnStatus == HTTPSuccess )
         {
-            pBufferCur += valueLen;
+            pBufferCur = &pBufferCur[ valueLen ];
 
             /* Copy the header end indicator, "\r\n\r\n" into the buffer. */
             ( void ) memcpy( pBufferCur,
@@ -1485,7 +1467,7 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders,
 
     /* Write the range start value in the buffer. */
     rangeValueLength += convertInt32ToAscii( rangeStartOrlastNbytes,
-                                             rangeValueBuffer + rangeValueLength,
+                                             &rangeValueBuffer[ rangeValueLength ],
                                              sizeof( rangeValueBuffer ) - rangeValueLength );
 
     /* Add remaining value data depending on the range specification type. */
@@ -1494,19 +1476,19 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders,
     if( rangeEnd != HTTP_RANGE_REQUEST_END_OF_FILE )
     {
         /* Write the "-" character to the buffer.*/
-        *( rangeValueBuffer + rangeValueLength ) = DASH_CHARACTER;
+        rangeValueBuffer[ rangeValueLength ] = DASH_CHARACTER;
         rangeValueLength += DASH_CHARACTER_LEN;
 
         /* Write the rangeEnd value of the request range to the buffer. */
         rangeValueLength += convertInt32ToAscii( rangeEnd,
-                                                 rangeValueBuffer + rangeValueLength,
+                                                 &rangeValueBuffer[ rangeValueLength ],
                                                  sizeof( rangeValueBuffer ) - rangeValueLength );
     }
     /* Case when request is for bytes in the range [rangeStart, EoF). */
     else if( rangeStartOrlastNbytes >= 0 )
     {
         /* Write the "-" character to the buffer.*/
-        *( rangeValueBuffer + rangeValueLength ) = DASH_CHARACTER;
+        rangeValueBuffer[ rangeValueLength ] = DASH_CHARACTER;
         rangeValueLength += DASH_CHARACTER_LEN;
     }
     else
@@ -1565,32 +1547,32 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders,
     {
         /* Write "<METHOD> <PATH> HTTP/1.1\r\n" to start the HTTP header. */
         ( void ) memcpy( pBufferCur, pMethod, methodLen );
-        pBufferCur += methodLen;
+        pBufferCur = &pBufferCur[ methodLen ];
 
         *pBufferCur = SPACE_CHARACTER;
-        pBufferCur += SPACE_CHARACTER_LEN;
+        pBufferCur = &pBufferCur[ SPACE_CHARACTER_LEN ];
 
         /* Use "/" as default value if <PATH> is NULL. */
         if( ( pPath == NULL ) || ( pathLen == 0U ) )
         {
             ( void ) memcpy( pBufferCur,
                              pHttpEmptyPath,
                              HTTP_EMPTY_PATH_LEN );
-            pBufferCur += HTTP_EMPTY_PATH_LEN;
+            pBufferCur = &pBufferCur[ HTTP_EMPTY_PATH_LEN ];
         }
         else
         {
             ( void ) memcpy( pBufferCur, pPath, pathLen );
-            pBufferCur += pathLen;
+            pBufferCur = &pBufferCur[ pathLen ];
         }
 
         *pBufferCur = SPACE_CHARACTER;
-        pBufferCur += SPACE_CHARACTER_LEN;
+        pBufferCur = &pBufferCur[ SPACE_CHARACTER_LEN ];
 
         ( void ) memcpy( pBufferCur,
                          pHttpProtocolVersion,
                          HTTP_PROTOCOL_VERSION_LEN );
-        pBufferCur += HTTP_PROTOCOL_VERSION_LEN;
+        pBufferCur = &pBufferCur[ HTTP_PROTOCOL_VERSION_LEN ];
         ( void ) memcpy( pBufferCur,
                          pHeaderLineSeparator,
                          HTTP_HEADER_LINE_SEPARATOR_LEN );
@@ -1880,7 +1862,7 @@ HTTPStatus_t HTTPClient_SendHttpData( const TransportInterface_t * pTransport,
             lastSendTimeMs = getTimestampMs();
 
             bytesRemaining -= ( size_t ) bytesSent;
-            pIndex += bytesSent;
+            pIndex = &pIndex[ bytesSent ];
             LogDebug( ( "Sent data over the transport: "
                         "BytesSent=%ld, BytesRemaining=%lu, TotalBytesSent=%lu",
                         ( long int ) bytesSent,
@@ -2081,7 +2063,7 @@ HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t
     {
         /* Receive the HTTP response data into the pResponse->pBuffer. */
         currentReceived = pTransport->recv( pTransport->pNetworkContext,
-                                            pResponse->pBuffer + totalReceived,
+                                            &( pResponse->pBuffer[ totalReceived ] ),
                                             pResponse->bufferLen - totalReceived );
 
         /* Transport receive errors are negative. */
@@ -2157,6 +2139,10 @@ HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t
         /* There may be dangling data if we parse with do not parse body flag.
          * We expose this data through body to let the applications access it. */
         pResponse->pBody = ( const uint8_t * ) parsingContext.pBufferCur;
+
+        /* MISRA Ref 11.4.1 [Casting pointer to int] */
+        /* More details at: https://github.com/FreeRTOS/coreHTTP/blob/main/MISRA.md#rule-114 */
+        /* coverity[misra_c_2012_rule_11_4_violation] */
         pResponse->bodyLen = totalReceived - ( size_t ) ( ( ( uintptr_t ) pResponse->pBody ) - ( ( uintptr_t ) pResponse->pBuffer ) );
     }
 
diff --git a/source/include/core_http_client_private.h b/source/include/core_http_client_private.h
@@ -207,7 +207,7 @@
  * @brief The state of the response message parsed after function
  * #parseHttpResponse returns.
  */
-typedef enum HTTPParsingState_t
+typedef enum HTTPParsingState
 {
     HTTP_PARSING_NONE = 0,   /**< The parser has not started reading any response. */
     HTTP_PARSING_INCOMPLETE, /**< The parser found a partial response. */
diff --git a/source/interface/transport_interface.h b/source/interface/transport_interface.h
@@ -195,14 +195,11 @@ typedef struct NetworkContext NetworkContext_t;
  * @transportcallback
  * @brief Transport interface for receiving data on the network.
  *
- * @note It is RECOMMENDED that the transport receive implementation
- * does NOT block when requested to read a single byte. A single byte
- * read request can be made by the caller to check whether there is a
- * new frame available on the network for reading.
- * However, the receive implementation MAY block for a timeout period when
- * it is requested to read more than 1 byte. This is because once the caller
- * is aware that a new frame is available to read on the network, then
- * the likelihood of reading more than one byte over the network becomes high.
+ * @note It is HIGHLY RECOMMENDED that the transport receive
+ * implementation does NOT block.
+ * coreMQTT will continue to call the transport interface if it receives
+ * a partial packet until it accumulates enough data to get the complete
+ * MQTT packet.
  *
  * @param[in] pNetworkContext Implementation-defined network context.
  * @param[in] pBuffer Buffer to receive the data into.
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
diff --git a/tools/coverity/misra.config b/tools/coverity/misra.config

Original file line number	Diff line number	Diff line change
`@@ -207,7 +207,7 @@`
`207`	`207`	`* @brief The state of the response message parsed after function`
`208`	`208`	`* #parseHttpResponse returns.`
`209`	`209`	`*/`
`210`		`-typedef enum HTTPParsingState_t`
	`210`	`+typedef enum HTTPParsingState`
`211`	`211`	`{`
`212`	`212`	`HTTP_PARSING_NONE = 0, /*< The parser has not started reading any response. /`
`213`	`213`	`HTTP_PARSING_INCOMPLETE, /*< The parser found a partial response. /`