bneradt commented on code in PR #13574:
URL: https://github.com/apache/trafficserver/pull/13574#discussion_r3855100085
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2145,6 +2145,16 @@ HttpSM::state_read_server_response_header(int event,
void *data)
// If there is a post body in transit, give up on it
if (tunnel.is_tunnel_alive()) {
tunnel.abort_tunnel();
+ // abort_tunnel() cancels I/O but does not close VCs or clean up
+ // vc_table entries. When a request transform is active the
+ // post_transform_info entry still references the TransformVConnection
+ // with in_tunnel=true, which causes cleanup_entry() to skip
+ // do_io_close() — leaking the VC. Close it explicitly here.
+ if (post_transform_info.entry != nullptr) {
+ post_transform_info.vc->do_io_close();
+ vc_table.cleanup_entry(post_transform_info.entry);
+ post_transform_info.entry = nullptr;
Review Comment:
Still open from the last round — flagging once rather than re-explaining.
This leaves `post_transform_info.vc` non-null with `entry == nullptr`, and
`tunnel_handler_post_or_put()` checks only `.vc` before dereferencing `.entry`.
Leaving `.vc` set is deliberate and correct (it's what makes
`transform_cleanup()` skip the chain), so the only question is whether
`tunnel_handler_post_or_put()` is reachable after this abort. If you've
satisfied yourself that it isn't, a one-line comment saying so would save the
next reader the trip.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2145,6 +2145,16 @@ HttpSM::state_read_server_response_header(int event,
void *data)
// If there is a post body in transit, give up on it
if (tunnel.is_tunnel_alive()) {
tunnel.abort_tunnel();
+ // abort_tunnel() cancels I/O but does not close VCs or clean up
+ // vc_table entries. When a request transform is active the
+ // post_transform_info entry still references the TransformVConnection
+ // with in_tunnel=true, which causes cleanup_entry() to skip
+ // do_io_close() — leaking the VC. Close it explicitly here.
+ if (post_transform_info.entry != nullptr) {
+ post_transform_info.vc->do_io_close();
Review Comment:
This is the shape I was hoping for — the comment now describes the actual
defect, and the close is explicit rather than a side effect of clearing
`in_tunnel`.
One nit: the guard tests `post_transform_info.entry`, but this line
dereferences `post_transform_info.vc`. The invariant does hold today —
`do_post_transform_open()` only creates the entry when `vc` is non-null and
sets `entry->vc = vc`, and `state_common_wait_for_transform_read()` nulls both
together — but `post_transform_info.entry->vc->do_io_close()` is the more
direct expression, and it's the very pointer `cleanup_entry()` asserts on
(`ink_assert(e->vc)`) one line later. Either that, or guard on `.vc` the way
the other sites in this file do.
For the record on the other direction: `TransformVConnection::do_io_close()`
early-returns on `m_closed != 0`, so a double close here is harmless.
##########
tests/gold_tests/slow_post/quick_server.test.py:
##########
@@ -105,22 +116,30 @@ def run(self):
http_utils = os.path.join(tools_dir, 'http_utils.py')
tr.Setup.CopyAs(self._init_file, Test.RunDirectory)
tr.Setup.CopyAs(http_utils, Test.RunDirectory)
- tr.Setup.CopyAs(self._slow_post_client, Test.RunDirectory)
tr.Setup.CopyAs(self._quick_server, Test.RunDirectory)
- client_command = (f'{sys.executable} {self._slow_post_client} '
- '127.0.0.1 '
- f'{self._ts.Variables.port} ')
- if not self._should_abort_request:
- client_command += '--finish-request '
- p = tr.Processes.Default
- p.Command = client_command
- if self._should_abort_request or self._should_abort_response_headers:
- p.Streams.All += Testers.ExcludesExpression('HTTP/1.1 200 OK',
'Verify response was received')
+ if self._use_request_transform:
+ tr.Setup.CopyAs(self._partial_post_client, Test.RunDirectory)
+ p = tr.Processes.Default
+ p.Command = (f'{sys.executable} {self._partial_post_client} '
+ f'127.0.0.1 {self._ts.Variables.port}')
+ p.ReturnCode = 0
+ p.Streams.All += Testers.ContainsExpression('HTTP/1.1', 'Verify
client received an HTTP response')
Review Comment:
This is the one item I'd hold the PR on.
The client's reset path prints `HTTP/1.1 connection reset (expected for
partial POST)` (`partial_post_client.py:56`), worded such that it satisfies
`ContainsExpression('HTTP/1.1')`. So the tester labelled "Verify client
received an HTTP response" passes when no response was received at all — and
`p.ReturnCode = 0` passes too, because that path returns 0. Someone scanning
this file sees a real check where there isn't one.
Please pin down what ATS is actually expected to do here. The origin sends a
complete `HTTP/1.1 200 OK` / `Content-Length: 0` before the body finishes, and
`state_read_server_response_header()` sets `NO_KEEPALIVE` on both sides after
the abort — so I'd expect the 200 to be forwarded and then the connection
closed, i.e. `Testers.ContainsExpression('HTTP/1.1 200 OK', ...)`
deterministically, matching the other runs in this file.
If it genuinely races between "200 then FIN" and "RST", then say so
explicitly: print something that isn't shaped like a status line, assert on it
separately, and comment the race. As written, the message and the assertion are
engineered to agree with each other regardless of what ATS did.
##########
tests/gold_tests/slow_post/quick_server.test.py:
##########
@@ -132,3 +151,7 @@ def run(self):
for abort_response_headers in [True, False]:
test = QuickServerTest(abort_request, drain_request,
abort_response_headers)
test.run()
+
+# Partial POST with a request transform plugin: exercises the abort_tunnel()
+# cleanup path for TransformVConnection entries in the vc_table.
+QuickServerTest(abort_request=True, drain_request=False,
abort_response_headers=False, use_request_transform=True).run()
Review Comment:
Two things on this line.
`abort_request=True` is inert for the transform run — nothing reads
`_should_abort_request` in the `use_request_transform` branch — yet the
generated run name will still print "Aborting request: True". Pass `False`, or
leave the flags that don't apply out of the name.
More important: now that the root cause is correctly identified as a leaked
`TransformVConnection` rather than a use-after-free, this run can't catch a
regression on its own — a leak doesn't fail an autest. It only has teeth under
ASAN/LSAN. Worth saying that in the comment above, and worth confirming the
ASAN autest job actually runs `slow_post`; otherwise this is a "doesn't crash"
smoke test and the leak could come back unnoticed.
##########
tests/gold_tests/slow_post/quick_server.test.py:
##########
@@ -27,24 +31,27 @@ class QuickServerTest:
"""Verify that ATS doesn't delay responses behind slow posts."""
_init_file = '__init__.py'
- _http_utils = 'http_utils.py'
_slow_post_client = 'slow_post_client.py'
+ _partial_post_client = 'partial_post_client.py'
_quick_server = 'quick_server.py'
_dns_counter = 0
_server_counter = 0
_ts_counter = 0
- def __init__(self, abort_request: bool, drain_request: bool,
abort_response_headers: bool):
+ def __init__(self, abort_request: bool, drain_request: bool,
abort_response_headers: bool, use_request_transform: bool = False):
Review Comment:
This signature is exactly 132 characters, right at `column_limit` in
`.style.yapf`. Please run `cmake --build build -t format` so we know yapf
agrees rather than relying on the boundary — it may well want to wrap it.
##########
tests/gold_tests/slow_post/partial_post_client.py:
##########
@@ -0,0 +1,77 @@
+#!/usr/bin/env python3
+"""Send a partial POST to trigger the abort_tunnel code path.
+
+Sends POST headers claiming a large Content-Length but only sends a small
+chunk of body data. When a request transform plugin is active, this causes
+ATS to call abort_tunnel() while the transform entry is still in the vc_table.
+"""
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import socket
+import sys
+
+
+def main() -> int:
+ """Run the client."""
+ host = sys.argv[1] if len(sys.argv) > 1 else '127.0.0.1'
+ port = int(sys.argv[2]) if len(sys.argv) > 2 else 8080
+
+ request = (
+ 'POST / HTTP/1.1\r\n'
+ 'Host: quick.server.com\r\n'
+ 'Content-Type: application/octet-stream\r\n'
+ 'Content-Length: 100000\r\n'
+ '\r\n').encode()
+
+ partial_body = b'x' * 4096
+
+ sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+ sock.settimeout(5)
+ try:
+ sock.connect((host, port))
+ sock.sendall(request + partial_body)
+ print(f'Sent POST headers (Content-Length: 100000) +
{len(partial_body)} bytes')
+
+ try:
+ response = sock.recv(4096)
+ except ConnectionError:
+ # ATS may reset the connection after responding since the POST
+ # body is incomplete. This is acceptable — the important thing
+ # is that ATS did not crash.
+ print('HTTP/1.1 connection reset (expected for partial POST)')
+ return 0
Review Comment:
Same point as on the test, from this side: `'HTTP/1.1 connection reset ...'`
is doing double duty as a human-readable log line and as the token that
`ContainsExpression('HTTP/1.1')` matches over in `quick_server.test.py`. If a
reset really is an acceptable outcome, print something that can't be mistaken
for a status line (`CONNECTION RESET`) and give the test a tester for that
string specifically.
Also worth checking: `except ConnectionError` won't catch a clean FIN — that
surfaces as `recv()` returning `b''`, which falls through to the `return 1`
branch below. Since ATS sets `NO_KEEPALIVE` on this path, a clean close after
the response is the more likely outcome, so make sure the branch you expect to
hit is the one you're actually asserting on.
##########
tests/tools/plugins/tunnel_transform.cc:
##########
@@ -99,7 +107,8 @@ handle_transform(TSCont contp, bool forward)
data->output_buffer = TSIOBufferCreate();
data->output_reader = TSIOBufferReaderAlloc(data->output_buffer);
Dbg(plugin_ctl, "\tWriting %" PRId64 " bytes on VConn",
TSVIONBytesGet(input_vio));
- data->output_vio = TSVConnWrite(output_conn, contp, data->output_reader,
INT64_MAX);
+ int64_t nbytes = (request_hdr_mode && TSVIONBytesGet(input_vio) > 0) ?
TSVIONBytesGet(input_vio) : INT64_MAX;
Review Comment:
This is load-bearing and needs a comment, because it reads like an
incidental tweak and someone will eventually "simplify" it back.
`TSVConnWrite()`'s `nbytes` becomes the terminus write VIO's `nbytes`, which
is what `TransformTerminus::handle_event()` hands the SM as the
`TRANSFORM_READ_READY` payload. `state_request_wait_for_transform_read()` then
does:
```cpp
size = *(static_cast<int64_t *>(data));
if (size != INT64_MAX && size >= 0) {
t_state.hdr_info.transform_request_cl = size;
...
} else {
// No content length from the post. This is a no go
event = VC_EVENT_ERROR;
Log::error("Request transformation failed to set content length");
}
```
So with the original `INT64_MAX`, the request-transform path fails the
transaction before the post tunnel is ever set up, and the new test run would
never reach `abort_tunnel()` at all. Something like "a request transform must
report a real content length — INT64_MAX makes
`state_request_wait_for_transform_read()` fail the transaction" would make that
clear.
Good that the default mode still passes `INT64_MAX`:
`tests/gold_tests/tunnel/tunnel_transform.test.py` loads this plugin with no
arguments, so that path is unchanged.
##########
tests/tools/plugins/tunnel_transform.cc:
##########
@@ -280,8 +291,9 @@ transform_plugin(TSCont /* contp ATS_UNUSED */, TSEvent
event, void *edata)
Dbg(plugin_ctl, "Entering transform_plugin()");
switch (event) {
+ case TS_EVENT_HTTP_READ_REQUEST_HDR:
case TS_EVENT_HTTP_TUNNEL_START:
- Dbg(plugin_ctl, "\tEvent is TS_EVENT_HTTP_TUNNEL_START");
+ Dbg(plugin_ctl, "\tEvent is %d", event);
Review Comment:
Minor: this drops the readable event name for both cases, and the two modes
are now the main thing you'd be using this plugin's debug output to
distinguish. Keeping them apart is worth more than sharing the line:
```cpp
Dbg(plugin_ctl, "\tEvent is %s",
event == TS_EVENT_HTTP_TUNNEL_START ? "TS_EVENT_HTTP_TUNNEL_START" :
"TS_EVENT_HTTP_READ_REQUEST_HDR");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
