bneradt commented on PR #13518:
URL: https://github.com/apache/trafficserver/pull/13518#issuecomment-5334611514
> ## 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:
> ```c++
> 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};
> };
> ```
>
>
>
>
>
>
>
>
>
>
>
> ```c++
> 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:
>
> ```c
> 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:
>
> ```c++
> TSStatus("[%s] accepted connection", PLUGIN_NAME);
> ```
>
> ```python
> 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.
Thank you for the review. The public layout dependency is removed in favor
of the original opaque handle ABI plus TSPortDescriptorDestroy(). Parse now
rejects fd-only descriptors, Accept retains defensive validation, ownership and
v11 upgrade guidance are documented, and the AuTest verifies the accept
callback fires without unexpected events.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
