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

[sxia-aviatrix]

Summary

Fix a resource leak in HttpSM::state_read_server_response_header() when
abort_tunnel() is called while a request transform plugin registered at
TS_HTTP_READ_REQUEST_HDR_HOOK is active.

Bug

When a POST request has a request transform and the origin server responds
before the full body is forwarded through the transform chain:

1. state_read_server_response_header() calls abort_tunnel()
2. abort_tunnel() cancels I/O on tunnel producers/consumers and calls
   reset(), but does not close VCs or clean up vc_table entries
3. post_transform_info.entry still references the TransformVConnection
   with in_tunnel = true
4. cleanup_all() in kill_this() calls cleanup_entry(), which skips
   do_io_close() because in_tunnel == true
5. The TransformVConnection and the plugin's transform continuations are
   never closed — a resource leak on every affected request

The bug only triggers when the request transform is added before the tunnel
starts (e.g. at TS_HTTP_READ_REQUEST_HDR_HOOK). Transforms added at
TS_HTTP_TUNNEL_START_HOOK become part of the tunnel chain and are properly
cleaned up by abort_tunnel().

An ink_release_assert(post_transform_info.entry == nullptr) placed after
abort_tunnel() confirms the stale entry on every request that hits this
path. GDB on the resulting core shows:

#7  HttpSM::state_read_server_response_header at HttpSM.cc:2158
post_transform_info = {entry = 0x7f4ee73ba950, vc = 0x5130000202c0}
*post_transform_info.entry = {vc = 0x5130000202c0, vc_type = TRANSFORM_VC,
  in_tunnel = true}

Fix

After abort_tunnel(), explicitly close and clean up the orphaned
TransformVConnection:

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;
}

This calls do_io_close() directly on the transform VC rather than
clearing the in_tunnel flag, preserving the ownership semantics that
other call sites rely on. With in_tunnel == true, cleanup_entry()
skips its own do_io_close() and falls through to remove_entry(),
so there is no double-close.

post_transform_info.vc is left non-null, which correctly tells
transform_cleanup() in kill_this() that the chain was already closed.

Test

Added post_early_response_transform.test.py with the
null_transform_request test plugin. The test sends a partial POST
(Content-Length: 100000, sends only small chunks slowly) while the
origin responds immediately. This exercises the abort_tunnel() path with
an active request transform, verifying ATS handles the cleanup without
leaking resources.

[Copilot]

Pull request overview

Fixes a use-after-free crash in ATS’s HTTP state machine when an early origin response triggers abort_tunnel() while a request transform is active, and adds an AuTest regression test + supporting test plugin to reproduce the timing-sensitive scenario.

Changes:

• Clean up post_transform_info.entry after tunnel.abort_tunnel() to prevent kill_this() → vc_table.cleanup_all() from closing a stale VC pointer.
• Add a new AuTest (post_early_response_transform.test.py) plus a partial-POST client helper to reproduce the early-response / request-transform timing case.
• Add a dedicated test plugin (null_transform_request) and wire it into the test-plugin build.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/proxy/http/HttpSM.cc	Clears the stale post-transform vc_table entry after abort_tunnel() to avoid use-after-free during later cleanup.
tests/tools/plugins/null_transform_request.cc	New test plugin registering a request transform at TS_HTTP_READ_REQUEST_HDR_HOOK to reproduce the pre-tunnel transform case.
tests/tools/plugins/CMakeLists.txt	Builds the new null_transform_request autest plugin.
tests/gold_tests/slow_post/post_early_response_transform.test.py	New AuTest scenario driving a partial POST through ATS with the request transform active and an origin that replies immediately.
tests/gold_tests/slow_post/partial_post_client.py	Helper client that sends a large Content-Length but only a small body to trigger abort behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

tests/tools/plugins/null_transform_request.cc:60

• output_reader allocated via TSIOBufferReaderAlloc() is never freed. Prefer freeing the reader (e.g., via TSIOBufferReaderFree(data->output_reader)) before destroying the buffer to avoid leaks and make ownership explicit.

  if (data) {
    if (data->output_buffer) {
      TSIOBufferDestroy(data->output_buffer);
    }
    TSfree(data);
  }

