Skip to content

Commit 86b4455

Browse files
authored
Return HTTPSuccess when HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG is set (#194)
* Return HTTPSuccess when HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG is set When the HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG is set, the HTTP_PARSING_INCOMPLETE state is expected and valid, as the body data may not yet be received from the transport layer. Previously, this scenario incorrectly returned HTTPInsufficientMemory. This commit corrects the behavior to return HTTPSuccess instead, properly handling the case where body parsing is intentionally deferred. Fixes: #193 Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com> * Fix unit tests and coverage Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com> * Fix formatting check Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com> * Fix SSOT check Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com> * Fix coverage check Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com> --------- Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
1 parent 96e32a1 commit 86b4455

File tree

5 files changed

+107
-29
lines changed

5 files changed

+107
-29
lines changed

.github/workflows/ci.yml

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,8 @@ jobs:
3434
- name: Run Coverage
3535
run: |
3636
make -C build/ coverage
37-
declare -a EXCLUDE=("\*test\*" "\*CMakeCCompilerId\*" "\*mocks\*" "\*3rdparty\*")
38-
echo ${EXCLUDE[@]} | xargs lcov --rc lcov_branch_coverage=1 -r build/coverage.info -o build/coverage.info
39-
lcov --rc lcov_branch_coverage=1 --list build/coverage.info
37+
lcov --rc branch_coverage=1 -r build/coverage.info -o build/coverage.info
38+
lcov --rc branch_coverage=1 --list build/coverage.info
4039
- name: Check Coverage
4140
uses: FreeRTOS/CI-CD-Github-Actions/coverage-cop@main
4241
with:
@@ -73,7 +72,7 @@ jobs:
7372
path: ./
7473

7574
formatting:
76-
runs-on: ubuntu-20.04
75+
runs-on: ubuntu-latest
7776
steps:
7877
- uses: actions/checkout@v3
7978
- name: Check formatting

source/core_http_client.c

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders,
137137
* @param[in] parsingState State of the parsing on the HTTP response.
138138
* @param[in] totalReceived The amount of network data received in the response
139139
* buffer.
140-
* @param[in] responseBufferLen The length of the response buffer.
140+
* @param[in] pResponse The response information.
141141
*
142142
* @return Returns #HTTPSuccess if the parsing state is complete. If
143143
* the parsing state denotes it never started, then return #HTTPNoResponse. If
@@ -147,7 +147,7 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders,
147147
*/
148148
static HTTPStatus_t getFinalResponseStatus( HTTPParsingState_t parsingState,
149149
size_t totalReceived,
150-
size_t responseBufferLen );
150+
const HTTPResponse_t * pResponse );
151151

152152
/**
153153
* @brief Send the HTTP request over the network.
@@ -1984,13 +1984,13 @@ static HTTPStatus_t sendHttpBody( const TransportInterface_t * pTransport,
19841984

19851985
static HTTPStatus_t getFinalResponseStatus( HTTPParsingState_t parsingState,
19861986
size_t totalReceived,
1987-
size_t responseBufferLen )
1987+
const HTTPResponse_t * pResponse )
19881988
{
19891989
HTTPStatus_t returnStatus = HTTPSuccess;
19901990

19911991
assert( parsingState >= HTTP_PARSING_NONE &&
19921992
parsingState <= HTTP_PARSING_COMPLETE );
1993-
assert( totalReceived <= responseBufferLen );
1993+
assert( totalReceived <= pResponse->bufferLen );
19941994

19951995
/* If no parsing occurred, that means network data was never received. */
19961996
if( parsingState == HTTP_PARSING_NONE )
@@ -2002,21 +2002,26 @@ static HTTPStatus_t getFinalResponseStatus( HTTPParsingState_t parsingState,
20022002
}
20032003
else if( parsingState == HTTP_PARSING_INCOMPLETE )
20042004
{
2005-
if( totalReceived == responseBufferLen )
2005+
/* HTTP_PARSING_INCOMPLETE is okay when HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG is set
2006+
* as the body data may yet to be read from the transport. */
2007+
if( ( pResponse->respOptionFlags & HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG ) == 0 )
20062008
{
2007-
LogError( ( "Cannot receive complete response from transport"
2008-
" interface: Response buffer has insufficient "
2009-
"space: responseBufferLen=%lu",
2010-
( unsigned long ) responseBufferLen ) );
2011-
returnStatus = HTTPInsufficientMemory;
2012-
}
2013-
else
2014-
{
2015-
LogError( ( "Received partial response from transport "
2016-
"receive(): ResponseSize=%lu, TotalBufferSize=%lu",
2017-
( unsigned long ) totalReceived,
2018-
( unsigned long ) ( responseBufferLen - totalReceived ) ) );
2019-
returnStatus = HTTPPartialResponse;
2009+
if( totalReceived == pResponse->bufferLen )
2010+
{
2011+
LogError( ( "Cannot receive complete response from transport"
2012+
" interface: Response buffer has insufficient "
2013+
"space: responseBufferLen=%lu",
2014+
( unsigned long ) pResponse->bufferLen ) );
2015+
returnStatus = HTTPInsufficientMemory;
2016+
}
2017+
else
2018+
{
2019+
LogError( ( "Received partial response from transport "
2020+
"receive(): ResponseSize=%lu, TotalBufferSize=%lu",
2021+
( unsigned long ) totalReceived,
2022+
( unsigned long ) ( pResponse->bufferLen - totalReceived ) ) );
2023+
returnStatus = HTTPPartialResponse;
2024+
}
20202025
}
20212026
}
20222027
else
@@ -2155,7 +2160,7 @@ HTTPStatus_t HTTPClient_ReceiveAndParseHttpResponse( const TransportInterface_t
21552160
* the parsing and how much data is in the buffer. */
21562161
returnStatus = getFinalResponseStatus( parsingContext.state,
21572162
totalReceived,
2158-
pResponse->bufferLen );
2163+
pResponse );
21592164
}
21602165

