Make port descriptors caller-owned

[bneradt]

TSPortDescriptorParse allocates an HttpProxyPort that plugins cannot
release, so every parsed descriptor leaks for the lifetime of Traffic
Server. The API also lacks end-to-end coverage for accepting
connections on a parsed port.

This patch replaces the pointer handle with caller-owned opaque storage
whose size and alignment are checked against HttpProxyPort. It updates
API users and adds an AuTest plugin that listens on a dynamically
selected port.

Fixes: #6894

[Copilot]

Pull request overview

This PR addresses a long-standing leak in the TSPortDescriptor API by changing the API from returning an unfreeable heap-allocated handle to using caller-owned opaque storage, and adds end-to-end coverage to verify a plugin can actually listen on a parsed descriptor.

Changes:

• Redesign TSPortDescriptor to be caller-owned opaque storage; update TSPortDescriptorParse / TSPortDescriptorAccept signatures and all in-tree call sites.
• Add an AuTest plugin + gold test that parses a dynamically selected port descriptor and successfully accepts a connection.
• Remove the TSPortDescriptor regression-test leak suppression now that the leak is fixed.

Reviewed changes

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

Show a summary per file

File	Description
include/ts/apidefs.h.in	Redefines TSPortDescriptor as caller-owned opaque storage.
include/ts/ts.h	Updates API declarations and docs for the new parse/accept signatures and ownership model.
src/api/InkAPI.cc	Implements storage-backed parsing (placement-new) and updates accept to use the new descriptor representation.
src/api/InkAPITest.cc	Updates the regression test to use the new parse/accept API shape.
example/plugins/c-api/passthru/passthru.cc	Updates example plugin to use the new parse/accept signatures.
tests/tools/plugins/port_descriptor.cc	Adds an autest plugin that parses/accepts a descriptor and closes accepted connections.
tests/tools/plugins/CMakeLists.txt	Builds the new port_descriptor autest plugin.
tests/gold_tests/pluginTest/port_descriptor/port_descriptor.test.py	Adds a gold test that connects to the dynamically chosen descriptor port (via nc).
ci/asan_leak_suppression/regression.txt	Drops the suppression for the previously-leaking regression test.

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/api/InkAPI.cc:27

• InkAPI.cc now uses std::is_trivially_destructible_v but does not include <type_traits> directly, relying on transitive includes. Add the header here to keep dependencies explicit and avoid build breaks if include graphs change.

#include <tuple>

include/ts/apidefs.h.in:1155

• TSPortDescriptor’s opaque storage is hard-coded to 216 bytes with 8-byte alignment, while the implementation enforces exact size/alignment equality with HttpProxyPort. This is brittle across platform/compiler/flag variations and requires manual updates whenever HttpProxyPort layout changes. Consider generating the size/alignment into apidefs.h from the build (or providing headroom and using >= static_asserts) to reduce churn and portability risk.

class alignas(std::uint64_t) TSPortDescriptor
{
  friend TSReturnCode TSPortDescriptorParse(const char *, TSPortDescriptor *);
  friend TSReturnCode TSPortDescriptorAccept(const TSPortDescriptor *, struct tsapi_cont *);

private:
  std::byte _opaque[216];
};

[Copilot]

Pull request overview

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

Suppressed comments (1)

include/ts/apidefs.h.in:1156

• TSPortDescriptorParse() placement-news HttpProxyPort into result->_opaque. Right now _opaque itself has alignment 1 (std::byte), so correct alignment relies on _opaque staying the first member and the class-level alignas not changing. If a future change adds a member before _opaque, the placement-new could become misaligned and trigger UB. Align _opaque itself so its address is always sufficiently aligned regardless of member ordering.

private:
  std::byte _opaque[216];
  bool      _is_valid{false};
};

[Copilot]

Pull request overview

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

[cmcfarlen]

Review

The direction here is right: the leak is genuinely fixed, nullptr handling is now well-defined and tested, the new negative-path coverage in tests/tools/plugins/port_descriptor.cc is good, and CI is green everywhere. Comments below, most-important first.

1. Design question: baked-in sizeof vs. an explicit destroy function

_opaque[216] puts sizeof(HttpProxyPort) into the public plugin ABI. I measured locally (macOS/arm64): sizeof(HttpProxyPort) == 216, i.e. zero headroom. Consequences:

• Adding any field to HttpProxyPort — an internal, non-API struct — now breaks the core build (static_assert) and requires editing a public header. That's a maintenance tripwire on a struct that has grown repeatedly (m_allow_plain, m_mptcp, the unix-socket members were all recent additions).
• Worse, it's silent at runtime across versions: a plugin built against 11.0.0 headers and loaded into an ATS whose HttpProxyPort is larger gets a placement-new past the end of its buffer. That's a stack/heap overflow with no diagnostic — strictly more dangerous than the leak being fixed.

Two ways to keep the fix without that hazard:

• Preferred: keep a handle and add the missing lifetime call — TSPortDescriptorDestroy() (or return it via a documented TSfree-able allocation). This matches the prevailing ATS pattern (TSMimeHdrDestroy, TSUrlDestroy, ...), keeps HttpProxyPort internal, and since this PR is already labeled Incompatible the churn budget is the same.