tests/gold_tests/slow_post/post_early_response_transform.test.py:73

• This assertion will pass even on the client's timeout/error paths because they also print Got response:. To make the regression test more robust, assert on a specific successful response pattern (e.g., Got response: HTTP/1.1) and/or explicitly fail on timeout to avoid false positives.

p.Streams.All += Testers.ContainsExpression('Got response', 'Verify client received a response from ATS')

src/proxy/http/HttpSM.cc:2157

• This fix relies on mutating an internal flag (in_tunnel) to force vc_table.cleanup_entry() behavior, which tightly couples HttpSM to vc_table/entry invariants. Consider encapsulating this as a dedicated helper (e.g., cleanup_post_transform_entry_after_abort()), or better: have abort_tunnel()/the tunnel own clearing any associated vc_table entries, so callers don’t need to manually adjust entry state to achieve correct cleanup.

      // 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;
      }

[bneradt]

Thanks for the fix.

Please reorganize tests/gold_tests/slow_post/post_early_response_transform.test.py as a Test class. See tests/gold_tests/ats_probe/ats_probe.test.py as an example.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/proxy/http/HttpSM.cc:2152

• The comment implies abort_tunnel() frees the transform chain, but HttpTunnel::abort_tunnel() only cancels I/O and resets the tunnel bookkeeping (it does not close or delete VCs). This is misleading for future maintenance and obscures why the explicit cleanup_entry() is needed here.

      // 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.

[bneradt]

Thanks for digging into this one — the teardown gap you found looks real. My main concern is that the mechanism described in the PR (and in the code comment and the test docstring) doesn't hold up: cleanup_entry() only calls do_io_close() when in_tunnel == false, and at this point it is true, so cleanup_all() can't have been dereferencing the stale VC. Details inline.

I do think there's a genuine bug here, just a different one: before this patch the TransformVConnection and the plugin's transform continuations are never closed on the abort path — a leak rather than a use-after-free. If that's what you were chasing, the fix is close to right, but it should say so directly instead of overwriting the in_tunnel ownership flag. If there really is a crash, could you attach the stack trace or ASAN report so we can confirm this addresses it?

The rest of the comments are on the test and the test plugin. Also flagging that one of the Copilot comments below is incorrect — replied in that thread.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

tests/gold_tests/slow_post/post_early_response_transform.test.py:6

• The module docstring describes the failure mode as a "use-after-free", but the PR description and the code comment in HttpSM.cc describe a stale post_transform_info.entry that prevents cleanup_entry() from closing the transform VC (i.e., a leak). Updating the docstring to match the actual bug being tested will avoid confusion for future readers.

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().

tests/tools/plugins/null_transform_request.cc:8

• The header comment says this reproduces a "use-after-free", but the PR description indicates the issue is that a request transform VC is never closed after abort_tunnel() (resource leak). Adjust the comment so the plugin’s purpose matches the bug being fixed/tested.

  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.

tests/gold_tests/slow_post/partial_post_client.py:38

• The PR description says the test sends a very large Content-Length (10,000,000) and trickles body chunks slowly, but this client currently uses Content-Length: 100000 and sends a single 4096-byte chunk in one sendall(). Consider aligning either the PR description or this client behavior to reduce confusion and ensure the test reliably exercises the intended abort_tunnel() path.

    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'

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

[shinrich]

[approve ci]

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

[bneradt]

Thank you for the updates. I'm going to build a dev rpm of this and try it in production to make sure it's stable for us. If that goes well I'll approve.

[bneradt]

This is much better — thanks for turning it around so quickly. The HttpSM.cc change now says what it does and does what it says, folding the test into quick_server.test.py came out cleaner than I expected, and teaching tunnel_transform.cc a mode flag beats a second copy of the plugin. The C++ fix itself looks right to me.

One item I'd call blocking, and it's in the test: the client's connection-reset message is worded so that it satisfies the ContainsExpression('HTTP/1.1') check, which means the tester labelled "Verify client received an HTTP response" passes even when no response arrived. Worth nailing down what ATS is actually expected to return here.

