Copilot commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r4117844101
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -166,23 +175,22 @@ NextHopStrategyFactory::createStrategy(const std::string
&name, const NHPolicyTy
NextHopSelectionStrategy *
NextHopStrategyFactory::strategyInstance(const char *name) const
{
- NextHopSelectionStrategy *ps_strategy = nullptr;
-
- if (!strategies_loaded) {
- NH_Error("no strategy configurations were defined, see definitions in '%s'
file", fn.c_str());
+ // Read-only lookup: this is reachable from transaction threads via the
+ // TSHttpTxnNextHopStrategyFind / TSRemapNextHopStrategyFind APIs, so it
+ // must not mutate strategy state. Load failures are logged once, at
+ // construction, and 'distance' is precomputed there.
+ if (!strategies_loaded || name == nullptr) {
return nullptr;
- } else {
- auto it = _strategies.find(name);
- if (it == _strategies.end()) {
- // NH_Error("no strategy found for name: %s", name);
- return nullptr;
- } else {
- ps_strategy = it->second;
- ps_strategy->distance = std::distance(_strategies.begin(), it);
- }
}
- return ps_strategy;
+ auto it = _strategies.find(name);
+ return (it == _strategies.end()) ? nullptr : it->second;
+}
+
+bool
+NextHopStrategyFactory::contains(NextHopSelectionStrategy const *strategy)
const
+{
+ return std::any_of(_strategies.begin(), _strategies.end(), [strategy](auto
const &entry) { return entry.second == strategy; });
Review Comment:
This provenance check linearly scans every strategy on each
TSHttpTxnNextHopStrategySet call. The updated header_rewrite global path and
Lua path invoke Set during request processing, so a strategy table with N
entries adds O(N) work to each such transaction; keep an owner pointer or
pointer index so the validation remains O(1).
##########
src/api/InkAPI.cc:
##########
@@ -5472,57 +5472,142 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
}
-void
-TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void const *stratptr)
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
- // null strategy falls back to parent.config
- // sdk_assert(sdk_sanity_check_null_ptr(strategy) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
- auto sm = reinterpret_cast<HttpSM *>(txnp);
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const *>(stratptr);
+ // m_remap is null if no rewrite table existed when the transaction started.
+ if (sm->m_remap.get() == nullptr || sm->m_remap->strategyFactory == nullptr)
{
+ return nullptr;
+ }
- sm->t_state.next_hop_strategy = const_cast<NextHopSelectionStrategy
*>(strategy);
+ // HttpSM has a reference count handle to UrlRewrite which has a
+ // pointer to NextHopStrategyFactory
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
+
+ return reinterpret_cast<TSStrategy>(strategy);
}
-char const *
-TSHttpNextHopStrategyNameGet(void const *stratptr)
+void
+TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, TSStrategy stratptr)
{
- char const *name = nullptr;
- if (nullptr != stratptr) {
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const
*>(stratptr);
- name = strategy->strategy_name.c_str();
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM *>(txnp);
+
+ // A null strategy falls back to parent.config.
+ if (stratptr == nullptr) {
+ sm->t_state.next_hop_strategy = nullptr;
+ return;
}
- return name;
+ // Provenance guard: the core dereferences this pointer during parent
+ // selection, so reject handles that are not live strategies in this
+ // transaction's own factory (e.g. one cached across a config reload).
+ auto const candidate = reinterpret_cast<NextHopSelectionStrategy
*>(stratptr);
+ if (sm->m_remap == nullptr || sm->m_remap->strategyFactory == nullptr ||
!sm->m_remap->strategyFactory->contains(candidate)) {
+ TSError("%s: strategy %p is not present in the live strategy factory;
ignoring", __func__, stratptr);
+ return;
+ }
+
+ sm->t_state.next_hop_strategy = candidate;
}
-void const *
-TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name)
+namespace
+{
+// Logs once per call site; a misbehaving plugin may call on every transaction.
+void
+log_outside_remap_init(char const *func, std::atomic<bool> &logged)
+{
+ if (!logged.exchange(true, std::memory_order_relaxed)) {
+ TSError("%s: must be called during remap plugin initialization (no loading
remap rule); returning nullptr", func);
+ }
+}
+} // namespace
+
+TSStrategy
+TSRemapNextHopStrategyFind(const char *name)
{
- sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
- auto sm = reinterpret_cast<HttpSM const *>(txnp);
+ // Outside of remap rule initialization (e.g. a globally loaded plugin)
+ // there is no loading mapping on this thread.
+ auto const um = url_mapping::instance;
+ if (um == nullptr) {
+ static std::atomic<bool> logged{false};
+ log_outside_remap_init(__func__, logged);
+ return nullptr;
+ }
- sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap.get()) ==
TS_SUCCESS);
- sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+ // No strategies.yaml is loaded.
+ if (um->strategyFactory == nullptr) {
+ return nullptr;
+ }
- // HttpSM has a reference count handle to UrlRewrite which has a
- // pointer to NextHopStrategyFactory
- NextHopSelectionStrategy const *const strat =
sm->m_remap->strategyFactory->strategyInstance(name);
+ // The loading UrlRewrite manages the NextHopStrategyFactory pointer.
+ return
reinterpret_cast<TSStrategy>(um->strategyFactory->strategyInstance(name));
+}
+
+void
+TSRemapNextHopStrategySet(TSStrategy strategy)
+{
+ auto const um = url_mapping::instance;
+ if (um == nullptr) {
+ TSError("%s: must be called during remap plugin initialization (no loading
remap rule)", __func__);
+ return;
+ }
+
+ if (strategy == nullptr) {
+ um->strategy = nullptr;
+ return;
+ }
- return static_cast<void const *>(strat);
+ // Compare by pointer only: a stale handle may point at freed memory.
+ auto const candidate = reinterpret_cast<NextHopSelectionStrategy
*>(strategy);
+ if (um->strategyFactory == nullptr ||
!um->strategyFactory->contains(candidate)) {
+ TSError("%s: strategy %p is not present in the loading strategy factory;
ignoring", __func__, strategy);
+ return;
+ }
+
+ um->strategy = candidate;
+}
+
+TSStrategy
+TSRemapNextHopStrategyGet()
+{
+ auto const um = url_mapping::instance;
+ if (um == nullptr) {
+ static std::atomic<bool> logged{false};
+ log_outside_remap_init(__func__, logged);
+ return nullptr;
+ }
+ return reinterpret_cast<TSStrategy>(um->strategy);
+}
+
+char const *
+TSNextHopStrategyNameGet(TSStrategy stratptr)
+{
+ // A null handle has no name; the config-layer "null" sentinel is a
+ // separate concern and must not be conflated with a missing strategy.
+ if (stratptr == nullptr) {
+ return nullptr;
Review Comment:
The implementation returns nullptr for a null strategy, but the PR's API
contract says this utility should return the null-terminated string "null" for
a nullptr handle. This also conflicts with the new public documentation and the
added unit test, so callers using the utility directly cannot observe the
promised sentinel; return a stable "null" string and align the docs/test with
that contract.
--
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]
