fix(http): stale skip_bytes in cache-write consumer after 100 Continue with transform#12906
Conversation
JakeChampion
force-pushed
the
jake/fix-master
branch
2 times, most recently
from
bafc3a0 to
ad3418f
Compare
|
I think AuTest 1of4 was a flakey fail |
|
[approve ci autest 1] |
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in HttpTunnel::producer_run that occurs when an origin server sends a 100 Continue response before a compressible, non-chunked final response that triggers a transform (like the compress plugin with cache=true for untransformed caching).
The root cause is that setup_100_continue_transfer() sets client_response_hdr_bytes to the size of the 100 Continue headers. Later, when setup_server_transfer_to_transform() creates the tunnel buffer for the final response, it calls server_transfer_init(buf, 0) with hdr_size=0, meaning no headers are written to the buffer. However, for non-chunked responses, client_response_hdr_bytes is not reset. When perform_cache_write_action() passes this stale value as skip_bytes to the cache-write consumer, and the response body is smaller than the 100 Continue headers, the assertion skip_bytes <= read_avail() fails.
Changes:
- Fixed the cache write skip_bytes calculation to account for transforms
- Added comprehensive regression test with custom origin and client scripts
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/proxy/http/HttpSM.cc | Fixed perform_cache_write_action() to set cache_write_skip=0 when transform is present, since setup_server_transfer_to_transform() creates buffers without headers |
| tests/gold_tests/pluginTest/compress/compress-cache-untransformed.test.py | Regression test that reproduces the crash scenario with 100 Continue + compress transform + untransformed caching |
| tests/gold_tests/pluginTest/compress/compress_100_continue_origin.py | Custom origin server that sends 100 Continue followed by a small compressible response |
| tests/gold_tests/pluginTest/compress/compress_100_continue_client.py | Client that sends POST with Expect: 100-continue and Accept-Encoding: gzip |
| tests/gold_tests/pluginTest/compress/etc/cache-true-untransformed.config | Compress plugin config with cache=false to enable untransformed caching only |
…nue with transform When an origin sends 100 Continue before a compressible response, `setup_100_continue_transfer()` sets `client_response_hdr_bytes` to the intermediate header size. `setup_server_transfer_to_transform()` then creates the tunnel buffer with `hdr_size=0` - body only, but does not reset `client_response_hdr_bytes` for non-chunked responses. `perform_cache_write_action()` passes the stale value as `skip_bytes` to the cache-write consumer, causing `ink_release_assert` in `producer_run` when the response body is smaller than the 100 Continue headers. Fix: Set `cache_write_skip` to `0` when a transform is present, since `setup_server_transfer_to_transform()` never writes headers into the tunnel buffer.
JakeChampion
force-pushed
the
jake/fix-master
branch
from
ad3418f to
5ec9e08
Compare
|
[approve ci autest] |
| return | ||
|
|
||
| # Send 100 Continue immediately. | ||
| conn.sendall(b'HTTP/1.1 100 Continue\r\n\r\n') |
There was a problem hiding this comment.
Thank you for adding this test case. But I guess Proxy-Verifier supports 100 Continue support. ( we can merge this first and change it later )
@bneradt can we cover this case with proxy-verifier?
masaori335
left a comment
masaori335
left a comment
There was a problem hiding this comment.
Makes sense. Thank you!
|
[approve ci] |
|
Cherry-picked to 10.2.x |
…nue with transform (#12906) When an origin sends 100 Continue before a compressible response, `setup_100_continue_transfer()` sets `client_response_hdr_bytes` to the intermediate header size. `setup_server_transfer_to_transform()` then creates the tunnel buffer with `hdr_size=0` - body only, but does not reset `client_response_hdr_bytes` for non-chunked responses. `perform_cache_write_action()` passes the stale value as `skip_bytes` to the cache-write consumer, causing `ink_release_assert` in `producer_run` when the response body is smaller than the 100 Continue headers. Fix: Set `cache_write_skip` to `0` when a transform is present, since `setup_server_transfer_to_transform()` never writes headers into the tunnel buffer. (cherry picked from commit 5b32738)
When an origin sends 100 Continue before a compressible response,
setup_100_continue_transfer()setsclient_response_hdr_bytesto the intermediate header size.setup_server_transfer_to_transform()then creates the tunnel buffer withhdr_size=0- body only, but does not resetclient_response_hdr_bytesfor non-chunked responses.perform_cache_write_action()passes the stale value asskip_bytesto the cache-write consumer, causingink_release_assertinproducer_runwhen the response body is smaller than the 100 Continue headers.Fix: Set
cache_write_skipto0when a transform is present, sincesetup_server_transfer_to_transform()never writes headers into the tunnel buffer.Standard flow with no 100 Continue, no compress plugin transform:
ATS creates a tunnel buffer that contains [response headers][response body]
When writing to cache, it needs to skip past the headers so only the body gets stored
It uses a variable called
skip_bytesfor thisskip_bytes= size of response headersWhat goes wrong with 100 Continue + compress transform:
1 - Origin sends 100 Continue:
Origin -> ATS: HTTP/1.1 100 Continue\r\n\r\n (25 bytes)
ATS forwards this to the client and records
client_response_hdr_bytes = 25(the size of those headers)This is used later to know how much header data was written to the tunnel buffer
2 - Origin sends the real 200 OK response:
Because compress plugin is active, ATS sets up a server-to-transform tunnel
The buffer for this tunnel contains only the body, no headers go into this buffer at all, since the compress transform handles headers separately
client_response_hdr_bytesis not reset here for non-chunked responses, it is still 25 bytes from Step 13 - Cache write:
perform_cache_write_action()says wants to skip past the headers in the tunnel buffer before writing to cache and usesclient_response_hdr_bytesas it'sskip_bytesIn this scenario that means it tries to skip 25 bytes
But the tunnel buffer only has the body bytes in it, making the assertion fire and crash ATS