Copilot commented on code in PR #13406: URL: https://github.com/apache/trafficserver/pull/13406#discussion_r3611042925
########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject.test.py: ########## @@ -0,0 +1,83 @@ +''' +Exercise the rate_limit SNI limiter's reject path against a TLS listener, so the +consumer-driven SSLNetVConnection teardown frees every rejected handshake VC +cleanly (no use-after-free or crash). +''' +# 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. + +import os + +Test.Summary = __doc__ + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class RateLimitSniRejectTest: + """Drive rate_limit's SNI reject path and assert ATS frees the VCs without a fault.""" + + def __init__(self) -> None: + tr = Test.AddTestRun('rate_limit SNI reject teardown') + self._configure_trafficserver() + self._configure_client(tr) + + def _configure_trafficserver(self) -> None: + ts = Test.MakeATSProcess('ts', enable_tls=True, enable_cache=False) + self._ts = ts + ts.addDefaultSSLFiles() + for line in ['ssl_multicert:', ' - dest_ip: "*"', ' ssl_cert_name: server.pem', ' ssl_key_name: server.key']: + ts.Disk.ssl_multicert_yaml.AddLine(line) + + # One concurrent handshake for this SNI and no queue, so every further concurrent + # handshake is rejected outright (TS_EVENT_ERROR) rather than queued. Named .config + # (not .yaml) so autest treats it as a plain config file; the plugin parses it as + # YAML regardless (YAML::LoadFile). + ts.Disk.MakeConfigFile('rate_limit.config').AddLines([ + 'selector:', + ' - sni: rate.limited.com', + ' limit: 1', + ]) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.config') + + # Disable the freelist / ProxyAllocator so a freed SSLNetVConnection is really + # free()'d rather than recycled; a stale-VC access then hits freed memory + # instead of a still-valid recycled object. + ts.Command += ' -f -F' + + ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + + # The reject disposition is reached... + ts.Disk.traffic_out.Content = Testers.ContainsExpression('Rejecting connection', 'over-limit handshakes were rejected') + # ...and ATS tears every rejected handshake VC down without a memory-safety fault. + ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + 'use-after-free|attempting free|SEGV|received signal', 'ATS must survive the reject churn') + + def _configure_client(self, tr) -> None: Review Comment: Parameter `tr` is missing a type annotation. Gold test Python code in this repo generally annotates `TestRun` parameters to keep helpers type-checkable and consistent with the Python style used elsewhere. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py: ########## @@ -0,0 +1,89 @@ +''' +Regression test for a queue-accounting balance bug in the rate_limit SNI limiter: a +queued connection never reserves a slot, but its VCONN_CLOSE unconditionally releases +one, so a queued connection that closes underflows the active-slot counter and the next +reserve() trips a release assertion, aborting the server. ATS must survive the queue +churn. +''' +# 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. + +import os + +Test.Summary = __doc__ + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class RateLimitSniQueueTest: + """Churn the rate_limit SNI queue and assert the active-slot counter never underflows.""" + + def __init__(self) -> None: + tr = Test.AddTestRun('rate_limit SNI queue accounting') + self._configure_trafficserver() + self._configure_client(tr) + + def _configure_trafficserver(self) -> None: + ts = Test.MakeATSProcess('ts', enable_tls=True, enable_cache=False) + self._ts = ts + ts.addDefaultSSLFiles() + for line in ['ssl_multicert:', ' - dest_ip: "*"', ' ssl_cert_name: server.pem', ' ssl_key_name: server.key']: + ts.Disk.ssl_multicert_yaml.AddLine(line) + + # One concurrent handshake for this SNI and a queue that admits exactly one more. + # No rate and no max_age -- the sweep's resume path alone drives the scenario, with + # no rate-bucket or expiry timing to confound it. Named .config (not .yaml) so autest + # treats it as a plain config file; the plugin parses it as YAML regardless. + ts.Disk.MakeConfigFile('rate_limit.config').AddLines( + [ + 'selector:', + ' - sni: rate.limited.com', + ' limit: 1', + ' queue:', + ' size: 1', + ]) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.config') + + # Disable the freelist / ProxyAllocator so freed objects are really released rather + # than recycled, keeping allocation reuse from masking a stale access. + ts.Command += ' -f -F' + + ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + + # The queue path is reached... + ts.Disk.traffic_out.Content = Testers.ContainsExpression('Queueing the VC', 'a connection was queued') + # ...and the active-slot counter never underflows into the release assertion. Match + # both the specific assertion (pins the failure to this bug) and the generic abort. + ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + '_active <= _limit|received signal', 'the active-slot counter must not underflow and abort ATS') + + def _configure_client(self, tr) -> None: Review Comment: Parameter `tr` is missing a type annotation. Other gold tests typically annotate `TestRun` parameters, which helps keep these helper methods consistent and type-checkable. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry_client.sh: ########## @@ -0,0 +1,63 @@ +#!/usr/bin/env bash +# +# 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. +# +# Exercise the max_age EXPIRY branch of the rate_limit SNI queue accounting. A queued +# connection never reserves a slot; when the sweep expires it (max_age), it must be +# detached so its close does not release a slot it never held. Otherwise the expiry is an +# unmatched decrement of the active-slot counter, and -- combined with the holder's own +# close -- the counter wraps below zero and the limiter's release assertion aborts ATS. +# +# 1. holder completes its handshake and holds the single slot (counter = 1); +# 2. one connection enqueues (slot full) and stays parked -- it is NOT disconnected, so +# only the sweep's max_age expiry removes it; +# 3. after max_age the sweep errors it out -> (unfixed) unmatched decrement -> counter 1->0; +# 4. the holder is closed; its matched decrement lands on the understated counter -> wrap; +# 5. a probe connection's reserve() observes the wrapped counter and the assertion aborts. +# +# args: host port sni +set -u +host="$1" +port="$2" +sni="$3" + +OSSL="openssl s_client -connect ${host}:${port} -servername ${sni} -quiet -verify_quiet -no_ign_eof" + +# 1. Holder: hold the single slot. Its stdin is a FIFO on fd 3 so we end it in step 4. +fifo="$(mktemp -u "${TMPDIR:-/tmp}/rl_holder.XXXXXX")" +mkfifo "$fifo" +${OSSL} <"$fifo" >/dev/null 2>&1 & +exec 3<>"$fifo" +rm -f "$fifo" Review Comment: `mktemp -u` is non-atomic and can race/collide; using `mktemp -d` and creating the FIFO inside that directory avoids TOCTOU issues and reduces flakiness. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh: ########## @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# +# 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. +# +# Deterministically drive the rate_limit SNI limiter's queue-accounting balance bug. A +# queued connection never increments the active-slot counter, but its VCONN_CLOSE always +# decrements it, so one queued connection that closes is a single unmatched decrement. +# With exactly one queued connection there is no way for the sweep to mask it: +# +# 1. holder completes its handshake and holds the single slot (counter = 1); +# 2. one connection enqueues (slot full), then closes cleanly (FIN) while parked; +# 3. the sweep resumes it, its handshake fails and it closes -> one unmatched decrement +# -> counter 1 -> 0 (the queue is now empty, so no reserve() can rebalance it); +# 4. the holder is closed; its matched decrement lands on the understated counter +# -> counter 0 -> wraps below zero; +# 5. a probe connection's reserve() observes the wrapped counter and the limiter's +# release assertion (_active <= _limit) aborts the server. +# +# args: host port sni +set -u +host="$1" +port="$2" +sni="$3" + +OSSL="openssl s_client -connect ${host}:${port} -servername ${sni} -quiet -verify_quiet -no_ign_eof" + +# 1. Holder: hold the single slot. Its stdin is a FIFO kept open on fd 3, so we end the +# holder deterministically in step 4 (closing fd 3 -> EOF -> clean TLS close -> FIN). +fifo="$(mktemp -u "${TMPDIR:-/tmp}/rl_holder.XXXXXX")" +mkfifo "$fifo" +${OSSL} <"$fifo" >/dev/null 2>&1 & +exec 3<>"$fifo" +rm -f "$fifo" Review Comment: `mktemp -u` is non-atomic and can race/collide; it can also be problematic in shared CI environments. Using `mktemp -d` and creating the FIFO inside the directory avoids TOCTOU issues and makes the test less flaky. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry.test.py: ########## @@ -0,0 +1,87 @@ +''' +Regression test for the max_age expiry branch of the rate_limit SNI queue accounting. A +queued connection never reserves a slot, so when the sweep expires it the plugin must +detach it rather than release a slot it never held; otherwise the expiry underflows the +active-slot counter and the next reserve() trips a release assertion, aborting the server. +ATS must survive the expiry. +''' +# 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. + +import os + +Test.Summary = __doc__ + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class RateLimitSniExpiryTest: + """Age a queued connection out via max_age and assert the active-slot counter stays balanced.""" + + def __init__(self) -> None: + tr = Test.AddTestRun('rate_limit SNI queue max_age expiry') + self._configure_trafficserver() + self._configure_client(tr) + + def _configure_trafficserver(self) -> None: + ts = Test.MakeATSProcess('ts', enable_tls=True, enable_cache=False) + self._ts = ts + ts.addDefaultSSLFiles() + for line in ['ssl_multicert:', ' - dest_ip: "*"', ' ssl_cert_name: server.pem', ' ssl_key_name: server.key']: + ts.Disk.ssl_multicert_yaml.AddLine(line) + + # One concurrent handshake for this SNI, a one-deep queue, and a 1s max age so the + # sweep expires the queued connection. Named .config (not .yaml) so autest treats it + # as a plain config file; the plugin parses it as YAML regardless. + ts.Disk.MakeConfigFile('rate_limit.config').AddLines( + [ + 'selector:', + ' - sni: rate.limited.com', + ' limit: 1', + ' queue:', + ' size: 1', + ' max_age: 1', + ]) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.config') + + # Disable the freelist / ProxyAllocator so allocation behavior is not a confound; the + # abort under test is a release-assertion, and this keeps the run representative of CI. + ts.Command += ' -f -F' + + ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + + # The expiry branch is actually reached... + ts.Disk.traffic_out.Content = Testers.ContainsExpression('too old', 'a queued connection was expired') + # ...and expiring it does not underflow the active-slot counter into the release assertion. + ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + '_active <= _limit|received signal', 'expiring a queued connection must not underflow and abort ATS') + + def _configure_client(self, tr) -> None: Review Comment: Parameter `tr` is missing a type annotation. The surrounding gold test codebase generally annotates `TestRun` parameters, so updating this keeps the new test aligned with existing conventions. -- 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]
