This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit c6bfe1f4c29336783016969e6842bf64b3f5f019 Author: Mo Chen <[email protected]> AuthorDate: Tue Oct 21 17:24:19 2025 -0500 [header_rewrite] Fix set-body (#12560) 1. set-body with an empty string argument causes heap corruption in The cause is incorrect handling of zero-length strings in MIOBuffer::append_xmalloced. 2. set-body with variable expansion fails to expand the variable. The cause is a typo in the OperatorSetBody::exec() method. (cherry picked from commit 56ec03c35f144e1c2b6dd8742d9c46852e23b658) --- include/iocore/eventsystem/IOBuffer.h | 12 ++- plugins/header_rewrite/operators.cc | 7 +- src/iocore/eventsystem/P_IOBuffer.h | 4 + .../header_rewrite/header_rewrite_set_body.test.py | 110 +++++++++++++++++++++ 4 files changed, 129 insertions(+), 4 deletions(-) diff --git a/include/iocore/eventsystem/IOBuffer.h b/include/iocore/eventsystem/IOBuffer.h index 33c151ff05..de01b59a6a 100644 --- a/include/iocore/eventsystem/IOBuffer.h +++ b/include/iocore/eventsystem/IOBuffer.h @@ -102,8 +102,16 @@ enum AllocType { #define BUFFER_SIZE_INDEX_IS_FAST_ALLOCATED(_size_index) (((uint64_t)_size_index) < DEFAULT_BUFFER_SIZES) #define BUFFER_SIZE_INDEX_IS_CONSTANT(_size_index) (_size_index >= DEFAULT_BUFFER_SIZES) -#define BUFFER_SIZE_FOR_XMALLOC(_size) (-(_size)) -#define BUFFER_SIZE_INDEX_FOR_XMALLOC_SIZE(_size) (-(_size)) +#define BUFFER_SIZE_FOR_XMALLOC(_size) (-(_size)) +[[nodiscard]] constexpr int64_t +BUFFER_SIZE_INDEX_FOR_XMALLOC_SIZE(int64_t size) +{ + // Positive size indices are interpreted as a BUFFER_SIZE_INDEX_*. + // Negative size indices are interpreted as a malloc size. + // A zero size index is BUFFER_SIZE_INDEX_128, which causes this buffer to be freed incorrectly. + ink_release_assert(size > 0 && "Zero-length xmalloc buffer causes heap corruption!"); + return -size; +} #define BUFFER_SIZE_FOR_CONSTANT(_size) (_size - DEFAULT_BUFFER_SIZES) #define BUFFER_SIZE_INDEX_FOR_CONSTANT_SIZE(_size) (_size + DEFAULT_BUFFER_SIZES) diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index 46709e2a3f..9666231491 100644 --- a/plugins/header_rewrite/operators.cc +++ b/plugins/header_rewrite/operators.cc @@ -815,8 +815,11 @@ OperatorSetBody::exec(const Resources &res) const std::string value; _value.append_value(value, res); - char *msg = TSstrdup(_value.get_value().c_str()); - TSHttpTxnErrorBodySet(res.txnp, msg, _value.size(), nullptr); + char *msg = nullptr; + if (!value.empty()) { + msg = TSstrdup(value.c_str()); + } + TSHttpTxnErrorBodySet(res.txnp, msg, value.size(), nullptr); return true; } diff --git a/src/iocore/eventsystem/P_IOBuffer.h b/src/iocore/eventsystem/P_IOBuffer.h index a72ff11556..e159cbafc5 100644 --- a/src/iocore/eventsystem/P_IOBuffer.h +++ b/src/iocore/eventsystem/P_IOBuffer.h @@ -942,6 +942,10 @@ MIOBuffer::set(void *b, int64_t len) TS_INLINE void MIOBuffer::append_xmalloced(void *b, int64_t len) { + if (len == 0) { + return; + } + IOBufferBlock *x = new_IOBufferBlock_internal(_location); x->set_internal(b, len, BUFFER_SIZE_INDEX_FOR_XMALLOC_SIZE(len)); append_block_internal(x); diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body.test.py b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body.test.py new file mode 100644 index 0000000000..05c3b8b907 --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body.test.py @@ -0,0 +1,110 @@ +''' +''' +# 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. + +Test.Summary = ''' +Test for empty set-body operator value. +''' + +Test.ContinueOnFail = True + + +class HeaderRewriteSetBodyTest: + + def __init__(self): + self.setUpOriginServer() + self.setUpTS() + + def setUpOriginServer(self): + self.server = Test.MakeOriginServer("server") + + # Response for original transaction + request_header = {"headers": "GET /test HTTP/1.1\r\nHost: www.example.com\r\n\r\n", "body": ""} + + response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "body": "ATS should not serve this body"} + self.server.addResponse("sessionlog.log", request_header, response_header) + + def setUpTS(self): + self.ts = Test.MakeATSProcess("ts") + empty_body_rule_file = Test.Disk.File("empty_body_rule.conf", "txt", "") + empty_body_rule_file.WriteOn( + ''' + cond %{REMAP_PSEUDO_HOOK} + set-status 200 + + cond %{SEND_RESPONSE_HDR_HOOK} + set-body "" + ''') + self.ts.Disk.remap_config.AddLine( + f'map http://www.example.com/emptybody http://127.0.0.1:{self.server.Variables.Port}/test @plugin=header_rewrite.so @pparam={empty_body_rule_file.AbsRunTimePath}' + ) + + set_body_rule_file = Test.Disk.File("set_body_rule.conf", "txt", "") + set_body_rule_file.WriteOn( + ''' + cond %{REMAP_PSEUDO_HOOK} + set-status 200 + + cond %{SEND_RESPONSE_HDR_HOOK} + set-body "%{STATUS}" + ''') + self.ts.Disk.remap_config.AddLine( + f'map http://www.example.com/setbody http://127.0.0.1:{self.server.Variables.Port}/test @plugin=header_rewrite.so @pparam={set_body_rule_file.AbsRunTimePath}' + ) + + # Enable debug logging to help diagnose issues + self.ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http|header_rewrite', + }) + + def test_empty_body(self): + ''' + Test that empty set-body doesn't crash the server and properly deletes internal error body + ''' + tr = Test.AddTestRun() + tr.MakeCurlCommand(f'-s -v --proxy 127.0.0.1:{self.ts.Variables.port} "http://www.example.com/emptybody";', ts=self.ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(self.server) + tr.Processes.Default.StartBefore(self.ts) + tr.Processes.Default.Streams.stderr.Content = Testers.ContainsExpression("200 OK", "expected 200 response") + tr.Processes.Default.Streams.stderr.Content += Testers.ContainsExpression("Content-Length: 0", "expected content-length 0") + tr.Processes.Default.Streams.stdout.Content = Testers.ExcludesExpression("should not", "body should be removed") + + tr.StillRunningAfter = self.ts # Verify server didn't crash + + def test_set_body(self): + ''' + Test that set-body with a variable works correctly + ''' + tr = Test.AddTestRun() + tr.MakeCurlCommand(f'-s -v --proxy 127.0.0.1:{self.ts.Variables.port} "http://www.example.com/setbody";', ts=self.ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stderr.Content = Testers.ContainsExpression("200 OK", "expected 200 response") + tr.Processes.Default.Streams.stderr.Content += Testers.ContainsExpression("Content-Length: 3", "expected content-length 3") + tr.Processes.Default.Streams.stdout.Content = Testers.ContainsExpression("200", "body should be set to 200") + tr.Processes.Default.Streams.stdout.Content += Testers.ExcludesExpression("should not", "body should be removed") + + tr.StillRunningAfter = self.ts # Verify server didn't crash + + def run(self): + self.test_empty_body() + self.test_set_body() + + +HeaderRewriteSetBodyTest().run()