• If you keep opaque storage: (a) round the reserve up with slack and a comment (// >= sizeof(HttpProxyPort); rounded up to leave room for new members), e.g. 256; and (b) make the mismatch detectable rather than fatal by having the header stamp the capacity and Parse check it:

  class alignas(std::max_align_t) TSPortDescriptor
  {
    ...
    std::byte     _opaque[256];
    std::uint32_t _capacity{sizeof(_opaque)};   // set by the plugin's header
    bool          _is_valid{false};
  };

  if (result->_capacity < sizeof(HttpProxyPort)) { return TS_ERROR; }   // stale plugin header

  That turns silent corruption into a clean TS_ERROR. Also worth a comment on HttpProxyPort in RecHttp.h pointing at the assert.

Also: alignas(std::uint64_t) is fine today (alignof(HttpProxyPort) == 8), but alignas(std::max_align_t) costs nothing and won't break if an over-aligned member ever appears.

2. Safety depends entirely on the implicit constructor running

_is_valid{false} protects "Accept before Parse" only for objects that are actually constructed. A plugin doing the very common C-ish thing:

TSPortDescriptor *d = TSmalloc(sizeof(*d));   // no constructor
TSPortDescriptorAccept(d, contp);             // _is_valid is garbage

reads a garbage HttpProxyPort and can crash inside main_accept. The docs currently say the storage is released "when the plugin deletes it", which implies new, but doesn't forbid malloc. Please state explicitly in TSPortDescriptorParse.en.rst and the ts.h comment that the storage must be default-constructed (automatic, static, or new) and that TSmalloc/memset storage is not valid. A _magic word checked in Accept would harden this further if you want belt-and-braces.

3. Parse accepts descriptors that Accept then rejects

HttpProxyPort::processOptions() returns true if it saw a port or a unix path or an fd=N token. So TSPortDescriptorParse("fd=5", &d) returns TS_SUCCESS with m_port == 0, and the new guard in TSPortDescriptorAccept() then returns TS_ERROR. Two notes:

• This isn't a regression — UnixNetProcessor.cc:118 has ink_assert(ip_family == AF_UNIX || 0 < local_port), so fd= descriptors previously aborted. Converting that to TS_ERROR is an improvement.
• But the new doc says Parse "returns TS_ERROR for ... invalid descriptor", which the fd= case contradicts. I'd move the family/port sanity check into Parse (keeping it in Accept as defense in depth) so the failure is reported where the plugin author can act on it, and add a sentence noting that fd=-only descriptors are unsupported by this API even though the config parser accepts them.
• Related pre-existing gap, not yours to fix, but maybe worth a doc line: a quic descriptor has isSSL() == false and gets accepted by netProcessor as TCP.

4. The autest doesn't verify the accept callback fires

nc -z 127.0.0.1 <port> succeeds as soon as something is listening — the test passes even if accept_connection() is never invoked, and the TS_EVENT_ERROR branch for an unexpected event is unobservable. Suggest emitting from the continuation and asserting on it:

TSStatus("[%s] accepted connection", PLUGIN_NAME);

ts.Disk.diags_log.Content += Testers.ContainsExpression(
    'port_descriptor.*accepted connection', 'plugin accepted the connection')

That makes the test actually cover the "accept on a parsed port" claim in the description. An ExcludesExpression on unexpected accept event would cover the error branch too.

Minor on the plugin: TSReleaseAssert in TSPluginInit turns a failure into an ATS abort. Fine for a test plugin, but TSError + non-registration would give a readable autest diagnostic instead of a crash log.

5. Docs / release notes

• The Incompatible label has no home in the docs. doc/release-notes/upgrading.en.rst only has an "Upgrading to ATS v10.x" section with a Changed TS API list. Since this lands on 11.0.0-dev, either start the v11 section or at least record TSPortDescriptorParse / TSPortDescriptorAccept somewhere plugin authors will look — the signature change is a hard compile break for out-of-tree plugins.
• TSPortDescriptorParse.en.rst declares .. class:: TSPortDescriptor but references it as :type:`TSPortDescriptor`. Docs CI is green so it resolves, but .. type:: would be more consistent with the rest of the API docs.
• The doc's statement that Accept "copies the information it needs and does not retain a pointer" is correct today (make_net_accept_options copies, m_fd is passed by value) — good that it's documented, since that's the property that makes stack storage safe.

Smaller things

• example/plugins/c-api/passthru/passthru.cc:299 — the descriptor declaration is now outside the aligned block; harmless, format CI is happy.
• InkAPITest.cc: splitting the Parse/Accept failure diagnostics is a good catch, that was mislabeled before.
• Dropping leak:RegressionTest_SDK_API_TSPortDescriptor from ci/asan_leak_suppression/regression.txt is the right proof that the leak is gone.

Verdict

I'd like item 1 settled before merge — as written, the fix trades a bounded leak for an unbounded, silent buffer overflow across version skew. Items 2-4 are small and worth doing in the same PR.