Fix TransformVConnection resource leak when abort_tunnel() is called with active request transform

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;
+      }
       // Make sure client connection is closed when we are done in case there is cruft left over
       t_state.client_info.keep_alive = HTTPKeepAlive::NO_KEEPALIVE;
       // Similarly the server connection should also be closed

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()
+
+    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')
+
+        # Read whatever response ATS sends back.
+        try:
+            response = sock.recv(4096)
+            if response:
+                first_line = response.split(b'\r\n')[0].decode(errors='replace')
+                print(f'Got response: {first_line}')
+            else:
+                print('Got response: connection closed')
+        except socket.timeout:
+            print('Got response: timeout (server may still be processing)')
+        except ConnectionError as e:
+            print(f'Got response: connection error ({e})')
+    finally:
+        sock.close()
+
+    return 0
+
+
+if __name__ == '__main__':
+    sys.exit(main())

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')
+
+        self._ts.StartBefore(self._dns)
+        self._ts.StartBefore(self._server)
+        p.StartBefore(self._ts)
+        tr.Timeout = 10
+
+
+PostEarlyResponseTransformTest()

tests/tools/plugins/CMakeLists.txt

@@ -42,6 +42,7 @@ add_autest_plugin(hook_tunnel_plugin hook_tunnel_plugin.cc)
 add_autest_plugin(tunnel_transform tunnel_transform.cc)
 add_autest_plugin(http2_close_connection http2_close_connection.cc)
 add_autest_plugin(redirect_rearm redirect_rearm.cc)
+add_autest_plugin(null_transform_request null_transform_request.cc)
 
 target_link_libraries(continuations_verify PRIVATE OpenSSL::SSL)
 target_link_libraries(ssl_client_verify_test PRIVATE OpenSSL::SSL)

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) {
+    TSHttpTxn txnp  = static_cast<TSHttpTxn>(edata);
+    TSVConn   connp = TSTransformCreate(null_transform, txnp);
+    TSHttpTxnHookAdd(txnp, TS_HTTP_REQUEST_TRANSFORM_HOOK, connp);
+    TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
+  }
+  return 0;
+}
+
+void
+TSPluginInit(int /* argc ATS_UNUSED */, const char ** /* argv ATS_UNUSED */)
+{
+  TSPluginRegistrationInfo info;
+
+  info.plugin_name   = PLUGIN_NAME;
+  info.vendor_name   = "Apache Software Foundation";
+  info.support_email = "dev@trafficserver.apache.org";
+
+  if (TSPluginRegister(&info) != TS_SUCCESS) {
+    TSError("[%s] Unable to initialize plugin (disabled)", PLUGIN_NAME);
+    return;
+  }
+
+  TSHttpHookAdd(TS_HTTP_READ_REQUEST_HDR_HOOK, TSContCreate(transform_plugin, nullptr));
+}