fix(http): crash in HttpTunnel::producer_run when 100 Continue precedes a transformed response#12905
fix(http): crash in HttpTunnel::producer_run when 100 Continue precedes a transformed response#12905JakeChampion wants to merge 2 commits intoapache:10.1.xfrom
HttpTunnel::producer_run when 100 Continue precedes a transformed response#12905Conversation
…cedes a transformed response When an origin sends 100 Continue before a compressible response, `setup_100_continue_transfer()` sets `client_response_hdr_bytes` to the intermediate header size. The subsequent call to `setup_server_transfer_to_transform()` creates the tunnel buffer with `hdr_size=0` - body only, but `client_response_hdr_bytes` is not reset for non-chunked responses. `perform_cache_write_action()` then passes the stale value as `skip_bytes` to the cache-write consumer, causing `ink_release_assert(c->skip_bytes <= c->buffer_reader->read_avail())` to fire in `producer_run` when the response body is smaller than the 100 Continue headers. Fix: set `cache_write_skip` to `0` whenever a transform is present, since `setup_server_transfer_to_transform()` never writes headers into the tunnel buffer. Fixes: apache#12244
|
@JakeChampion we weren't going to do another 10.1.x release. |
I've also opened this against main, and can back port to 10.2.x instead of 10.1.x if preferred |
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the HTTP tunneling/cache-write path when an origin sends an intermediate 100 Continue before a final response that goes through a transform, leaving a stale client_response_hdr_bytes value that can be misused as cache-write skip_bytes.
Changes:
- Adjusts cache-write setup to skip
0bytes when a transform is present (server→transform tunnel buffer is body-only). - Adds a new gold test that reproduces the
100 Continue+ transform + cache-write scenario using custom Python origin/client scripts. - Introduces a dedicated compress plugin config used by the new regression test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/proxy/http/HttpSM.cc |
Prevents passing stale header-skip values into cache-write consumers when a transform is present. |
tests/gold_tests/pluginTest/compress/compress-cache-untransformed.test.py |
Adds regression test harness wiring (ATS + custom origin/client) to reproduce the crash. |
tests/gold_tests/pluginTest/compress/compress_100_continue_origin.py |
Custom origin to emit 100 Continue then a small compressible 200 OK. |
tests/gold_tests/pluginTest/compress/compress_100_continue_client.py |
Custom client to drive Expect: 100-continue through ATS and complete the exchange. |
tests/gold_tests/pluginTest/compress/etc/cache-true-untransformed.config |
Compress plugin config used by the new regression test. |
| @@ -0,0 +1,5 @@ | |||
| cache false | |||
There was a problem hiding this comment.
The config filename and surrounding test/docs say "cache=true", but this file sets cache false. In the compress plugin, cache true enables caching the transformed response and disables untransformed caching; cache false does the opposite. Please align the filename (and any references) with the actual setting, or flip the setting if the intent really is transformed caching.
| cache false | |
| cache true |
| """Origin server that sends 100 Continue then a compressible 200 OK. | ||
|
|
||
| Used to reproduce the crash in HttpTunnel::producer_run when compress.so | ||
| with cache=true is combined with a 100 Continue response from the origin. |
There was a problem hiding this comment.
Docstring says the crash is reproduced with compress.so configured with cache=true, but the test config used here sets cache false (which corresponds to untransformed caching in compress). Please update the docstring to match the actual configuration/behavior to avoid confusion when maintaining the test.
| with cache=true is combined with a 100 Continue response from the origin. | |
| with cache=false (untransformed caching) is combined with a 100 Continue | |
| response from the origin. |
| """Client that sends a POST with Expect: 100-continue through ATS. | ||
|
|
||
| Used to reproduce the crash in HttpTunnel::producer_run when compress.so | ||
| with cache=true is combined with a 100 Continue response from the origin. |
There was a problem hiding this comment.
Docstring says the crash is reproduced with compress.so configured with cache=true, but the test config used here sets cache false (which corresponds to untransformed caching in compress). Please update the docstring to match the actual configuration/behavior to avoid confusion when maintaining the test.
| with cache=true is combined with a 100 Continue response from the origin. | |
| with cache=false (untransformed caching) is combined with a 100 Continue response from the origin. |
| The crash requires two conditions in the same transaction: | ||
| 1. An intermediate response (100 Continue) is forwarded to the client, which | ||
| sets client_response_hdr_bytes to the intermediate header size via | ||
| setup_100_continue_transfer(). | ||
| 2. The final response goes through a compress transform with untransformed | ||
| cache writing (cache=true), which calls setup_server_transfer_to_transform(). | ||
| For non-chunked responses, client_response_hdr_bytes is NOT reset to 0. | ||
|
|
There was a problem hiding this comment.
This test description repeatedly refers to compress running with cache=true, but the config file used (cache-true-untransformed.config) currently sets cache false (untransformed caching). Please update the wording (and ideally the config filename) so the test documents the actual condition needed to hit the crash path.
| Regression test for compress plugin with cache=true causing assertion failure | ||
| when origin sends 100 Continue before a compressible response (#12244) |
There was a problem hiding this comment.
Test.Summary mentions "cache=true" causing the assertion failure, but the config used in this test sets cache false (untransformed caching in compress). Please update the summary string to accurately reflect the configuration under test.
| Regression test for compress plugin with cache=true causing assertion failure | |
| when origin sends 100 Continue before a compressible response (#12244) | |
| Regression test for compress plugin with untransformed caching (cache=false) | |
| causing assertion failure when origin sends 100 Continue before a compressible | |
| response (#12244) |
When an origin sends 100 Continue before a compressible response,
setup_100_continue_transfer()setsclient_response_hdr_bytesto the intermediate header size.The subsequent call to
setup_server_transfer_to_transform()creates the tunnel buffer withhdr_size=0- body only, butclient_response_hdr_bytesis not reset for non-chunked responses.perform_cache_write_action()then passes the stale value asskip_bytesto the cache-write consumer, causingink_release_assert(c->skip_bytes <= c->buffer_reader->read_avail())to fire inproducer_runwhen the response body is smaller than the 100 Continue headers.Fix: set
cache_write_skipto0whenever a transform is present, sincesetup_server_transfer_to_transform()never writes headers into the tunnel buffer.Fixes: #12244