Copilot commented on code in PR #12868:
URL: https://github.com/apache/trafficserver/pull/12868#discussion_r2788094745
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -139,19 +139,49 @@ bypass_ok(HttpTransact::State *s)
return false;
}
+// wrapper to choose between a remap next hop strategy or use parent.config
+// remap next hop strategy is preferred
+inline static bool
+host_override(HttpTransact::State *s)
+{
+ bool res = false;
+ if (nullptr != s->next_hop_strategy) {
+ // remap strategies do not support the TSHttpTxnParentProxySet API.
+ res = s->next_hop_strategy->host_override;
+ } else if (nullptr != s->parent_params) {
+ res = s->parent_result.host_override();
+ }
+ return res;
+}
+
+// wrapper to choose between a remap next hop strategy or use parent.config
+// remap next hop strategy is preferred
+inline static bool
+is_some(HttpTransact::State *s)
+{
+ bool res = false;
+ if (nullptr != s->next_hop_strategy) {
+ // remap strategies do not support the TSHttpTxnParentProxySet API.
+ res = s->parent_result.result == ParentResultType::SPECIFIED;
+ } else if (nullptr != s->parent_params) {
+ res = s->parent_result.is_some();
+ }
Review Comment:
Similar to host_override(), is_some() falls back to parent_result.is_some()
whenever parent_params is non-null, which can assert if response_action.handled
set a SPECIFIED result without a parent_result.rec. Consider adding a
response_action.handled branch that treats SPECIFIED as "some" without
consulting rec.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -659,6 +699,22 @@ find_server_and_update_current_info(HttpTransact::State *s)
}
}
+ if (s->parent_result.result == ParentResultType::SPECIFIED &&
host_override(s)) {
+ char const *const hostname = s->parent_result.hostname;
+ TxnDbg(dbg_ctl_http_trans, "overriding host header with parent %s",
hostname);
+ if (!s->hdr_info.server_request.valid()) {
+
s->hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ s->hdr_info.client_request.mark_target_dirty();
+ } else {
+
s->hdr_info.server_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ s->hdr_info.server_request.mark_target_dirty();
+ }
+
+ if (nullptr != hostname) {
+ s->parent_info.name = s->arena.str_store(hostname, strlen(hostname));
+ }
+ }
+
switch (s->parent_result.result) {
case ParentResultType::SPECIFIED:
s->parent_info.name = s->arena.str_store(s->parent_result.hostname,
strlen(s->parent_result.hostname));
Review Comment:
In the host_override block, hostname is used with "%s" logging and passed to
value_set() before any nullptr check. If hostname is ever null (e.g., via
TSHttpTxnResponseActionSet), this is undefined behavior. Consider guarding
hostname up front (and skipping override if null) or logging with a safe
fallback string.
```suggestion
if (hostname != nullptr) {
TxnDbg(dbg_ctl_http_trans, "overriding host header with parent %s",
hostname);
if (!s->hdr_info.server_request.valid()) {
s->hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
s->hdr_info.client_request.mark_target_dirty();
} else {
s->hdr_info.server_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
s->hdr_info.server_request.mark_target_dirty();
}
s->parent_info.name = s->arena.str_store(hostname, strlen(hostname));
} else {
TxnDbg(dbg_ctl_http_trans, "host override requested but parent
hostname is null, skipping host header override");
}
}
switch (s->parent_result.result) {
case ParentResultType::SPECIFIED:
if (s->parent_result.hostname != nullptr) {
s->parent_info.name = s->arena.str_store(s->parent_result.hostname,
strlen(s->parent_result.hostname));
}
```
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -139,19 +139,49 @@ bypass_ok(HttpTransact::State *s)
return false;
}
+// wrapper to choose between a remap next hop strategy or use parent.config
+// remap next hop strategy is preferred
+inline static bool
+host_override(HttpTransact::State *s)
+{
+ bool res = false;
+ if (nullptr != s->next_hop_strategy) {
+ // remap strategies do not support the TSHttpTxnParentProxySet API.
+ res = s->next_hop_strategy->host_override;
+ } else if (nullptr != s->parent_params) {
+ res = s->parent_result.host_override();
+ }
Review Comment:
The new host_override() wrapper ignores response_action.handled, so when a
plugin sets TSHttpTxnResponseActionSet (and parent_result.rec may be
null/stale), calling parent_result.host_override() can trigger
ParentResult::is_some() assertions. Consider handling response_action.handled
explicitly here (e.g., return false, or base behavior solely on
parent_result.result without touching rec).
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -659,6 +699,22 @@ find_server_and_update_current_info(HttpTransact::State *s)
}
}
+ if (s->parent_result.result == ParentResultType::SPECIFIED &&
host_override(s)) {
+ char const *const hostname = s->parent_result.hostname;
+ TxnDbg(dbg_ctl_http_trans, "overriding host header with parent %s",
hostname);
+ if (!s->hdr_info.server_request.valid()) {
+
s->hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ s->hdr_info.client_request.mark_target_dirty();
+ } else {
+
s->hdr_info.server_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ s->hdr_info.server_request.mark_target_dirty();
+ }
+
+ if (nullptr != hostname) {
+ s->parent_info.name = s->arena.str_store(hostname, strlen(hostname));
+ }
Review Comment:
This assignment to parent_info.name is redundant because the SPECIFIED case
immediately below unconditionally overwrites parent_info.name from
parent_result.hostname. Consider removing this write (or refactoring so the
hostname is stored exactly once) to avoid confusion and double arena
allocations.
```suggestion
```
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -594,15 +628,19 @@ find_server_and_update_current_info(HttpTransact::State
*s)
TxnDbg(dbg_ctl_http_trans, "request is from localhost, so bypass parent");
s->parent_result.result = ParentResultType::DIRECT;
} else if (s->method == HTTP_WKSIDX_CONNECT &&
s->http_config_param->disable_ssl_parenting) {
+ /// BNO
Review Comment:
Stray "/// BNO" comment looks like a debugging artifact and doesn’t describe
behavior. It should be removed or replaced with a meaningful explanation of the
CONNECT/disable_ssl_parenting logic.
```suggestion
// For CONNECT (typically HTTPS tunneling) with disable_ssl_parenting
enabled, avoid sending
// the request to a forward parent proxy. We still run parent selection
to honor any origin
// server mappings, but if there is no usable parent, the result comes
from an API override,
// or the selected parent is itself a proxy, we bypass the parent and go
DIRECT instead.
```
--
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]
