Copilot commented on code in PR #13518:
URL: https://github.com/apache/trafficserver/pull/13518#discussion_r3738389670
##########
src/api/InkAPI.cc:
##########
@@ -23,6 +23,7 @@
#include <atomic>
#include <charconv>
+#include <new>
#include <tuple>
#include <unordered_map>
Review Comment:
InkAPI.cc now uses std::is_trivially_destructible_v but does not include
<type_traits> directly; it currently relies on transitive includes, which is
fragile and can break builds if header dependencies change.
##########
src/api/InkAPI.cc:
##########
@@ -7775,24 +7776,31 @@ TSHttpTxnCloseAfterResponse(TSHttpTxn txnp, int
should_close)
}
// Parse a port descriptor for the proxy.config.http.server_ports descriptor
format.
-TSPortDescriptor
-TSPortDescriptorParse(const char *descriptor)
+TSReturnCode
+TSPortDescriptorParse(const char *descriptor, TSPortDescriptor *result)
{
- HttpProxyPort *port = new HttpProxyPort();
+ static_assert(sizeof(TSPortDescriptor) == sizeof(HttpProxyPort));
+ static_assert(alignof(TSPortDescriptor) == alignof(HttpProxyPort));
+ static_assert(std::is_trivially_destructible_v<HttpProxyPort>);
- if (descriptor && port->processOptions(descriptor)) {
- return reinterpret_cast<TSPortDescriptor>(port);
+ if (descriptor == nullptr || result == nullptr) {
+ return TS_ERROR;
}
- delete port;
- return nullptr;
+ auto *port = new (result->_get_internal()) HttpProxyPort();
+
+ return port->processOptions(descriptor) ? TS_SUCCESS : TS_ERROR;
}
TSReturnCode
-TSPortDescriptorAccept(TSPortDescriptor descp, TSCont contp)
+TSPortDescriptorAccept(const TSPortDescriptor *descp, TSCont contp)
{
+ if (descp == nullptr || contp == nullptr) {
+ return TS_ERROR;
+ }
+
Action *action = nullptr;
- HttpProxyPort *port = reinterpret_cast<HttpProxyPort
*>(descp);
+ const HttpProxyPort *port = std::launder(reinterpret_cast<const
HttpProxyPort *>(descp->_get_internal()));
NetProcessor::AcceptOptions net(make_net_accept_options(port, -1 /* nthreads
*/));
Review Comment:
TSPortDescriptorAccept() passes the parsed port directly to the
NetProcessor, which has an ink_assert that the port is non-zero for INET
sockets. A caller mistake (e.g., accepting after a failed parse or with
uninitialized storage) could therefore trip an assertion. Consider validating
the parsed descriptor here and returning TS_ERROR instead of reaching the
assert.
##########
src/api/InkAPI.cc:
##########
@@ -7775,24 +7776,31 @@ TSHttpTxnCloseAfterResponse(TSHttpTxn txnp, int
should_close)
}
// Parse a port descriptor for the proxy.config.http.server_ports descriptor
format.
-TSPortDescriptor
-TSPortDescriptorParse(const char *descriptor)
+TSReturnCode
+TSPortDescriptorParse(const char *descriptor, TSPortDescriptor *result)
{
- HttpProxyPort *port = new HttpProxyPort();
+ static_assert(sizeof(TSPortDescriptor) == sizeof(HttpProxyPort));
+ static_assert(alignof(TSPortDescriptor) == alignof(HttpProxyPort));
+ static_assert(std::is_trivially_destructible_v<HttpProxyPort>);
Review Comment:
The size/alignment checks for TSPortDescriptor are stricter than necessary:
requiring exact equality will break compilation if HttpProxyPort ever shrinks
(even though the opaque storage would still be sufficient). Using >= keeps the
intended safety property while being more future-proof.
##########
src/api/InkAPITest.cc:
##########
@@ -1579,17 +1579,21 @@
REGRESSION_TEST(SDK_API_TSPortDescriptor)(RegressionTest *test, int /* atype ATS
TSContDataSet(server_cont, params);
TSContDataSet(client_cont, params);
- port = TSPortDescriptorParse(nullptr);
- if (port) {
- SDK_RPRINT(test, "TSPortDescriptorParse", "NULL port descriptor", TC_FAIL,
"TSPortDescriptorParse(NULL) returned %s", port);
+ if (TSPortDescriptorParse(nullptr, &port) != TS_ERROR) {
+ SDK_RPRINT(test, "TSPortDescriptorParse", "NULL port descriptor", TC_FAIL,
"TSPortDescriptorParse(NULL) returned TS_SUCCESS");
*pstatus = REGRESSION_TEST_FAILED;
return;
}
snprintf(desc, sizeof(desc), "%u", params->port);
- port = TSPortDescriptorParse(desc);
+ if (TSPortDescriptorParse(desc, &port) != TS_SUCCESS) {
+ SDK_RPRINT(test, "TSPortDescriptorParse", "Basic port descriptor",
TC_FAIL, "TSPortDescriptorParse(%s) returned TS_ERROR",
+ desc);
+ *pstatus = REGRESSION_TEST_FAILED;
+ return;
+ }
- if (TSPortDescriptorAccept(port, server_cont) == TS_ERROR) {
+ if (TSPortDescriptorAccept(&port, server_cont) == TS_ERROR) {
SDK_RPRINT(test, "TSPortDescriptorParse", "Basic port descriptor",
TC_FAIL, "TSPortDescriptorParse(%s) returned TS_ERROR",
desc);
Review Comment:
This failure branch is for TSPortDescriptorAccept(), but the regression
output still labels it as TSPortDescriptorParse(), which makes failures harder
to diagnose.
--
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]
