bneradt commented on code in PR #13574:
URL: https://github.com/apache/trafficserver/pull/13574#discussion_r3825870825
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2145,6 +2145,17 @@ 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() does not clean up vc_table entries. If a request
+ // transform is present, post_transform_info.entry still points at the
+ // TransformVConnection whose chain will be freed by the abort cascade.
+ // Clean it up now so cleanup_all() in kill_this() does not call
+ // do_io_close() on freed memory.
Review Comment:
This comment doesn't match what the code below actually does, and I don't
think the crash it describes can happen.
`HttpVCTable::cleanup_entry()` only calls `do_io_close()` when `in_tunnel ==
false`:
```cpp
void
HttpVCTable::cleanup_entry(HttpVCTableEntry *e)
{
ink_assert(e->vc);
if (e->in_tunnel == false) {
...
e->vc->do_io_close();
e->vc = nullptr;
}
remove_entry(e);
}
```
At this point `post_transform_info.entry->in_tunnel` is `true`. It is set in
`do_setup_client_request_body_tunnel()` (`case HttpVC_t::TRANSFORM_VC`) and
again in `setup_transform_to_server_transfer()`, and it is cleared only in
`tunnel_handler_transform_write()` — which `abort_tunnel()` never invokes,
since `abort_tunnel()` doesn't call any consumer/producer handlers. So
`cleanup_all()` in `kill_this()` falls straight through to `remove_entry()`,
which never dereferences `e->vc`. There is no `do_io_close()` on freed memory
to prevent.
The two cases:
* `in_tunnel == true` — the real case, and this patch having to clear it is
the proof: `cleanup_all()` never touched `e->vc`, so the described
use-after-free cannot have occurred.
* `in_tunnel == false` — hypothetically: the assignment is a no-op, and
`cleanup_entry()` performs the very same `do_io_close()` on the very same
pointer, just earlier in the call chain. Also not a fix.
That said, I think you have found a real bug, just a different one.
`abort_tunnel()` only issues `do_io_read/do_io_write(this, 0, nullptr)` on the
producers/consumers and then `reset()`; it never closes the transform chain.
`kill_tunnel()` afterwards finds an empty tunnel, and `transform_cleanup()`
skips the chain because `post_transform_info.vc != nullptr`. So today
`TransformVConnection::m_closed` is never set,
`TransformTerminus::handle_event()` never reaches `delete m_tvc`, and the
TransformVConnection plus the plugin's transform continuations **leak**.
Forcing a close here does fix that.
So this looks like a fix for a leak / missing teardown rather than for a
use-after-free. Could you attach the actual stack trace or ASAN report? If
there genuinely is a crash, I want to be sure this isn't masking it rather than
fixing it. And either way the comment, the PR description, and the test
docstring should be rewritten around the real mechanism.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2145,6 +2145,17 @@ 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() does not clean up vc_table entries. If a request
+ // transform is present, post_transform_info.entry still points at the
+ // TransformVConnection whose chain will be freed by the abort cascade.
+ // Clean it up now so cleanup_all() in kill_this() does not call
+ // do_io_close() on freed memory.
+ if (post_transform_info.entry != nullptr) {
+ post_transform_info.entry->in_tunnel = false;
+ vc_table.cleanup_entry(post_transform_info.entry);
+ post_transform_info.entry = nullptr;
+ }
+ ink_release_assert(post_transform_info.entry == nullptr);
Review Comment:
This assert is tautological: the block immediately above sets
`post_transform_info.entry = nullptr`, and if the `if` wasn't taken it was
already nullptr. It can never fire. Looks like leftover repro scaffolding —
please drop it.
Relatedly, the PR description still says to "uncomment the assert after
`abort_tunnel()`" to reproduce, which no longer matches the diff.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2145,6 +2145,17 @@ 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() does not clean up vc_table entries. If a request
+ // transform is present, post_transform_info.entry still points at the
+ // TransformVConnection whose chain will be freed by the abort cascade.
+ // Clean it up now so cleanup_all() in kill_this() does not call
+ // do_io_close() on freed memory.
+ if (post_transform_info.entry != nullptr) {
+ post_transform_info.entry->in_tunnel = false;
Review Comment:
Overwriting `in_tunnel` to coerce a side effect out of `cleanup_entry()` is
a blunt instrument, and it works against the rest of the file. Every other
post-transform cleanup site deliberately preserves this flag:
* `tunnel_handler_post_or_put()` asserts
`post_transform_info.entry->in_tunnel == true` and then calls `cleanup_entry()`
*specifically so that it will not close*, with a comment explaining why.
* `state_common_wait_for_transform_read()` and `handle_server_setup_error()`
likewise call `cleanup_entry()` without touching the flag.
`in_tunnel` encodes who owns the VC — the tunnel or the SM. If the intent is
"close the transform chain now", it reads much better to say that directly:
```cpp
post_transform_info.vc->do_io_close();
vc_table.cleanup_entry(post_transform_info.entry);
post_transform_info.entry = nullptr;
```
There's a robustness cost too. The `in_tunnel == true` check is currently
what protects paths where the TVC has already been closed but the entry is
still populated — `tunnel_handler_transform_write()`'s `VC_EVENT_ERROR` case
does `c->vc->do_io_close(EHTTP_ERROR)` and leaves the entry alone. Clearing the
flag unconditionally removes that protection and turns any such path into a
genuine use-after-free.
Last thought on placement: `abort_tunnel()` is the function that leaves the
vc_table inconsistent, and `kill_tunnel()` has the same shape. Patching this
one caller leaves the trap set for the next person. A small SM helper (or
handling it on the abort path itself) would cover both.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2145,6 +2145,17 @@ 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() does not clean up vc_table entries. If a request
+ // transform is present, post_transform_info.entry still points at the
+ // TransformVConnection whose chain will be freed by the abort cascade.
+ // Clean it up now so cleanup_all() in kill_this() does not call
+ // do_io_close() on freed memory.
+ if (post_transform_info.entry != nullptr) {
+ post_transform_info.entry->in_tunnel = false;
+ vc_table.cleanup_entry(post_transform_info.entry);
+ post_transform_info.entry = nullptr;
Review Comment:
Nulling `entry` while leaving `post_transform_info.vc` non-null breaks the
pairing that two sites rely on — they check `.vc` and then dereference `.entry`:
* `tunnel_handler_post_or_put()`: `if (post_transform_info.vc != nullptr) {
ink_assert(post_transform_info.entry->in_tunnel == true); ...;
vc_table.cleanup_entry(post_transform_info.entry); }`
* `handle_server_setup_error()`: `if (post_transform_info.vc) { ...;
vc_table.cleanup_entry(post_transform_info.entry); }`
The `handle_server_setup_error()` one is safe here only incidentally:
`abort_tunnel()` already called `reset()`, so
`tunnel.get_consumer(post_transform_info.vc)` returns nullptr and the inner
guard short-circuits. Worth confirming `tunnel_handler_post_or_put()` can't be
reached after this abort — in a release build the `ink_assert`s compile out and
`cleanup_entry(nullptr)` faults on `e->in_tunnel`.
`state_common_wait_for_transform_read()` already produces this `entry ==
nullptr && vc != nullptr` state in the TRANSFORM_FAIL case, so it may well be
fine. I'd just rather see it checked than assumed.
##########
tests/gold_tests/slow_post/partial_post_client.py:
##########
@@ -0,0 +1,69 @@
+#!/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 = (
+ f'POST / HTTP/1.1\r\n'
+ f'Host: quick.server.com\r\n'
+ f'Content-Type: application/octet-stream\r\n'
+ f'Content-Length: 100000\r\n'
+ f'\r\n').encode()
Review Comment:
Minor: none of these `f` prefixes have placeholders, so plain string
literals would do (yapf/flake8 will likely flag them).
More generally, this overlaps `slow_post_client.py` in the same directory,
which already does the "send a POST and don't finish it" dance. The difference
that matters is `Content-Length` vs. `Transfer-Encoding: chunked`, so a
`--content-length N --send-bytes M` mode there would avoid a third client
script in this directory. Your call — the mechanism is different enough that a
separate file is defensible.
##########
tests/tools/plugins/null_transform_request.cc:
##########
@@ -0,0 +1,162 @@
+/** @file
+
+ Null request transform plugin hooked at TS_HTTP_READ_REQUEST_HDR_HOOK.
+
+ Used by post_early_response_transform.test.py to reproduce a use-after-free
+ in HttpSM::state_read_server_response_header() when abort_tunnel() is called
+ while a request transform is active. The transform passes request body data
+ through unmodified.
+
+ @section license License
+
+ 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.
+ */
+
+#include <cstdio>
+#include <cinttypes>
+
+#include "ts/ts.h"
+
+#define PLUGIN_NAME "null_transform_request"
+
+typedef struct {
+ TSVIO output_vio;
+ TSIOBuffer output_buffer;
+ TSIOBufferReader output_reader;
+} TransformData;
Review Comment:
`typedef struct { ... } TransformData;` is a C idiom; in C++ this is just
`struct TransformData { ... };`.
Also `<cstdio>` and `<cinttypes>` (lines 29-30) are unused now that the
`Dbg()` calls are gone.
##########
tests/tools/plugins/null_transform_request.cc:
##########
@@ -0,0 +1,162 @@
+/** @file
+
+ Null request transform plugin hooked at TS_HTTP_READ_REQUEST_HDR_HOOK.
+
+ Used by post_early_response_transform.test.py to reproduce a use-after-free
+ in HttpSM::state_read_server_response_header() when abort_tunnel() is called
+ while a request transform is active. The transform passes request body data
+ through unmodified.
+
+ @section license License
+
+ 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.
+ */
+
+#include <cstdio>
+#include <cinttypes>
+
+#include "ts/ts.h"
+
+#define PLUGIN_NAME "null_transform_request"
+
+typedef struct {
+ TSVIO output_vio;
+ TSIOBuffer output_buffer;
+ TSIOBufferReader output_reader;
+} TransformData;
+
+static TransformData *
+transform_data_alloc()
+{
+ auto *data = static_cast<TransformData
*>(TSmalloc(sizeof(TransformData)));
+ data->output_vio = nullptr;
+ data->output_buffer = nullptr;
+ data->output_reader = nullptr;
+ return data;
+}
+
+static void
+transform_data_destroy(TransformData *data)
+{
+ if (data) {
+ if (data->output_buffer) {
+ TSIOBufferDestroy(data->output_buffer);
+ }
+ TSfree(data);
+ }
+}
+
+static void
+handle_transform(TSCont contp)
+{
+ TSVConn output_conn = TSTransformOutputVConnGet(contp);
+ TSVIO input_vio = TSVConnWriteVIOGet(contp);
+ TransformData *data = static_cast<TransformData
*>(TSContDataGet(contp));
+
+ if (!data) {
+ data = transform_data_alloc();
+ data->output_buffer = TSIOBufferCreate();
+ data->output_reader = TSIOBufferReaderAlloc(data->output_buffer);
+ data->output_vio = TSVConnWrite(output_conn, contp,
data->output_reader, TSVIONBytesGet(input_vio));
+ TSContDataSet(contp, data);
+ }
+
+ if (!TSVIOBufferGet(input_vio)) {
+ TSVIONBytesSet(data->output_vio, TSVIONDoneGet(input_vio));
+ TSVIOReenable(data->output_vio);
+ return;
+ }
+
+ int64_t towrite = TSVIONTodoGet(input_vio);
+ if (towrite > 0) {
+ int64_t avail = TSIOBufferReaderAvail(TSVIOReaderGet(input_vio));
+ if (towrite > avail) {
+ towrite = avail;
+ }
+ if (towrite > 0) {
+ TSIOBufferCopy(TSVIOBufferGet(data->output_vio),
TSVIOReaderGet(input_vio), towrite, 0);
+ TSIOBufferReaderConsume(TSVIOReaderGet(input_vio), towrite);
+ TSVIONDoneSet(input_vio, TSVIONDoneGet(input_vio) + towrite);
+ }
+ }
+
+ if (TSVIONTodoGet(input_vio) > 0) {
+ if (towrite > 0) {
+ TSVIOReenable(data->output_vio);
+ TSContCall(TSVIOContGet(input_vio), TS_EVENT_VCONN_WRITE_READY,
input_vio);
+ }
+ } else {
+ TSVIONBytesSet(data->output_vio, TSVIONDoneGet(input_vio));
+ TSVIOReenable(data->output_vio);
+ TSContCall(TSVIOContGet(input_vio), TS_EVENT_VCONN_WRITE_COMPLETE,
input_vio);
+ }
+}
+
+static int
+null_transform(TSCont contp, TSEvent event, void * /* edata ATS_UNUSED */)
+{
+ if (TSVConnClosedGet(contp)) {
+ transform_data_destroy(static_cast<TransformData *>(TSContDataGet(contp)));
+ TSContDestroy(contp);
+ return 0;
+ }
+
+ switch (event) {
+ case TS_EVENT_ERROR: {
+ TSVIO input_vio = TSVConnWriteVIOGet(contp);
+ TSContCall(TSVIOContGet(input_vio), TS_EVENT_ERROR, input_vio);
+ break;
+ }
+ case TS_EVENT_VCONN_WRITE_COMPLETE:
+ TSVConnShutdown(TSTransformOutputVConnGet(contp), 0, 1);
+ break;
+ default:
+ handle_transform(contp);
+ break;
+ }
+
+ return 0;
+}
+
+static int
+transform_plugin(TSCont /* contp ATS_UNUSED */, TSEvent event, void *edata)
+{
+ if (event == TS_EVENT_HTTP_READ_REQUEST_HDR) {
Review Comment:
Following up on the de-duplication thread above: after the trimming, the
only remaining difference from `tunnel_transform.cc` is this hook point (plus
not adding the response transform). The ~70-line transform body is a verbatim
copy.
`PrepareTestPlugin` already forwards `plugin_args` into `plugin.config`, so
`tunnel_transform` could take an argument selecting
`TS_HTTP_READ_REQUEST_HDR_HOOK` vs. `TS_HTTP_TUNNEL_START_HOOK` (and whether to
add the response transform), and this file could go away entirely. That's a
better outcome than two copies of the same null transform to keep in sync.
##########
tests/gold_tests/slow_post/post_early_response_transform.test.py:
##########
@@ -0,0 +1,112 @@
+"""Verify ATS does not crash when a server replies before receiving the full
POST body and a request transform plugin is active.
+
+When a POST request has a request transform and the origin responds before the
+full body is forwarded through the transform chain, abort_tunnel() is called.
+Without the fix, post_transform_info.entry is left stale in the vc_table,
+causing a use-after-free in cleanup_all().
+"""
+
+# 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 os
+from ports import get_port
+import sys
+
+Test.Summary = __doc__
+
+
+class PostEarlyResponseTransformTest:
+ """Verify abort_tunnel with a request transform does not crash ATS."""
+
+ _partial_post_client = 'partial_post_client.py'
+ _quick_server = 'quick_server.py'
+ _init_file = '__init__.py'
+ _http_utils = 'http_utils.py'
Review Comment:
+1 to Copilot on this one: `_http_utils` is never used — line 94 rebuilds
the path inline. Please drop the class attribute.
##########
tests/gold_tests/slow_post/post_early_response_transform.test.py:
##########
@@ -0,0 +1,112 @@
+"""Verify ATS does not crash when a server replies before receiving the full
POST body and a request transform plugin is active.
+
+When a POST request has a request transform and the origin responds before the
+full body is forwarded through the transform chain, abort_tunnel() is called.
+Without the fix, post_transform_info.entry is left stale in the vc_table,
+causing a use-after-free in cleanup_all().
Review Comment:
Two things.
First, this docstring restates the "use-after-free in `cleanup_all()`"
mechanism, which I don't think holds — see my comment on `HttpSM.cc`. Whatever
the real root cause turns out to be, this needs to match it.
Second, and more important: **does this test fail on master?** AuTest would
catch a genuine traffic_server crash — `MakeATSProcess` sets `p.ReturnCode = 0`
on the ATS process and adds an `ExcludesExpression("FATAL:")` tester on
diags.log — so a real SIGSEGV/SIGABRT does fail the run. But if the pre-patch
defect is a leak rather than a crash (which is what I believe it is), this test
passes with and without the fix and isn't a regression test at all. Could you
post the failing output from master?
I also don't see any CI runs on this branch yet. Worth getting Jenkins green
before this goes further.
##########
tests/gold_tests/slow_post/post_early_response_transform.test.py:
##########
@@ -0,0 +1,112 @@
+"""Verify ATS does not crash when a server replies before receiving the full
POST body and a request transform plugin is active.
+
+When a POST request has a request transform and the origin responds before the
+full body is forwarded through the transform chain, abort_tunnel() is called.
+Without the fix, post_transform_info.entry is left stale in the vc_table,
+causing a use-after-free in cleanup_all().
+"""
+
+# 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 os
+from ports import get_port
+import sys
+
+Test.Summary = __doc__
+
+
+class PostEarlyResponseTransformTest:
Review Comment:
This class re-implements `QuickServerTest` in `quick_server.test.py` almost
line for line: same DNS server, same `quick_server.py` origin, same remap line,
the same four `records.yaml` keys, the same `StartBefore` chain, the same
`tr.Timeout = 10`. That file already parameterizes over three booleans.
Adding a `use_request_transform` parameter there — installing the plugin and
swapping in the partial-POST client — would fold this in without a second copy
of the scaffolding.
##########
tests/gold_tests/slow_post/post_early_response_transform.test.py:
##########
@@ -0,0 +1,112 @@
+"""Verify ATS does not crash when a server replies before receiving the full
POST body and a request transform plugin is active.
+
+When a POST request has a request transform and the origin responds before the
+full body is forwarded through the transform chain, abort_tunnel() is called.
+Without the fix, post_transform_info.entry is left stale in the vc_table,
+causing a use-after-free in cleanup_all().
+"""
+
+# 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 os
+from ports import get_port
+import sys
+
+Test.Summary = __doc__
+
+
+class PostEarlyResponseTransformTest:
+ """Verify abort_tunnel with a request transform does not crash ATS."""
+
+ _partial_post_client = 'partial_post_client.py'
+ _quick_server = 'quick_server.py'
+ _init_file = '__init__.py'
+ _http_utils = 'http_utils.py'
+
+ def __init__(self):
+ """Configure and run the test."""
+ tr = Test.AddTestRun('Partial POST with request transform and early
server response')
+ self._configure_dns(tr)
+ self._configure_server(tr)
+ self._configure_traffic_server(tr)
+ self._configure_client(tr)
+
+ def _configure_dns(self, tr: 'TestRun') -> None:
+ """Configure the DNS process.
+
+ :param tr: The test run to associate the DNS process with.
+ """
+ self._dns = tr.MakeDNServer('dns', default='127.0.0.1')
+
+ def _configure_server(self, tr: 'TestRun') -> None:
+ """Configure the quick-responding origin server.
+
+ The server responds immediately after receiving the request headers,
+ before the full POST body arrives.
+
+ :param tr: The test run to associate the server process with.
+ """
+ server = tr.Processes.Process('server')
+ server_port = get_port(server, 'http_port')
+ server.Command = f'{sys.executable} {self._quick_server} 127.0.0.1
{server_port}'
+ server.Ready = When.PortOpenv4(server_port)
+ self._server = server
+
+ def _configure_traffic_server(self, tr: 'TestRun') -> None:
+ """Configure ATS with the null_transform_request plugin.
+
+ :param tr: The test run to associate the ATS process with.
+ """
+ self._ts = tr.MakeATSProcess('ts')
+ self._ts.Disk.remap_config.AddLine(f'map /
http://quick.server.com:{self._server.Variables.http_port}')
+ self._ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http',
+ 'proxy.config.dns.nameservers':
f'127.0.0.1:{self._dns.Variables.Port}',
+ 'proxy.config.dns.resolv_conf': 'NULL',
+ })
+ Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir,
'null_transform_request.so'), self._ts)
+
+ def _configure_client(self, tr: 'TestRun') -> None:
+ """Configure the partial POST client.
+
+ Sends a POST with a large Content-Length but only a small body,
+ triggering abort_tunnel when the origin responds early.
+
+ :param tr: The test run to associate the client process with.
+ """
+ tools_dir = self._ts.Variables.AtsTestToolsDir
+ 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._quick_server, Test.RunDirectory)
+ 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('Got response', 'Verify
client received a response from ATS')
Review Comment:
Both of these assertions are vacuous:
* The client's `main()` unconditionally does `return 0` on every path,
including the `socket.timeout` and `ConnectionError` handlers, so `p.ReturnCode
= 0` cannot fail.
* `ContainsExpression('Got response', ...)` matches `Got response: timeout
(server may still be processing)` and `Got response: connection closed` just as
happily as a real response.
So the test currently rests entirely on the implicit "ATS didn't crash"
check. Please assert on the actual status line the proxy is expected to return,
and have the client exit non-zero when it doesn't get one — compare
`quick_server.test.py`, which checks for `HTTP/1.1 200 OK` explicitly.
##########
tests/tools/plugins/null_transform_request.cc:
##########
@@ -0,0 +1,162 @@
+/** @file
+
+ Null request transform plugin hooked at TS_HTTP_READ_REQUEST_HDR_HOOK.
+
+ Used by post_early_response_transform.test.py to reproduce a use-after-free
+ in HttpSM::state_read_server_response_header() when abort_tunnel() is called
+ while a request transform is active. The transform passes request body data
+ through unmodified.
+
+ @section license License
+
+ 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.
+ */
+
+#include <cstdio>
+#include <cinttypes>
+
+#include "ts/ts.h"
+
+#define PLUGIN_NAME "null_transform_request"
+
+typedef struct {
+ TSVIO output_vio;
+ TSIOBuffer output_buffer;
+ TSIOBufferReader output_reader;
+} TransformData;
+
+static TransformData *
+transform_data_alloc()
+{
+ auto *data = static_cast<TransformData
*>(TSmalloc(sizeof(TransformData)));
+ data->output_vio = nullptr;
+ data->output_buffer = nullptr;
+ data->output_reader = nullptr;
+ return data;
+}
+
+static void
+transform_data_destroy(TransformData *data)
+{
+ if (data) {
+ if (data->output_buffer) {
+ TSIOBufferDestroy(data->output_buffer);
+ }
+ TSfree(data);
+ }
+}
+
+static void
+handle_transform(TSCont contp)
+{
+ TSVConn output_conn = TSTransformOutputVConnGet(contp);
+ TSVIO input_vio = TSVConnWriteVIOGet(contp);
+ TransformData *data = static_cast<TransformData
*>(TSContDataGet(contp));
+
+ if (!data) {
+ data = transform_data_alloc();
+ data->output_buffer = TSIOBufferCreate();
+ data->output_reader = TSIOBufferReaderAlloc(data->output_buffer);
+ data->output_vio = TSVConnWrite(output_conn, contp,
data->output_reader, TSVIONBytesGet(input_vio));
+ TSContDataSet(contp, data);
+ }
+
+ if (!TSVIOBufferGet(input_vio)) {
+ TSVIONBytesSet(data->output_vio, TSVIONDoneGet(input_vio));
+ TSVIOReenable(data->output_vio);
+ return;
+ }
+
+ int64_t towrite = TSVIONTodoGet(input_vio);
+ if (towrite > 0) {
+ int64_t avail = TSIOBufferReaderAvail(TSVIOReaderGet(input_vio));
+ if (towrite > avail) {
+ towrite = avail;
+ }
+ if (towrite > 0) {
+ TSIOBufferCopy(TSVIOBufferGet(data->output_vio),
TSVIOReaderGet(input_vio), towrite, 0);
+ TSIOBufferReaderConsume(TSVIOReaderGet(input_vio), towrite);
+ TSVIONDoneSet(input_vio, TSVIONDoneGet(input_vio) + towrite);
+ }
+ }
+
+ if (TSVIONTodoGet(input_vio) > 0) {
+ if (towrite > 0) {
+ TSVIOReenable(data->output_vio);
+ TSContCall(TSVIOContGet(input_vio), TS_EVENT_VCONN_WRITE_READY,
input_vio);
+ }
+ } else {
+ TSVIONBytesSet(data->output_vio, TSVIONDoneGet(input_vio));
+ TSVIOReenable(data->output_vio);
+ TSContCall(TSVIOContGet(input_vio), TS_EVENT_VCONN_WRITE_COMPLETE,
input_vio);
+ }
+}
+
+static int
+null_transform(TSCont contp, TSEvent event, void * /* edata ATS_UNUSED */)
+{
+ if (TSVConnClosedGet(contp)) {
Review Comment:
This bot comment is incorrect — please don'''t act on it.
`TSTransformCreate()` returns a `TSVConn` that *is* the continuation, so
passing it to `TSVConnClosedGet()` is the standard idiom for every transform
plugin in the tree: see `tests/tools/plugins/tunnel_transform.cc` and
`example/plugins/c-api/null_transform/null_transform.cc`, which both do exactly
this. The code here is correct as written.
--
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]
