bryancall commented on code in PR #13116:
URL: https://github.com/apache/trafficserver/pull/13116#discussion_r3149425509
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7578,12 +7617,19 @@
HttpSM::setup_client_request_plugin_agents(HttpTunnelProducer *p, int num_header
inline void
HttpSM::transform_cleanup(TSHttpHookID hook, HttpTransformInfo *info)
{
+ if (info->entry == nullptr) {
+ return;
+ }
APIHook *t_hook = api_hooks.get(hook);
if (t_hook && info->vc == nullptr) {
do {
- VConnection *t_vcon = t_hook->m_cont;
- t_vcon->do_io_close();
- t_hook = t_hook->m_link.next;
+ APIHook *next = t_hook->m_link.next;
+ // Some transform hooks can already be detached by the time kill_this()
runs.
+ // Guard against null continuations while still draining the remaining
hooks.
+ if (auto *t_vcon = static_cast<VConnection *>(t_hook->m_cont); t_vcon !=
nullptr) {
+ t_vcon->do_io_close();
+ }
+ t_hook = next;
} while (t_hook != nullptr);
}
Review Comment:
Validated this path with AuTests. Removing the info->entry guard
reintroduced the same -11 crash in transform_cleanup() for
header_rewrite_set_body_origin. Kept the guard, added rationale comments, and
re-ran header_rewrite_set_body_origin + filter_body + full ctest + -R 1
regression in this round.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7664,7 +7710,11 @@ HttpSM::kill_this()
// In that case, we need to manually close all the
// transforms to prevent memory leaks (INKqa06147)
if (hooks_set) {
- transform_cleanup(TS_HTTP_RESPONSE_TRANSFORM_HOOK, &transform_info);
+ bool bypassed_response_transform =
+ t_state.api_info.cache_untransformed && t_state.internal_msg_buffer &&
!t_state.api_server_request_body_set;
+ if (!bypassed_response_transform) {
+ transform_cleanup(TS_HTTP_RESPONSE_TRANSFORM_HOOK, &transform_info);
+ }
Review Comment:
Validated this suggestion directly: always invoking response
transform_cleanup() in the bypass case reintroduced a transform_cleanup()
segfault (-11). Keeping the bypass-specific skip here is intentional, and the
code now documents why. Tests rerun: header_rewrite_set_body_origin,
filter_body, full ctest, full -R 1 regression.
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml:
##########
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Test set-body origin replacement without response transforms'
Review Comment:
Good catch. Updated replay metadata wording to reflect that null_transform
is loaded globally, but selected blocks intentionally keep transform inactive.
Included in commit 6f2dfda5e7.
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml:
##########
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Test set-body origin replacement without response transforms'
+
+ dns:
+ name: 'dns'
+
+ server:
+ name: 'server'
+
+ client:
+ name: 'client'
+ process_config:
+ other_args: '--thread-limit 1'
+
+ ats:
+ name: 'ts'
+
+ plugin_config:
+ - null_transform.so
Review Comment:
Addressed with the same clarification update: replay description/comments
now explicitly describe transform-loaded vs transform-inactive paths. Included
in commit 6f2dfda5e7.
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml:
##########
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Test set-body origin replacement without response transforms'
+
+ dns:
+ name: 'dns'
+
+ server:
+ name: 'server'
+
+ client:
+ name: 'client'
+ process_config:
+ other_args: '--thread-limit 1'
+
+ ats:
+ name: 'ts'
+
+ plugin_config:
+ - null_transform.so
+
+ copy_to_config_dir:
+ - 'rules'
+
+ records_config:
+ proxy.config.http.insert_response_via_str: 0
+ proxy.config.http.cache.http: 0
Review Comment:
Agreed. Renamed these notes from cache-bypass probes to repeated-URL probes
so the terminology matches cache-disabled test behavior. Included in commit
6f2dfda5e7.
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml:
##########
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Test set-body origin replacement without response transforms'
+
+ dns:
+ name: 'dns'
+
+ server:
+ name: 'server'
+
+ client:
+ name: 'client'
+ process_config:
+ other_args: '--thread-limit 1'
+
+ ats:
+ name: 'ts'
+
+ plugin_config:
+ - null_transform.so
+
+ copy_to_config_dir:
+ - 'rules'
+
+ records_config:
+ proxy.config.http.insert_response_via_str: 0
+ proxy.config.http.cache.http: 0
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: 'http|header_rewrite'
+
+ remap_config:
+ # Block 1: READ_RESPONSE hook replacement path (no transform plugin).
+ - from: "http://www.example.com/set_body_read_resp_403/";
+ to: "http://backend.ex:{SERVER_HTTP_PORT}/origin_read_403/";
+ plugins:
+ - name: "header_rewrite.so"
+ args:
+ - "rules/rule_set_body_origin_read_resp.conf"
+
+ # Block 2: SEND_RESPONSE hook replacement path (no transform plugin).
+ - from: "http://www.example.com/set_body_send_resp_403/";
+ to: "http://backend.ex:{SERVER_HTTP_PORT}/origin_send_403/";
+ plugins:
+ - name: "header_rewrite.so"
+ args:
+ - "rules/rule_set_body_origin_send_resp.conf"
+
+ # Block 3a: cache-bypass probe for READ_RESPONSE hook (same URL twice).
Review Comment:
Addressed. Updated replay comments to repeated-URL probe wording (cache
remains intentionally disabled). Included in commit 6f2dfda5e7.
##########
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_origin.replay.yaml:
##########
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Test set-body origin replacement without response transforms'
+
+ dns:
+ name: 'dns'
+
+ server:
+ name: 'server'
+
+ client:
+ name: 'client'
+ process_config:
+ other_args: '--thread-limit 1'
+
+ ats:
+ name: 'ts'
+
+ plugin_config:
+ - null_transform.so
+
+ copy_to_config_dir:
+ - 'rules'
+
+ records_config:
+ proxy.config.http.insert_response_via_str: 0
+ proxy.config.http.cache.http: 0
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: 'http|header_rewrite'
+
+ remap_config:
+ # Block 1: READ_RESPONSE hook replacement path (no transform plugin).
+ - from: "http://www.example.com/set_body_read_resp_403/";
+ to: "http://backend.ex:{SERVER_HTTP_PORT}/origin_read_403/";
+ plugins:
+ - name: "header_rewrite.so"
+ args:
+ - "rules/rule_set_body_origin_read_resp.conf"
+
+ # Block 2: SEND_RESPONSE hook replacement path (no transform plugin).
+ - from: "http://www.example.com/set_body_send_resp_403/";
+ to: "http://backend.ex:{SERVER_HTTP_PORT}/origin_send_403/";
+ plugins:
+ - name: "header_rewrite.so"
+ args:
+ - "rules/rule_set_body_origin_send_resp.conf"
+
+ # Block 3a: cache-bypass probe for READ_RESPONSE hook (same URL twice).
+ - from: "http://www.example.com/cache_probe_read/";
+ to: "http://backend.ex:{SERVER_HTTP_PORT}/cache_probe_read/";
+ plugins:
+ - name: "header_rewrite.so"
+ args:
+ - "rules/rule_set_body_origin_read_resp.conf"
+
+ # Block 3b: cache-bypass probe for SEND_RESPONSE hook (same URL twice).
Review Comment:
Addressed in the same replay cleanup: renamed cache-bypass wording to
repeated-URL probe for correctness. Included in commit 6f2dfda5e7.
--
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]
