traeak commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r4169179695
##########
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:
If an address is reused, contains() only succeeds when the pointer is a live
strategy in the transaction's current factory, so the core never dereferences
freed memory; the worst case is a different valid strategy for a plugin that
kept a handle across a reload against the documented contract. Making stale
handles provably invalid would mean turning TSStrategy into a non-reused ID
with a global registry, which is out of proportion here. Docs now describe the
check as best-effort.
--
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]
