fix(esp_http_client): fix parser wedge and dead error-classification branches on read timeout (IDFGH-17949)

[94xhn]

Summary

Two related bugs in components/esp_http_client/esp_http_client.c's handling of esp_transport_read()'s timeout return value (ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT == 0), found while reading the read/retry path.

1. Blocking-mode read timeout permanently wedges the HTTP parser

esp_http_client_get_data():

int rlen = esp_transport_read(client->transport, res_buffer->data, client->buffer_size_rx, client->timeout_ms);
if (rlen >= 0) {
    if (!(client->is_async && rlen == 0)) {
        http_parser_execute(client->parser, client->parser_settings, res_buffer->data, rlen);
    }
}

esp_transport_read() (tcp_read()/ssl_read() in components/tcp_transport/transport_ssl.c) returns 0 (ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT) whenever esp_transport_poll_read() times out with no data ready. This is not an async-only condition — it happens in blocking mode too, any time the peer pauses longer than client->timeout_ms mid-response (slow server, WiFi hiccup, etc). The guard above only skips the parser call when client->is_async && rlen == 0, so a blocking-mode client calls http_parser_execute(parser, settings, buf, /*len=*/0) whenever this happens.

http_parser_execute(..., len=0) is only valid to signal genuine end-of-stream (http_parser.c, states s_body_identity_eof/s_dead/s_start_*). In any other state — which is exactly the state a mid-response stall leaves the parser in (mid-header, mid-chunk, etc.) — it sets HPE_INVALID_EOF_STATE. Every subsequent call to http_parser_execute() starts with:

if (HTTP_PARSER_ERRNO(parser) != HPE_OK) {
    return 0;
}

so the parser is permanently wedged for the rest of that connection: further reads can keep succeeding (rlen > 0), but the bytes are silently dropped by the dead parser — data_process never advances. In esp_http_client_perform()'s body-reading loop (while (data_process < content_length) { ret = get_data(); if (ret <= 0) break; }), ret stays positive while data_process never reaches content_length, so the loop never terminates — a permanent hang of the calling task on a normal blocking client, triggered by a single transient read stall.

Fix: skip the parser call whenever rlen == 0, in any mode. rlen == 0 from esp_transport_read() is never a legitimate zero-byte success — a clean close is reported as -1 (ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN), not 0 — so there's no case where the len==0 call is actually wanted at this call site. (A different call site, esp_http_client_read() at line ~1467, already does the correct, narrowly-scoped thing: it explicitly calls http_parser_execute(..., 0) only when rlen == ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN, i.e. genuine EOF.)

2. Dead error-classification branches (wrong constant family)

esp_http_client_perform(), in both the chunked and content-length body-reading loops:

if (ret == ESP_ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT) {
    err = ESP_ERR_HTTP_READ_TIMEOUT;
} else if (ret == ESP_ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN) {
    err = ESP_ERR_HTTP_CONNECTION_CLOSED;
} else {
    err = ESP_ERR_HTTP_INCOMPLETE_DATA;
}

ret here is the raw, untranslated value returned by esp_http_client_get_data() — i.e. esp_transport_read()'s own ERR_TCP_TRANSPORT_* enum (include/esp_transport.h: 0/-1/-2/-3). It's compared against ESP_ERR_TCP_TRANSPORT_* — the translated esp_err_t codes (0xe001/0xe002), only ever produced by esp_transport_translate_error(). Since ret can never equal 0xe001/0xe002, both branches are dead code, and every read timeout or clean close during body reading gets reported as the generic ESP_ERR_HTTP_INCOMPLETE_DATA — contradicting the documented return values in esp_http_client.h ("ESP_ERR_HTTP_READ_TIMEOUT if read operation times out ... ESP_ERR_HTTP_CONNECTION_CLOSED if server closes the connection").

This same file already gets this right elsewhere, e.g. esp_http_client_read():

if (rlen == ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT) { ... }          // line ~1475
if (rlen == ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN && ...) { ... } // line ~1465
if (buffer->len == ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT) { ... }    // line ~1673

