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: 10000000, 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.

[bneradt]

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:

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.

[sxia-aviatrix]

Thanks for the review. I think you are right. This is a leak instead of use-after-free.
Unfortunately, since I think this is a resource leak not memory leak, so I cannot find anything with ASAN or LSAN.

[sxia-aviatrix]

I do have gdb stack trace and hope this can be helpful:

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/tmp/sb_pr4/post_early_response_transform/ts/bin/traffic_server --bind_stderr /'.
Program terminated with signal SIGABRT, Aborted.
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44

warning: 44     ./nptl/pthread_kill.c: No such file or directory
[Current thread is 1 (Thread 0x7f4ef591e6c0 (LWP 287043))]
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007f4f014a927e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007f4f0148c8ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x0000632477666236 in ink_abort (message_format=message_format@entry=0x6324787af000 "%s:%d: failed assertion `%s`") at /src/trafficserver_pr/trafficserver/src/tscore/ink_error.cc:99
#6  0x000063247765f9b6 in _ink_assert (expression=expression@entry=0x6324787d9200 "post_transform_info.entry == nullptr", file=file@entry=0x6324787d3480 "/src/trafficserver_pr/trafficserver/src/proxy/http/HttpSM.cc", line=line@entry=2158) at /src/trafficserver_pr/trafficserver/src/tscore/ink_assert.cc:35
#7  0x00006324777cd115 in HttpSM::state_read_server_response_header (this=0x7f4ee73b8000, event=<optimized out>, data=<optimized out>) at /src/trafficserver_pr/trafficserver/src/proxy/http/HttpSM.cc:2158
#8  0x00006324777deec7 in HttpSM::main_handler (this=0x7f4ee73b8000, event=<optimized out>, data=<optimized out>) at /src/trafficserver_pr/trafficserver/src/proxy/http/HttpSM.cc:2880
#9  0x000063247823737a in Continuation::handleEvent (data=0x7f4ee749b5c8, event=100, this=0x7f4ee73b8000) at /src/trafficserver_pr/trafficserver/include/iocore/eventsystem/Continuation.h:228
#10 Continuation::handleEvent (data=0x7f4ee749b5c8, event=100, this=0x7f4ee73b8000) at /src/trafficserver_pr/trafficserver/include/iocore/eventsystem/Continuation.h:224
#11 read_signal_and_update (event=100, vc=0x7f4ee749b1b0) at /src/trafficserver_pr/trafficserver/src/iocore/net/UnixNetVConnection.cc:86
#12 0x00006324782488b8 in UnixNetVConnection::net_read_io (this=0x7f4ee749b1b0, nh=<optimized out>) at /src/trafficserver_pr/trafficserver/src/iocore/net/UnixNetVConnection.cc:600
#13 0x00006324782f51c4 in NetHandler::process_ready_list (this=this@entry=0x7f4efa3a43a0) at /src/trafficserver_pr/trafficserver/src/iocore/net/NetHandler.cc:265
#14 0x00006324782f5da6 in NetHandler::waitForActivity (this=0x7f4efa3a43a0, timeout=<optimized out>) at /src/trafficserver_pr/trafficserver/src/iocore/net/NetHandler.cc:360
#15 0x0000632478443b90 in EThread::execute_regular (this=this@entry=0x7f4efa3a3800) at /src/trafficserver_pr/trafficserver/src/iocore/eventsystem/UnixEThread.cc:326
#16 0x0000632478444552 in EThread::execute (this=0x7f4efa3a3800) at /src/trafficserver_pr/trafficserver/src/iocore/eventsystem/UnixEThread.cc:383
#17 EThread::execute (this=0x7f4efa3a3800) at /src/trafficserver_pr/trafficserver/src/iocore/eventsystem/UnixEThread.cc:360
#18 0x000063247843876e in spawn_thread_internal (a=a@entry=0x50600000faa0) at /src/trafficserver_pr/trafficserver/src/iocore/eventsystem/Thread.cc:75
#19 0x00007f4f027b1a42 in asan_thread_start (arg=0x7f4f0074f000) at ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234
#20 0x00007f4f01500aa4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:447
#21 0x00007f4f0158dc6c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
#7  0x00006324777cd115 in HttpSM::state_read_server_response_header (this=0x7f4ee73b8000, event=<optimized out>, data=<optimized out>) at /src/trafficserver_pr/trafficserver/src/proxy/http/HttpSM.cc:2158
2158          ink_release_assert(post_transform_info.entry == nullptr);
$1 = {entry = 0x7f4ee73ba950, vc = 0x5130000202c0}
$2 = {vc = 0x5130000202c0, read_buffer = 0x0, write_buffer = 0x0, read_vio = 0x0, write_vio = 0x0, vc_read_handler = NULL, vc_write_handler = NULL, vc_type = HttpVC_t::TRANSFORM_VC, sm = 0x7f4ee73b8000, eos = false, in_tunnel = true}
[ats-develop@b07c50b9e0d7 trafficserver ]$ 

[sxia-aviatrix]

The test and stack trace are got in upstream/master.

[sxia-aviatrix]

I can reproduce the ATS crash with a proprietary plugin under stress, but I can't share that stack trace. This fix can resolve the crash. I will keep current null_transform_request plugin and assert as the way to reproduce the bug. Please let me know if this is not sufficient or anything else we need other than assert and gdb.

[sxia-aviatrix]

Have updated the title and description

[bneradt]

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:

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.

[sxia-aviatrix]

This makes sense. I have use explicit free for vc.

[bneradt]

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

[sxia-aviatrix]

Good catch on the pairing. I traced both sites:

tunnel_handler_post_or_put() — Cannot be reached after this abort_tunnel(). It fires via HTTP_TUNNEL_EVENT_DONE, but abort_tunnel() sets active = false, marks all producers/consumers dead, and calls reset(). The tunnel will never deliver that callback again.

handle_server_setup_error() — Safe as you noted: abort_tunnel() calls reset(), so tunnel.get_consumer(post_transform_info.vc) returns nullptr and the inner guard short-circuits before touching .entry.

The entry == nullptr && vc != nullptr state also matches the existing pattern at line 2929 (tunnel_handler_post_or_put itself does this) and in state_common_wait_for_transform_read()'s TRANSFORM_FAIL case.

So I'll keep .vc non-null to stay consistent with the rest of the file.

[bneradt]

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.

[sxia-aviatrix]

You're right, removed. The assert was repro scaffolding used to confirm the stale entry via GDB — it's not part of the fix.

[bneradt]

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.

[bneradt]

+1 to Copilot on this one: _http_utils is never used — line 94 rebuilds the path inline. Please drop the class attribute.

[bneradt]

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.

[bneradt]

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.

[bneradt]

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.

[bneradt]

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.

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