Everything else is smaller — one nit on the fix, a note that the nbytes change in the plugin is load-bearing and should be commented (I traced why: INT64_MAX would make state_request_wait_for_transform_read() fail the transaction outright, so without it the new run wouldn't exercise the abort path at all), and a flag that a leak regression won't be caught by this test outside ASAN.

[bneradt]

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.

[sxia-aviatrix]

I have updated the code and comments. But I don't think ASAN/LSAN can catch this, since this is resource leak and vc pointer is reachable. I also tried ASAN/LSAN myself and confirmed that they cannot fail the tests.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

tests/gold_tests/slow_post/partial_post_client.py:57

• The ConnectionError path prints a string starting with HTTP/1.1, so the gold test's ContainsExpression('HTTP/1.1', ...) can pass even when no HTTP response bytes were actually received. Use a distinct marker for the reset case so the test output reflects what happened.

        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)')

tests/gold_tests/slow_post/quick_server.test.py:5

• The PR description says it adds post_early_response_transform.test.py with a null_transform_request plugin, but this change set instead extends slow_post/quick_server.test.py, adds partial_post_client.py, and updates tunnel_transform with a request_hdr mode. Please update the PR description (or filenames) so the documented test/plugin names match what’s actually in the diff.

"""Verify ATS handles a server that replies before receiving a full request.

Also verifies ATS does not leak the TransformVConnection when abort_tunnel()
is called with a request transform plugin active (use_request_transform=True).
"""

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

tests/gold_tests/slow_post/quick_server.test.py:127

• This assertion is currently too broad: it will also match the client's "HTTP/1.1 connection reset..." message, so the test can pass even if ATS never delivers the expected response. Tighten the matcher to the actual response status line returned by quick_server.py (HTTP/1.1 200 OK) so the test fails when no response is received.

            p.Streams.All += Testers.ContainsExpression('HTTP/1.1', 'Verify client received an HTTP response')

tests/gold_tests/slow_post/partial_post_client.py:57

• The partial-post client treats a connection reset as success and prints a line containing "HTTP/1.1", which can make the gold test pass even when no HTTP response was actually received. This weakens the regression and can mask failures in the early-response path.

Consider treating a reset before receiving the response as an error (non-zero exit) and logging it as such.

            # 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

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/proxy/http/HttpSM.cc:2156

• vc_table.cleanup_entry() will call do_io_close() itself when entry->in_tunnel == false (see HttpVCTable::cleanup_entry). The current code unconditionally calls do_io_close() first, which can become a double-close / UAF if this block ever runs with in_tunnel == false. Guard the explicit close (or assert the invariant) so this remains safe even if the control-flow changes.

      if (post_transform_info.entry != nullptr) {
        post_transform_info.entry->vc->do_io_close();
        vc_table.cleanup_entry(post_transform_info.entry);
        post_transform_info.entry = nullptr;

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tests/gold_tests/slow_post/quick_server.test.py:5

• The module docstring says this test “verifies ATS does not leak the TransformVConnection”, but the added run is explicitly a “doesn't crash” smoke test and does not (and cannot) assert resource-leak behavior. Please reword the docstring to avoid claiming a leak check that isn’t performed.

This issue also appears on line 99 of the same file.

"""Verify ATS handles a server that replies before receiving a full request.

Also verifies ATS does not leak the TransformVConnection when abort_tunnel()
is called with a request transform plugin active (use_request_transform=True).
"""

tests/gold_tests/slow_post/quick_server.test.py:101

• The PR description says it adds post_early_response_transform.test.py using a null_transform_request plugin, but the actual change extends slow_post/quick_server.test.py and uses tunnel_transform.so with a request_hdr mode. Please update the PR description (or rename/move the test/plugin) so reviewers and future readers can find the test that covers the fix.

        if self._use_request_transform:
            Test.PrepareTestPlugin(
                os.path.join(Test.Variables.AtsTestPluginsDir, 'tunnel_transform.so'), self._ts, plugin_args='request_hdr')