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]

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.

[bneradt]

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.

[bneradt]

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.

[bneradt]

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.

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

[bneradt]

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:

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.

[bneradt]

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:

Dbg(plugin_ctl, "\tEvent is %s",
    event == TS_EVENT_HTTP_TUNNEL_START ? "TS_EVENT_HTTP_TUNNEL_START" : "TS_EVENT_HTTP_READ_REQUEST_HDR");