Copilot commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r4169130127
##########
src/api/InkAPI.cc:
##########
@@ -5472,57 +5472,143 @@ 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;
Review Comment:
The ownership check is based only on the raw strategy address, so it cannot
distinguish a handle retained from an older config generation if the allocator
reuses that address for a strategy in the new factory. In that ABA case
`contains()` succeeds and the core accepts/dereferences the stale handle,
contrary to the documented reload-safety guarantee. The handle needs
generation/owner identity that survives object address reuse, or another
validation mechanism that makes old handles unambiguously invalid.
--
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]