Fix: compare against the correct, untranslated ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT/ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN constants (already in scope via the existing esp_transport.h include), matching the convention used everywhere else in this file.

Testing

I verified issue #1 (the parser wedge, the more serious of the two) with a standalone reproduction built against the unmodified components/http_parser/http_parser.c/.h (pure C, no other esp-idf dependencies):

• Feed a chunked-response header plus a chunk header (14\r\n, i.e. 20 bytes) plus only the first 5 bytes of that chunk's data — exactly the parser state left behind by a partial TCP read.
• Before fix (simulating what esp_http_client_get_data() does today on a blocking-mode timeout): call http_parser_execute(parser, settings, buf, 0). Result: HTTP_PARSER_ERRNO(parser) == HPE_INVALID_EOF_STATE. Feeding the remaining 15 chunk bytes + the terminating 0\r\n\r\n afterward parses 0 bytes; on_body fires only once (5 bytes captured instead of 20), on_message_complete never fires.
• After fix (the len==0 call is simply never issued mid-chunk): the same remaining bytes parse fully; on_body fires twice for a total of 20 bytes, on_message_complete fires once. Full output of both runs available on request.

Issue #2 is a straightforward constant-family mismatch, confirmed by reading esp_transport.h (ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT == 0 vs ESP_ERR_TCP_TRANSPORT_CONNECTION_TIMEOUT == 0xe001) and cross-checking against the three other call sites in the same file that already compare ret/rlen against the correct, untranslated constants.

I don't have a full on-target/QEMU or mocked-transport test environment set up for esp_http_client in my current setup, so I can't attach a test_apps-level run exercising the real transport + timeout path; the http_parser-only harness above isolates and confirms the actual state-machine defect this fix addresses. Happy to add a test_apps case if there's an existing pattern for mocking a stalling transport that I should follow.

Generative AI

I used generative AI tools (Claude) when creating this PR, but a human has checked the code and is responsible for the code and the description above.

---

Note

Medium Risk
Touches core HTTP read/parse and error paths used by blocking esp_http_client_perform(); fixes serious hang/misreporting bugs but changes observable error codes on timeout/close during body reads.

Overview
Fixes two bugs in how esp_http_client handles esp_transport_read() returning 0 (read poll timeout).

In esp_http_client_get_data(), http_parser_execute(..., 0) is no longer called when rlen == 0. The old guard only skipped that in async mode, so blocking clients could wedge the HTTP parser (HPE_INVALID_EOF_STATE) on a mid-response stall and hang esp_http_client_perform().

In esp_http_client_perform() body-read loops (chunked and content-length), incomplete-read handling now compares ret against raw ERR_TCP_TRANSPORT_* values instead of translated ESP_ERR_TCP_TRANSPORT_* codes, so timeouts and clean FIN closes surface as ESP_ERR_HTTP_READ_TIMEOUT and ESP_ERR_HTTP_CONNECTION_CLOSED instead of always ESP_ERR_HTTP_INCOMPLETE_DATA.

^{Reviewed by Cursor Bugbot for commit 412114a. Bugbot is set up for automated code reviews on this repo. Configure here.}

[CLAassistant]

All committers have signed the CLA.

[Ashish285]

Hey @94xhn, thanks for raising this PR. The changes look good, but can you please help to update the commit message

The current commit message says

later reads keep
succeeding but the data is silently dropped, so `data_process` never
reaches `content_length` and esp_http_client_perform()'s read loop spins
forever on a blocking client that hits one transient stall mid-response.

This is incorrect as the client doesn't block forever here. The consequence here is silent truncation is treated as success.

[94xhn]

Updated in 412114a. The commit message now describes the actual outcome: later bytes are discarded and silent truncation can be treated as success; it no longer claims the blocking client loops forever. I also removed the duplicate explanatory comment.

[Ashish285]

sha=412114a9939d3cf91f8bb8501ad5f09a2fef4240