Re: [PR] Add port descriptor destroy API [trafficserver]

["bneradt (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org