21612166
return returnStatus;

source/interface/transport_interface.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@
9696
* without TLS, it is typically implemented by calling the TCP layer receive
9797
* function. @ref TransportRecv_t may be invoked multiple times by the protocol
9898
* library, if fewer bytes than were requested to receive are returned.
99+
* Please note that it is HIGHLY RECOMMENDED that the transport receive implementation does NOT block.
99100
* <br><br>
100101
* <b>Example code:</b>
101102
* @code{c}
@@ -200,6 +201,9 @@ typedef struct NetworkContext NetworkContext_t;
200201
* coreMQTT will continue to call the transport interface if it receives
201202
* a partial packet until it accumulates enough data to get the complete
202203
* MQTT packet.
204+
* A non‐blocking implementation is also essential so that the library's inbuilt
205+
* keep‐alive mechanism can work properly, given the user chooses to use
206+
* that over their own keep alive mechanism.
203207
*
204208
* @param[in] pNetworkContext Implementation-defined network context.
205209
* @param[in] pBuffer Buffer to receive the data into.

test/unit-test/core_http_send_utest.c

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -760,6 +760,32 @@ static llhttp_errno_t llhttp_execute_partial_body( llhttp_t * pParser,
760760
return HPE_OK;
761761
}
762762

763+
764+
/* Mocked llhttp_execute callback that will be called when partial body has been
765+
* received from the network. It returns HPE_PAUSED to mimic stopping parsing
766+
* the body because user set HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG. */
767+
static llhttp_errno_t llhttp_execute_partial_body_do_not_parse( llhttp_t * pParser,
768+
const char * pData,
769+
size_t len,
770+
int cmock_num_calls )
771+
{
772+
( void ) cmock_num_calls;
773+
( void ) len;
774+
775+
const char * pNext = pData;
776+
llhttp_settings_t * pSettings = ( llhttp_settings_t * ) pParser->settings;
777+
778+
pSettings->on_message_begin( pParser );
779+
780+
helper_parse_status_line( &pNext, pParser, pSettings );
781+
helper_parse_headers( &pNext, pParser, pSettings );
782+
helper_parse_headers_finish( &pNext, pParser, pSettings, NULL );
783+
784+
pParser->error_pos = pNext;
785+
pParser->error = HPE_PAUSED;
786+
return HPE_PAUSED;
787+
}
788+
763789
/* Mocked llhttp_execute callback that will be on a response of type
764790
* transfer-encoding chunked. */
765791
static llhttp_errno_t llhttp_execute_chunked_body( llhttp_t * pParser,
@@ -768,6 +794,7 @@ static llhttp_errno_t llhttp_execute_chunked_body( llhttp_t * pParser,
768794
int cmock_num_calls )
769795
{
770796
( void ) cmock_num_calls;
797+
( void ) len;
771798

772799
const char * pNext = pData;
773800
uint8_t isHeadResponse = 0;
@@ -1158,6 +1185,47 @@ void test_HTTPClient_Send_parse_partial_body( void )
11581185

11591186
/*-----------------------------------------------------------*/
11601187

1188+
/* Test successfully parsing a response where up to the middle of the body
1189+
* is received on the first network read, then the rest of the response is not
1190+
* received from the network because HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG is set.
1191+
*/
1192+
void test_HTTPClient_Send_do_not_parse_partial_body( void )
1193+
{
1194+
HTTPStatus_t returnStatus = HTTPSuccess;
1195+
1196+
llhttp_execute_Stub( llhttp_execute_partial_body_do_not_parse );
1197+
1198+
memcpy( requestHeaders.pBuffer,
1199+
HTTP_TEST_REQUEST_GET_HEADERS,
1200+
HTTP_TEST_REQUEST_GET_HEADERS_LENGTH );
1201+
requestHeaders.headersLen = HTTP_TEST_REQUEST_GET_HEADERS_LENGTH;
1202+
pNetworkData = ( uint8_t * ) HTTP_TEST_RESPONSE_GET;
1203+
networkDataLen = HTTP_TEST_RESPONSE_GET_LENGTH;
1204+
firstPartBytes = HTTP_TEST_RESPONSE_GET_PARTIAL_BODY_LENGTH;
1205+
1206+
response.respOptionFlags |= HTTP_RESPONSE_DO_NOT_PARSE_BODY_FLAG;
1207+
1208+
returnStatus = HTTPClient_Send( &transportInterface,
1209+
&requestHeaders,
1210+
NULL,
1211+
0,
1212+
&response,
1213+
0 );
1214+
TEST_ASSERT_EQUAL( HTTPSuccess, returnStatus );
1215+
TEST_ASSERT_EQUAL( response.pBuffer + ( sizeof( HTTP_STATUS_LINE_OK ) - 1 ), response.pHeaders );
1216+
TEST_ASSERT_EQUAL( HTTP_TEST_RESPONSE_GET_HEADERS_LENGTH - HTTP_HEADER_END_INDICATOR_LEN,
1217+
response.headersLen );
1218+
TEST_ASSERT_EQUAL( response.pHeaders + HTTP_TEST_RESPONSE_GET_HEADERS_LENGTH, response.pBody );
1219+
TEST_ASSERT_EQUAL( HTTP_TEST_RESPONSE_GET_PARTIAL_BODY_LENGTH - HTTP_TEST_RESPONSE_HEAD_LENGTH, response.bodyLen );
1220+
TEST_ASSERT_EQUAL( HTTP_STATUS_CODE_OK, response.statusCode );
1221+
TEST_ASSERT_EQUAL( HTTP_TEST_RESPONSE_GET_CONTENT_LENGTH, response.contentLength );
1222+
TEST_ASSERT_EQUAL( HTTP_TEST_RESPONSE_GET_HEADER_COUNT, response.headerCount );
1223+
TEST_ASSERT_BITS_HIGH( HTTP_RESPONSE_CONNECTION_CLOSE_FLAG, response.respFlags );
1224+
TEST_ASSERT_BITS_LOW( HTTP_RESPONSE_CONNECTION_KEEP_ALIVE_FLAG, response.respFlags );
1225+
}
1226+
1227+
/*-----------------------------------------------------------*/
1228+
11611229
/* Test receiving a response where the body is of Transfer-Encoding chunked. */
11621230
void test_HTTPClient_Send_parse_chunked_body( void )
11631231
{

tools/cmock/coverage.cmake

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,9 @@ execute_process( COMMAND lcov --directory ${CMAKE_BINARY_DIR}
1414
--base-directory ${CMAKE_BINARY_DIR}
1515
--initial
1616
--capture
17-
--rc lcov_branch_coverage=1
18-
--rc genhtml_branch_coverage=1
17+
--rc branch_coverage=1
1918
--output-file=${CMAKE_BINARY_DIR}/base_coverage.info
19+
--include "*source*"
2020
)
2121
file(GLOB files "${CMAKE_BINARY_DIR}/bin/tests/*")
2222

@@ -45,11 +45,11 @@ execute_process(COMMAND ruby
4545
# capture data after running the tests
4646
execute_process(
4747
COMMAND lcov --capture
48-
--rc lcov_branch_coverage=1
49-
--rc genhtml_branch_coverage=1
48+
--rc branch_coverage=1
5049
--base-directory ${CMAKE_BINARY_DIR}
5150
--directory ${CMAKE_BINARY_DIR}
5251
--output-file ${CMAKE_BINARY_DIR}/second_coverage.info
52+
--include "*source*"
5353
)
5454

5555
# combile baseline results (zeros) with the one after running the tests
@@ -60,10 +60,12 @@ execute_process(
6060
--add-tracefile ${CMAKE_BINARY_DIR}/second_coverage.info
6161
--output-file ${CMAKE_BINARY_DIR}/coverage.info
6262
--no-external
63-
--rc lcov_branch_coverage=1
63+
--rc branch_coverage=1
64+
--include "*source*"
65+
--exclude "*dependency*"
6466
)
6567
execute_process(
66-
COMMAND genhtml --rc lcov_branch_coverage=1
68+
COMMAND genhtml --rc branch_coverage=1
6769
--branch-coverage
6870
--output-directory ${CMAKE_BINARY_DIR}/coverage
6971
${CMAKE_BINARY_DIR}/coverage.info

0 commit comments

Comments
 (0)