Fix: replace reachable assert with error return in header field callbacks and bump llhttp to v9.3.1 (#204)

Author: felicityzhao9

docs/doxygen/include/size_table.md

@@ -14,22 +14,22 @@
     </tr>
     <tr>
         <td>api.c (llhttp)</td>
-        <td><center>2.8K</center></td>
-        <td><center>2.2K</center></td>
+        <td><center>7.5K</center></td>
+        <td><center>6.8K</center></td>
     </tr>
     <tr>
         <td>http.c (llhttp)</td>
-        <td><center>0.3K</center></td>
+        <td><center>0.4K</center></td>
         <td><center>0.3K</center></td>
     </tr>
     <tr>
         <td>llhttp.c (llhttp)</td>
-        <td><center>19.1K</center></td>
-        <td><center>17.1K</center></td>
+        <td><center>25.8K</center></td>
+        <td><center>23.0K</center></td>
     </tr>
     <tr>
         <td><b>Total estimates</b></td>
-        <td><b><center>25.5K</center></b></td>
-        <td><b><center>22.3K</center></b></td>
+        <td><b><center>37.0K</center></b></td>
+        <td><b><center>32.8K</center></b></td>
     </tr>
 </table>

manifest.yml

@@ -6,7 +6,7 @@ license: "MIT"
 
 dependencies:
   - name: "llhttp"
-    version: "release/v6.1.1"
+    version: "release/v9.3.1"
     license: "MIT"
     repository:
       type: "git"

source/core_http_client.c

@@ -717,6 +717,12 @@ static int httpParserOnHeaderFieldCallback( llhttp_t * pHttpParser,
     assert( pHttpParser->data != NULL );
     assert( pLoc != NULL );
 
+    if( length == 0U )
+    {
+        LogError( ( "Received zero-length header field name from parser." ) );
+        return LLHTTP_STOP_PARSING;
+    }
+
     pParsingContext = ( HTTPParsingContext_t * ) ( pHttpParser->data );
     pResponse = pParsingContext->pResponse;
 
@@ -2323,7 +2329,12 @@ static int findHeaderFieldParserCallback( llhttp_t * pHttpParser,
     assert( pHttpParser != NULL );
     assert( pHttpParser->data != NULL );
     assert( pFieldLoc != NULL );
-    assert( fieldLen > 0U );
+
+    if( fieldLen == 0U )
+    {
+        LogError( ( "Received zero-length header field name from parser." ) );
+        return LLHTTP_STOP_PARSING;
+    }
 
     pContext = ( findHeaderContext_t * ) pHttpParser->data;
 

source/dependency/3rdparty/llhttp

@@ -445,6 +445,7 @@ static llhttp_errno_t llhttp_execute_error( llhttp_t * pParser,
     return httpParsingErrno;
 }
 
+
 /* Mock helper that parses the status line starting from pNext. */
 static void helper_parse_status_line( const char ** pNext,
                                       llhttp_t * pParser,
@@ -575,6 +576,33 @@ static void helper_parse_body( const char ** pNext,
     }
 }
 
+/* Mocked llhttp_execute callback that parses the status line, then invokes
+ * on_header_field with a zero-length field name to exercise the early return
+ * guard in httpParserOnHeaderFieldCallback. */
+static llhttp_errno_t llhttp_execute_zero_length_header_field( llhttp_t * pParser,
+                                                               const char * pData,
+                                                               size_t len,
+                                                               int cmock_num_calls )
+{
+    ( void ) cmock_num_calls;
+    ( void ) len;
+    const char * pNext = pData;
+    llhttp_settings_t * pSettings = ( llhttp_settings_t * ) pParser->settings;
+    int cbReturnValue = 0;
+
+    pSettings->on_message_begin( pParser );
+    helper_parse_status_line( &pNext, pParser, pSettings );
+
+    /* Invoke on_header_field with length zero. The real callback should return
+     * LLHTTP_STOP_PARSING. */
+    cbReturnValue = pSettings->on_header_field( pParser, pNext, 0 );
+    TEST_ASSERT_EQUAL( LLHTTP_STOP_PARSING, cbReturnValue );
+
+    pParser->error = HPE_USER;
+    httpParserExecuteCallCount++;
+    return HPE_USER;
+}
+
 /* Mocked llhttp_execute callback that expects a whole response to be in
  * the given data to parse. */
 static llhttp_errno_t llhttp_execute_whole_response( llhttp_t * pParser,
@@ -1960,3 +1988,24 @@ void test_HTTPClient_Send_parsing_errors( void )
                                     0 );
     TEST_ASSERT_EQUAL( HTTPParserInternalError, returnStatus );
 }
+
+/* Test that httpParserOnHeaderFieldCallback returns LLHTTP_STOP_PARSING when
+ * the parser invokes it with a zero-length header field name (lines 720-724
+ * of core_http_client.c). */
+void test_HTTPClient_Send_zero_length_header_field( void )
+{
+    HTTPStatus_t returnStatus = HTTPSuccess;
+
+    llhttp_execute_Stub( llhttp_execute_zero_length_header_field );
+
+    returnStatus = HTTPClient_Send( &transportInterface,
+                                    &requestHeaders,
+                                    NULL,
+                                    0,
+                                    &response,
+                                    0 );
+
+    /* HPE_USER falls through to the default case in processLlhttpError,
+     * which maps to HTTPParserInternalError. */
+    TEST_ASSERT_EQUAL( HTTPParserInternalError, returnStatus );
+}

test/unit-test/core_http_send_utest.c

@@ -306,6 +306,36 @@ llhttp_errno_t parserExecuteExpectationsCb( llhttp_t * parser,
     return parserErrNo;
 }
 
+/**
+ * @brief Callback variant for llhttp_execute() mock that expects the
+ * on_header_field callback to return LLHTTP_STOP_PARSING when invoked
+ * with a zero-length header field name.
+ */
+llhttp_errno_t parserExecuteZeroLenFieldCb( llhttp_t * parser,
+                                            const char * data,
+                                            size_t len,
+                                            int cmock_num_calls )
+{
+    ( void ) cmock_num_calls;
+
+    TEST_ASSERT_EQUAL( pCapturedParser, parser );
+    TEST_ASSERT_NOT_NULL( parser );
+    TEST_ASSERT_EQUAL( pCapturedSettings, parser->settings );
+    TEST_ASSERT_EQUAL( expectedBufferSize, len );
+    TEST_ASSERT_EQUAL( pExpectedBuffer, data );
+
+    if( invokeHeaderFieldCallback == 1U )
+    {
+        TEST_ASSERT_EQUAL( LLHTTP_STOP_PARSING,
+                           ( ( llhttp_settings_t * ) ( parser->settings ) )->on_header_field( parser,
+                                                                                              pFieldLocToReturn,
+                                                                                              fieldLenToReturn ) );
+    }
+
+    parser->error = parserErrNo;
+    return parserErrNo;
+}
+
 /**
  * @brief Fills the test input buffer and expectation buffers with pre-existing data
  * before calling the API function under test.
@@ -1458,6 +1488,42 @@ void test_Http_ReadHeader_Invalid_Response_Only_Header_Field_Found()
     TEST_ASSERT_EQUAL( HTTPInvalidResponse, retCode );
 }
 
+/**
+ * @brief Test that a zero-length header field name from the parser causes
+ * findHeaderFieldParserCallback to return LLHTTP_STOP_PARSING, resulting
+ * in HTTPInvalidResponse from HTTPClient_ReadHeader.
+ */
+void test_Http_ReadHeader_Zero_Length_Header_Field( void )
+{
+    /* Add expectations for llhttp init dependencies. */
+    llhttp_settings_init_ExpectAnyArgs();
+    llhttp_init_ExpectAnyArgs();
+
+    /* Override the default llhttp_execute mock callback to use the variant
+     * that expects LLHTTP_STOP_PARSING from on_header_field. */
+    llhttp_execute_AddCallback( parserExecuteZeroLenFieldCb );
+
+    /* Configure the llhttp_execute mock to invoke on_header_field with
+     * a zero-length field name. */
+    invokeHeaderFieldCallback = 1U;
+    pFieldLocToReturn = &pTestResponse[ headerFieldInRespLoc ];
+    fieldLenToReturn = 0U;
+
+    /* The callback returns LLHTTP_STOP_PARSING (HPE_USER) for the zero-length
+     * field. Since no header was found (fieldFound == 0) and parserErrno is
+     * not HPE_OK, findHeaderInResponse returns HTTPInvalidResponse. */
+    parserErrNo = HPE_USER;
+    llhttp_execute_ExpectAnyArgsAndReturn( HPE_USER );
+
+    /* Call the function under test. */
+    retCode = HTTPClient_ReadHeader( &testResponse,
+                                     HEADER_IN_BUFFER,
+                                     HEADER_IN_BUFFER_LEN,
+                                     &pValueLoc,
+                                     &valueLen );
+    TEST_ASSERT_EQUAL( HTTPInvalidResponse, retCode );
+}
+
 
 /**
  * @brief Test happy path with zero-initialized requestHeaders and requestInfo.